Sofi Fateeva

Analysis of bugs in Orchard CMS

Programming

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 description of the project is taken from the site http://www.orchardproject.net/mission.

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:

  1. An open and free project that is built on community requests;
  2. A fast engine with a modular architecture, and all necessary CMS means;
  3. A public online gallery of modules, themes, and other components for extension from the community;
  4. High quality typography, attention to the layout and markup of pages.
  5. Focus on creating a comfortable and functional administration panel;
  6. 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:

https://github.com/OrchardCMS/Orchard-Harvest-Website/blob/master/src/Orchard.4.5.resharper

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.

Infinite recursion

V3110 Possible infinite recursion inside ‘ReturnTypeCustomAttributes’ property. ContentItemAlteration.cs 121

public override ICustomAttributeProvider 
  ReturnTypeCustomAttributes 
{
  get { return ReturnTypeCustomAttributes; }
}

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:

public override ICustomAttributeProvider

ReturnTypeCustomAttributes
{
  get { return _dynamicMethod.ReturnTypeCustomAttributes; }
}

And here’s another typo which will cause an infinite recursion, and, as a consequence, throw a StackOverflow exception.

V3110 Possible infinite recursion inside ‘IsDefined’ method. ContentItemAlteration.cs 101

public override bool IsDefined(Type attributeType, bool inherit) {
  return IsDefined(attributeType, inherit);
}

In this case the IsDefined method calls itself. I think the programmer wanted to call a method with the same name as the object _dynamicMethod. Then the correct version would look like this:

public override bool IsDefined(Type attributeType, bool inherit) {
  return _dynamicMethod.IsDefined(attributeType, inherit);
}

When a minute doesn’t always have 60 seconds

V3118 Seconds componentof TimeSpan is used, which does not represent full time interval. Possibly ‘TotalSeconds’ value was intended instead. AssetUploader.cs 182

void IBackgroundTask.Sweep() 
{
  ....
  // Don't flood the database with progress updates; 
  // Limit it to every 5 seconds.
  if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) 
  {
    ....
  }
}

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.

Then the correct code could be written as such:

void IBackgroundTask.Sweep() 
{
  ....
  // Don't flood the database with progress updates; 
  // Limit it to every 5 seconds.
  if ((_clock.UtcNow - lastUpdateUtc).TotalSeconds >= 5) // <=
  {
    ....
  }
}

Missing enumeration value during the switch check

V3002 The switch statement does not cover all values of the ‘TypeCode’ enum: Byte. InfosetFieldIndexingHandler.cs 65

public enum TypeCode
{
  Empty = 0,
  Object = 1,
  DBNull = 2,
  Boolean = 3,
  Char = 4,
  SByte = 5,
  Byte = 6,
  Int16 = 7,
  UInt16 = 8,
  Int32 = 9,
  UInt32 = 10,
  Int64 = 11,
  UInt64 = 12,
  Single = 13,
  Double = 14,
  Decimal = 15,
  DateTime = 16,
  String = 18
}
public InfosetFieldIndexingHandler(....)
{
  ....
  var typeCode = Type.GetTypeCode(t);
  switch (typeCode) {
    case TypeCode.Empty:
    case TypeCode.Object:
    case TypeCode.DBNull:
    case TypeCode.String:
    case TypeCode.Char:
      ....
      break;
    case TypeCode.Boolean:
      ....
      break;
    case TypeCode.SByte:
    case TypeCode.Int16:
    case TypeCode.UInt16:
    case TypeCode.Int32:
    case TypeCode.UInt32:
    case TypeCode.Int64:
    case TypeCode.UInt64:
      ....
      break;
    case TypeCode.Single:
    case TypeCode.Double:
    case TypeCode.Decimal:
      ....
      break;
    case TypeCode.DateTime:
      ....
      break;
  }
}

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

