We search and analyze errors in the Orchard CMS code.
Orchard is a free, open source content management system that is part of the open source ASP.NET project gallery of the non-profit Outercurve Foundation.
For us, the developers of the PVS-Studio static analyzer, this is another opportunity to test an interesting project, tell people (and developers including) about errors found and, in turn, test our analyzer again. Today we will talk about errors found in the project Orchard CMS.
The project description is taken from the site .
The Orchard CMS project was announced in March 2010 with the release of the first beta version of the project. The creators of Orchard CMS set a goal to build a content management system on a new successful ASP.NET MVC framework that would meet the following requirements:
')
Initially, Orchard and its source codes were licensed on the basis of a free license MS-PL, but, with the release of the first public version, the project changed the license to a simpler and more common New BSD License.
Four preliminary versions were released during the year, until Orchard CMS reached version 1.0. All this time, the developers kept in touch with the community, accepting suggestions, taking into account comments and correcting errors found. To publish the source code and collect user feedback, the project was launched on the open source project codeplex.com at orchard.codeplex.com .
Today you can find extensive documentation on all aspects of the use of Orchard CMS, you can participate in the discussion of the project on the forums, you can send a report about the error found on the bugtracker, you can download the latest project source codes and binary builds.
In addition to the developer page, the official project website was launched at http://www.orchardproject.net/ , which today contains all the supporting documentation necessary for working with Orchard CMS. In addition, the official website contains a gallery of modules and other components created by the community to expand the functionality of Orchard CMS.
All source files (3,739 pieces) with the .cs extension participated in the check. In total, they contained 214,564 lines of code.
Following the audit, 137 warnings were received. If we look in more detail, then at the first (high) level, 39 warnings were received. 28 of them clearly indicated problem areas or errors. At the second level (average), 60 warnings were received. In my subjective opinion, 31 warnings correctly pointed out the places with errors or typographical errors. We will not consider the third (low) level of warnings, as these are warnings with a low level of confidence and, as a rule, among them there are a lot of false positives or there are warnings that are not relevant for many types of projects.
So, we proceed to the consideration of the most interesting of the errors found. In more detail, the authors of the project will be able to study the errors by requesting a temporary license from us and performing the verification themselves.
As I understand it, the Orchard CMS developers are already using ReSharper in their work on the project. I draw this conclusion based on:
» Https://github.com/OrchardCMS/Orchard-Harvest-Website/blob/master/src/Orchard.4.5.resharper
We do not like the idea of comparing ReSharper and PVS-Studio, since these are tools of different types. But, as you can see, using ReSharper does not prevent us from finding real errors in the code.
V3110 Possible infinite recursion inside 'ReturnTypeCustomAttributes' property. ContentItemAlteration.cs 121
Opens a list of errors we have a fairly common typo, which, when getting the value of the property ReturnTypeCustomAttributes, will lead to an infinite recursion and, as a result, to throw out a StackOverflow exception, which in turn will lead to a program crash. Most likely, the programmer wanted to return the _dynamicMethod object from the property of the same field, then the correct version would look like this:
And here is another typo, which will lead to an infinite recursion, and, as a consequence, the release of the StackOverflow exception.
V3110 Possible infinite recursion inside 'IsDefined' method. ContentItemAlteration.cs 101
In this case, the IsDefined method calls itself. I think that here the programmer also wanted to call the same-named method on the _dynamicMethod object. Then the correct version would look like this:
V3118 Seconds component of TimeSpan is used, which does not represent the full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182
Another fairly common typo, arising from the not quite obvious implementation of the TimeSpan type. From the commentary it is clear that this method should block the database request if less than 5 seconds have passed since the previous request. But the programmer, apparently, did not know that the Seconds property of an object of type TimeSpan does not return the total number of seconds in a given time interval, but the residual number of seconds.
For example, if we have a time interval of 1 minute and 4 seconds long, the call to the Seconds method will return us only 4 seconds. If you need to return the total number of seconds, you must use the TotalSeconds property. For this example, it will be 64 seconds.
The correct version would look like this:
V3002 Enum : Byte. InfosetFieldIndexingHandler.cs 65
This piece of code is quite cumbersome, so it is difficult to immediately notice the error. In this case, there is an enumeration TypeCode and a method InfosetFieldIndexingHandler, in which it is checked to which of the elements of the enumeration the typeCode variable belongs . In this case, the programmer who wrote this code most likely forgot about one small element called Byte , but added it to his fellow 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 in which an exception about an unprocessed enumeration element is thrown.
V3010 The return value of the 'Except' is required to be utilized. AdminController.cs 140
As you know, the Except method excludes from the collection for which it was called, elements of another collection. But the programmer, apparently, did not know, or did not pay attention to the fact that the result of the work of this method returns a new collection. The correct version would look like this:
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. ContentIdentity.cs 139
Notice the check for the indexOfEquals variable. The check protects against the case if the value of the variable is negative. But there is no protection against zero.
If the '=' character is at the very beginning, an exception will be thrown, since the second argument of the Substring () method will have a negative value of -1.
Similar errors were also found, which separately does not make sense to consider separately:
V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName 'is always not null. ContentFieldProperties.cs 56
In this case, the programmer most likely thought that the operator ?? checks only the storageName variable for null , and if so, then an empty string will be added to the expression instead. But since the + operator has a higher priority, the whole expression will be checked for null . It is also worth noting that if we add null to the line, we will get the same line without changes. Thus, in this case, you can safely discard this test as unnecessary and misleading. The corrected version could look like this:
The analyzer also found another similar error:
V3022 Expression 'i == 4' is always false. WebHost.cs 162
It can be seen that the loop iterator i will always have a value in the range from 0 to 3, but despite this, the programmer inside the loop body boldly checks whether the iterator has a value of 4. This test will never be performed, but without knowing the true purpose of this segment code, it's hard for me to say how dangerous this error is in the scope of the project.
The analyzer has also found a number of similar errors. They all say that the test will always be either true or false.
V3027 The variable "x.Container" was used in the same logical expression. ContainerService.cs 130
As can be seen from the code above (we are interested in 2 and 3 lines), the programmer first checks the reference to the Id property of the container, and only then checks the container for null . The correct option would be to first check for null , and only then access the elements of the container.
The following case is similar to the previous one. Try to find the error here yourself.
V3095 The 'Delegate' object was used against it. Check lines: 37, 40. SerializableDelegate.cs 37
The analyzer found 2 more similar errors that are similar to the one described above, so I will not cite them.
In this project, there were other errors, typos, shortcomings. But they did not seem interesting to me to describe them in the article. Orchard CMS developers can easily find all the shortcomings using the PVS-Studio tool. You can also look for errors in your projects using the static analyzer proposed above.
I want to remind you that the main advantage of static analysis is its regular use. Download and check one-time code - this is not serious. For example, programmers watch the compiler warnings regularly, rather than turning them on every 3 years before one of the releases. With regular use, a static analyzer will save a lot of time searching for typos and errors.
PS For those who missed the news, I want to remind you that PVS-Studio is now able to integrate with SonarQube . An article on this topic: "We control the quality of the code using the platform SonarQube ".
If you want to share this article with an English-speaking audience, then please use the link to the translation: Ivan Kishchenko. Analysis of bugs in Orchard CMS .
For us, the developers of the PVS-Studio static analyzer, this is another opportunity to test an interesting project, tell people (and developers including) about errors found and, in turn, test our analyzer again. Today we will talk about errors found in the project Orchard CMS.
About Orchard CMS
The project description is taken from the site .
The Orchard CMS project was announced in March 2010 with the release of the first beta version of the project. The creators of Orchard CMS set a goal to build a content management system on a new successful ASP.NET MVC framework that would meet the following requirements:
')
- Open free and free project, depending on community requests;
- Fast engine with a modular architecture and all the necessary means of CMS;
- A publicly accessible online gallery of modules, themes, and other extension components from the community;
- High quality typography, attention to layout and page layout;
- Emphasis on creating a convenient and functional administration panel;
- Rapid deployment of the system on the workplace and easy publishing to the server.
Initially, Orchard and its source codes were licensed on the basis of a free license MS-PL, but, with the release of the first public version, the project changed the license to a simpler and more common New BSD License.
Four preliminary versions were released during the year, until Orchard CMS reached version 1.0. All this time, the developers kept in touch with the community, accepting suggestions, taking into account comments and correcting errors found. To publish the source code and collect user feedback, the project was launched on the open source project codeplex.com at orchard.codeplex.com .
Today you can find extensive documentation on all aspects of the use of Orchard CMS, you can participate in the discussion of the project on the forums, you can send a report about the error found on the bugtracker, you can download the latest project source codes and binary builds.
In addition to the developer page, the official project website was launched at http://www.orchardproject.net/ , which today contains all the supporting documentation necessary for working with Orchard CMS. In addition, the official website contains a gallery of modules and other components created by the community to expand the functionality of Orchard CMS.
Test results
All source files (3,739 pieces) with the .cs extension participated in the check. In total, they contained 214,564 lines of code.
Following the audit, 137 warnings were received. If we look in more detail, then at the first (high) level, 39 warnings were received. 28 of them clearly indicated problem areas or errors. At the second level (average), 60 warnings were received. In my subjective opinion, 31 warnings correctly pointed out the places with errors or typographical errors. We will not consider the third (low) level of warnings, as these are warnings with a low level of confidence and, as a rule, among them there are a lot of false positives or there are warnings that are not relevant for many types of projects.
So, we proceed to the consideration of the most interesting of the errors found. In more detail, the authors of the project will be able to study the errors by requesting a temporary license from us and performing the verification themselves.
As I understand it, the Orchard CMS developers are already using ReSharper in their work on the project. I draw this conclusion based on:
» Https://github.com/OrchardCMS/Orchard-Harvest-Website/blob/master/src/Orchard.4.5.resharper
We do not like the idea of comparing ReSharper and PVS-Studio, since these are tools of different types. But, as you can see, using ReSharper does 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; } } Opens a list of errors we have a fairly common typo, which, when getting the value of the property ReturnTypeCustomAttributes, will lead to an infinite recursion and, as a result, to throw out a StackOverflow exception, which in turn will lead to a program crash. Most likely, the programmer wanted to return the _dynamicMethod object from the property of the same field, then the correct version would look like this:
public override ICustomAttributeProvider ReturnTypeCustomAttributes { get { return _dynamicMethod.ReturnTypeCustomAttributes; } } And here is another typo, which will lead to an infinite recursion, and, as a consequence, the release of the 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 that here the programmer also wanted to call the same-named method on the _dynamicMethod object. Then the correct version would look like this:
public override bool IsDefined(Type attributeType, bool inherit) { return _dynamicMethod.IsDefined(attributeType, inherit); } When in the minute is not always 60 seconds
V3118 Seconds component of TimeSpan is used, which does not represent the 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, arising from the not quite obvious implementation of the TimeSpan type. From the commentary it is clear that this method should block the database request if less than 5 seconds have passed since the previous request. But the programmer, apparently, did not know that the Seconds property of an object of type TimeSpan does not return the total number of seconds in a given time interval, but the residual number of seconds.
For example, if we have a time interval of 1 minute and 4 seconds long, the call to the Seconds method will return us only 4 seconds. If you need to return the total number of seconds, you must use the TotalSeconds property. For this example, it will be 64 seconds.
The correct version would look like this:
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 when checking via switch
V3002 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 piece of code is quite cumbersome, so it is difficult to immediately notice the error. In this case, there is an enumeration TypeCode and a method InfosetFieldIndexingHandler, in which it is checked to which of the elements of the enumeration the typeCode variable belongs . In this case, the programmer who wrote this code most likely forgot about one small element called Byte , but added it to his fellow 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 in which an exception about an unprocessed enumeration element is thrown.
No return value processing from Except method
V3010 The return value of the '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 you know, the Except method excludes from the collection for which it was called, elements of another collection. But the programmer, apparently, did not know, or did not pay attention to the fact that the result of the work of this method returns a new collection. The correct version would look like this:
alreadyEnabledFeatures = alreadyEnabledFeatures.Except(new[] { themeId }); The substring method does not accept 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); .... } Notice the check for the indexOfEquals variable. The check protects against the case if the value of the variable is negative. But there is no protection against zero.
If the '=' character is at the very beginning, an exception will be thrown, since the second argument of the Substring () method will have a negative value of -1.
Similar errors were also found, which separately does not make sense to consider separately:
- 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
The priority of operations in the expression
V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName 'is always not null. ContentFieldProperties.cs 56
localPart.Name + "." + localField.Name + "." + storageName ?? "" In this case, the programmer most likely thought that the operator ?? checks only the storageName variable for null , and if so, then an empty string will be added to the expression instead. But since the + operator has a higher priority, the whole expression will be checked for null . It is also worth noting that if we add null to the line, we will get the same line without changes. Thus, in this case, you can safely discard this test as unnecessary and misleading. The corrected version could look like this:
localPart.Name + "." + localField.Name + "." + storageName The analyzer also found another similar error:
V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName' is always not null. ContentFieldsSortCriteria.cs 53 Verification is always false
V3022 Expression '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; } } } It can be seen that the loop iterator i will always have a value in the range from 0 to 3, but despite this, the programmer inside the loop body boldly checks whether the iterator has a value of 4. This test will never be performed, but without knowing the true purpose of this segment code, it's hard for me to say how dangerous this error is in the scope of the project.
The analyzer has also found a number of similar errors. They all say that the test will always 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 checking an object for null
V3027 The variable "x.Container" was used in the same logical expression. ContainerService.cs 130
query.Where(x => (x.Container.Id == containerId || x.Container == null) && ids.Contains(x.Id)); As can be seen from the code above (we are interested in 2 and 3 lines), the programmer first checks the reference to the Id property of the container, and only then checks the container for null . The correct option would be to first check for null , and only then access the elements of the container.
The following case is similar to the previous one. Try to find the error here yourself.
V3095 The 'Delegate' object was used against it. Check lines: 37, 40. SerializableDelegate.cs 37
void ISerializable.GetObjectData( SerializationInfo info, StreamingContext context) { info.AddValue("delegateType", Delegate.GetType()); .... if (.... && Delegate != null) { .... } } The analyzer found 2 more similar errors that are similar to the one described above, so I will not cite them.
- V3095 The 'Delegate' object was used against it. Check lines: 37, 40. SerializableDelegate.cs 37
- V3095 The 'widget' object was used against it. Check lines: 81, 93. TagsWidgetCommands.cs 81
Conclusion
In this project, there were other errors, typos, shortcomings. But they did not seem interesting to me to describe them in the article. Orchard CMS developers can easily find all the shortcomings using the PVS-Studio tool. You can also look for errors in your projects using the static analyzer proposed above.
I want to remind you that the main advantage of static analysis is its regular use. Download and check one-time code - this is not serious. For example, programmers watch the compiler warnings regularly, rather than turning them on every 3 years before one of the releases. With regular use, a static analyzer will save a lot of time searching for typos and errors.
PS For those who missed the news, I want to remind you that PVS-Studio is now able to integrate with SonarQube . An article on this topic: "We control the quality of the code using the platform SonarQube ".
If you want to share this article with an English-speaking audience, then please use the link to the translation: Ivan Kishchenko. Analysis of bugs in Orchard CMS .
Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.
Source: https://habr.com/ru/post/316418/
All Articles