Analysis of bugs in Orchard CMS

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform. Software IP management and project development governance are provided by Outercurve Foundation, a nonprofit fund.

For us, the developers of the PVS-Studio static analyzer, this is another chance to check an interesting project, tell people (and developers) about the errors we find and test our analyzer, of course. Today we’ll speak about the errors we found in the Orchard CMS project.

About the Orchard CMS project

The Orchard CMS project was announced in March of 2010 with the release of the first beta-version of the project. The creators of Orchard CMS set a goal of creating a content management system on a new successful framework, ASP.NET MVC, that would meet the following demands:

An open and free project that is built on community requests;

A fast engine with a modular architecture, and all necessary CMS means;

A public online gallery of modules, themes, and other components for extension from the community;

High quality typography, attention to the layout and markup of pages.

Focus on creating a comfortable and functional administration panel;

Quick desktop deployment of the system, and easy publishing on the server.

Initially, the Orchard project and its source code was licensed under a free MS-PL license, but with the release of the first public version, the project changed the license to a simpler and more widespread New BSD License.

Four pre-release versions were released within a year, until Orchard CMS reached version 1.0. All this time the developers kept in touch with the community, accepting requests, considering comments, and fixing bugs. The project was launched on the codeplex.com portal in order to get user feedback — http://orchard.codeplex.com/.

Today you can find exhaustive documentation on all aspects of Orchard CMS, participate in discussions on the project on the forums, report bugs to the bugtracker, download the latest source code and binary builds of the project.

In addition to the page for developers, an official web site, was launched that contains all the necessary documentation to work with Orchard CMS. Besides that, the official site has a gallery of modules and other components, created by the community, in order to enlarge the functionality of Orchard CMS.

The analysis results

We did the analysis of all source code files (3739 items) with the .cs extension. In sum total there were 214 564 lines of code.

The result of the check was 137 warnings. To be more precise, there were 39 first (high) level warnings. 28 of them clearly pointed to a problem fragment or an error. There were also 60 second (medium) level warnings. In my subjective opinion, 31 of these warnings were correct and indicated fragments that contained real bugs or typos. We aren’t going to look at the lowest level warnings, because these warnings don’t usually indicate real errors, are made up of quite a large number of false positives, and contain warnings that aren’t relevant for most of the projects.

So, let’s take a look at the most interesting of the bugs that we found. The project authors can do a more detailed review of the bugs by doing the project check themselves, or making a request for a temporary license.

Also, as far as I understand, the Orchard CMS developers are already using ReSharper in their project. I drew this conclusion based on the following:

We do not like the idea of comparing PVS-Studio and ReSharper, because they are two different types of tool. But as you can see, using ReSharper did not prevent us from finding real errors in the code.

The first error on our list is quite a widespread typo that will lead to an infinite recursion and StackOverflow exception, which will also cause the program to crash once the value of the ReturnTypeCustomAttributes is gotten. Most likely, the programmer wanted to return, from the property, a field of the same name as the object _dynamicMethod; then, the correct variant should be like this:

Another fairly common typo, which occurs because of the TimeSpan type implementation. The comment shows that this method should block the database query, if there were less than 5 seconds since the previous query. But apparently, the programmer did not know that the Seconds property of the object of TimeSpan type returns not the total number of seconds in this interval, but the remaining number of seconds.

For example, if the time interval is 1 minute and 4 seconds, then the call of the Seconds method will return only 4 seconds. If it is necessary to return a total number of seconds, we should use the property TotalSeconds. For this example it will be 64 seconds.

This code fragment is quite bulky, so it’s hard to notice the error. In this case we have the enum TypeCode, and a method InfosetFieldIndexingHandler, where it is checked, to which element of the enum the typeCode variable belongs. The programmer most likely forgot about one small Byte element, but added SByte. Because of this error, Byte processing will be ignored. It would be easier to avoid this error if the programmer added a default block, where the exception is thrown in regards to an unhandled enum items.

No handling the return value from the Except method

V3010 The return value of function ‘Except’ is required to be utilized. AdminController.cs 140

As is well known, the Except method eliminates from the collection, for which it is called, items of another collection. Perhaps the programmer wasn’t aware of this, or did not pay attention to the fact that the result of this method returns a new collection. Then the correct variant may be as follows:

Operation precedence in a statement

localPart.Name + "." + localField.Name + "." + storageName ?? ""
The programmer thought that the ?? operator would verify only the variable storageName against null and if so, then instead of it, an empty string will added to the expression. But since the + operator has a supercedes the ??, the whole expression will be verified against null. It should be noted that we’ll get the same string without any changes if we add null to the string. Thus, in this case we can safely remove this redundant misleading check. The correct variant can look like this:

localPart.Name + "." + localField.Name + "." + storageName
The analyzer found one more similar bug:

We see that the iterator of the i loop will always range in value from 0 to 3, but despite this, the programmer checks if the iterator has value of 4 inside the body of the loop. The check will be never executed, but it’s hard for me to say how dangerous this bug is in the scope of the whole project without knowing the real goal of this code fragment.

There were other similar bugs found. All of them denote that the check will be either true or false.

As you can see from the code above (we are interested in lines 2 and 3), the programmer first checks the access to the Id property from the container, and then checks the container for null. It would be correct to verify against null, and then access the container elements.

Next fragment:

V3095 The ‘Delegate’ object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37

PVS-Studio found two more errors that are just the same as the one described above, so I won’t go into them here.

V3095 The ‘Delegate’ object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37

V3095 The ‘widget’ object was used before it was verified against null. Check lines: 81, 93. TagsWidgetCommands.cs 81

Conclusion

There were many more errors, typos and issues found in this project. But they didn’t seem interesting enough to describe in this article. The Orchard CMS developers can easily find all the issues, using the PVS-Studio tool. You may also find bugs in your projects with the help of the mentioned tool.

I would like to mention that the greatest benefit of static analysis is found in its regular use. There’s no real use in downloading the tool and doing a one-time check — that could not be considered to be serious analysis. For example, programmers regularly review the compiler warnings; not just 3 times a year before the release. If the analyzer is used on a regular basis, it will significantly save time which is usually spent on searching for typos and errors.

Comments

Most of us work with strings one way or another. There’s no way to avoid them — when writing code, you’re doomed to concatinate strings every day, split them into parts and access certain characters by index. We are used to the fact that strings are fixed-length arrays of characters, which leads to certain limitations when working with them. For instance, we cannot quickly concatenate two strings. To do this, we will at first need to allocate the required amount of memory, and then copy there the data from the concatenated strings.