public ActionResult Preview(string themeId, string returnUrl) {
  ....
  if (_extensionManager.AvailableExtensions()
    ....
  } else {
    var alreadyEnabledFeatures = GetEnabledFeatures();
    ....
    alreadyEnabledFeatures.Except(new[] { themeId }); // <=
    TempData[AlreadyEnabledFeatures] = alreadyEnabledFeatures;
  }
  ....
}

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:

alreadyEnabledFeatures = 
  alreadyEnabledFeatures.Except(new[] { themeId });

Substring method does not take a negative value

V3057 The ‘Substring’ function could receive the ‘-1’ value while non-negative value is expected. Inspect the second argument. ContentIdentity.cs 139

.... GetIdentityKeyValue(....) {
  ....
  var indexOfEquals = identityEntry.IndexOf("=");
  if (indexOfEquals < 0) return null;
  var key = identityEntry.Substring(1, indexOfEquals - 1);
  ....
}

Pay attention to the check of the indexOfEquals variable. The check protects from cases where the value of the variable is negative; but, there is no protection from a null value.

If the symbol ‘=’ is right at the beginning, then there will be an exception, because the second argument of the Substring() method will have a negative value -1.

The analyzer found other similar bugs, but I saw no point in describing each and every one of them.

  • V3057 The ‘Substring’ function could receive the ‘-1’ value while non-negative value is expected. Inspect the second argument. CommandParametersParser.cs 18
  • V3057 The ‘Substring’ function could receive the ‘-1’ value while non-negative value is expected. Inspect the second argument. CommandStep.cs 73

Operation precedence in a statement

V3022 Expression ‘localPart.Name + “.” + localField.Name + “.” + storageName’ is always not null. ContentFieldProperties.cs 56

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:

V3022 Expression ‘localPart.Name + “.” + localField.Name + “.” + storageName’ is always not null. ContentFieldsSortCriteria.cs 53

A check that is always false

V3022Expression ‘i == 4’ is always false. WebHost.cs 162

public void Clean() {
  // Try to delete temporary files for up to ~1.2 seconds.
  for (int i = 0; i < 4; i++) {
    Log("Waiting 300msec before trying to delete ....");
    Thread.Sleep(300);
    if (TryDeleteTempFiles(i == 4)) {
      Log("Successfully deleted all ....");
      break;
    }
  }
}

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.

  • V3022 Expression ‘result == null’ is always false. ContentFieldDriver.cs 175
  • V3022 Expression ‘String.IsNullOrWhiteSpace(url)’ is always true. GalleryController.cs 93
  • V3022 Expression ‘_smtpSettings.Host != null’ is always true. SmtpMessageChannel.cs 143
  • V3022 Expression ‘loop’ is always false. IndexingTaskExecutor.cs 270
  • V3022 Expression ‘Convert.ToString(Shape.Value)’ is always not null. EditorShapes.cs 372
  • V3022 Expression ‘ModelState.IsValid’ is always true. RecycleBinController.cs 81
  • V3022 Expression ‘previousXml != null’ is always true. ContentDefinitionEventHandler.cs 104
  • V3022 Expression ‘previousXml != null’ is always true. ContentDefinitionEventHandler.cs 134
  • V3022 Expression ‘layoutId != null’ is always true. ProjectionElementDriver.cs 120

Using a class member before verification of the object against null

V3027 The variable ‘x.Container’ was utilized in the logical expression before it was verified against null in the same logical expression. ContainerService.cs 130

query.Where(x => 
           (x.Container.Id == containerId || 
            x.Container == null) && 
            ids.Contains(x.Id));

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

void ISerializable.GetObjectData(
  SerializationInfo info, 
  StreamingContext context)
{
  info.AddValue("delegateType", Delegate.GetType());
  ....
  if (.... && Delegate != null)
  {
    ....
  }                
}

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.

P.S. For those who missed the news, let me remind you that PVS-Studio can now integrate with SonarQube. An article on this topic: «Control source code quality using the SonarQube platform»

Sofi Fateeva

Comments

    Flux 3,751

    Ropes — Fast Strings

    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.