# 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\. ![0456_OrchardCMS/image1.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image1.png) 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: 1. An open and free project that is built on community requests\. 1. A fast engine with a modular architecture, and all necessary CMS means\. 1. A public online gallery of modules, themes, and other components for extension from the community\. 1. High quality typography, attention to the layout and markup of pages\. 1. Focus on creating a comfortable and functional administration panel\. 1. 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/](https://archive.codeplex.com/?p=orchard)\. 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, [http://www\.orchardproject\.net/](https://www.orchardcore.net/) 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\. ![0456_OrchardCMS/image2.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image2.png) 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](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](https://pvs-studio.com/en/docs/warnings/v3110/) Possible infinite recursion inside 'ReturnTypeCustomAttributes' property\. ContentItemAlteration\.cs 121 ```cpp 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 _stack overflow_ 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: ```cpp 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 stack overflow exception\. V3110 Possible infinite recursion inside 'IsDefined' method\. ContentItemAlteration\.cs 101 ```cpp 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: ```cpp public override bool IsDefined(Type attributeType, bool inherit) { return _dynamicMethod.IsDefined(attributeType, inherit); } ``` ### When a minute doesn't always have 60 seconds ![0456_OrchardCMS/image3.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image3.png) [V3118](https://pvs-studio.com/en/docs/warnings/v3118/) Seconds componentof TimeSpan is used, which does not represent full time interval\. Possibly 'TotalSeconds' value was intended instead\. AssetUploader\.cs 182 ```cpp 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: ```cpp 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 ![0456_OrchardCMS/image4.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image4.png) [V3002](https://pvs-studio.com/en/docs/warnings/v3002/) The switch statement does not cover all values of the 'TypeCode' enum: Byte\. InfosetFieldIndexingHandler\.cs 65 ```cpp 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 ![0456_OrchardCMS/image5.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image5.png) [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Except' is required to be utilized\. AdminController\.cs 140 ```cpp 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: ```cpp alreadyEnabledFeatures = alreadyEnabledFeatures.Except(new[] { themeId }); ``` ### Substring method does not take a negative value [V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. ContentIdentity\.cs 139 ```cpp .... 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 ![0456_OrchardCMS/image6.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image6.png) [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'localPart\.Name \+ "\." \+ localField\.Name \+ "\." \+ storageName' is always not null\. ContentFieldProperties\.cs 56 ```cpp 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: ```cpp 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 [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'i \=\= 4' is always false\. WebHost\.cs 162 ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'Delegate' object was used before it was verified against null\. Check lines: 37, 40\. SerializableDelegate\.cs 37 ```cpp 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 ![0456_OrchardCMS/image7.png](https://import.viva64.com/docx/blog/0456_OrchardCMS/image7.png) 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](https://pvs-studio.com/en/) 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](https://pvs-studio.com/en/blog/posts/0452/)"