public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }

public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }

public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }

public override async Task ProcessAsync(....) { .... IHtmlContent content; .... try { content = await output.GetChildContentAsync(); } finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); .... }

public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....; .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }

public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = ....; if(String.IsNullOrEmpty(itemTag)) throw .... .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }

public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey)); if (remoteClient == null || ....) { .... } .... }

public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }

var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault(); await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders);

var contentTypePartDefinition = ....Parts.FirstOrDefault(....); return contentTypePartDefinition.Settings....;

var typeEntry = node.ContentTypes.Where(....).FirstOrDefault(); return AddPrefixToClasses(typeEntry.IconClass);

public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... }

public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this); // A new scope can be only used on a non released shell. if (!released) { return scope; } scope.Dispose(); return null; }

public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }

public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }

public class SerialDocumentExecuter : DocumentExecuter { private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy(); private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy(); protected override IExecutionStrategy SelectExecutionStrategy(....) { switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } } }

switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }

switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }

switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }

private async Task ExecuteAsync(HttpContext context....) { .... GraphQLRequest request = null; .... if (HttpMethods.IsPost(context.Request.Method)) { .... } else if (HttpMethods.IsGet(context.Request.Method)) { .... request = new GraphQLRequest(); .... } var queryToExecute = request.Query; .... }

private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }

public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }

public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }

public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }

public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }

public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... }

public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }

public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... }

This article reviews the results of a second check of the Orchard project with the PVS-Studio static analyzer. Orchard is an open-source content manager system delivered as part of the ASP.NET Open Source Gallery under the non-profit Outercurve Foundation. Today's check is especially interesting because both the project and the analyzer have come a long way since the first check, and this time we'll be looking at new diagnostic messages and some nice bugs.We checked Orchard three years ago. PVS-Studio's C# analyzer has greatly evolved since then: we have improved the data flow analysis, added interprocedural analysis and new diagnostics, and fixed a number of false positives. More than that, the second check revealed that the developers of Orchard had fixed all the bugs reported in the first article, which means we had achieved our goal, i.e. had helped them make their code better.I hope they will pay attention to this article as well and make the necessary fixes or, better still, adopt PVS-Studio for use on a regular basis. As a reminder, we provide open-source developers with a free license. By the way, there are other options that proprietary projects may enjoy as well.Orchard's source code is available for download here . The complete project description is found here . If you don't have a PVS-Studio copy yet, you can download the trial version. I used PVS-Studio 7.05 Beta and will include some of its warnings in this article. I hope this review will convince you that PVS-Studio is a useful tool. Just keep in mind that it's meant to be used regularly Here are some of the figures from the first check of Orchard so that you don't have to switch between the two articles for comparison.During the previous check, «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. There were also 60 second (medium) level warnings.»The current version of Orchard is made up of 2,767 .cs files, i.e. it's about one thousand files smaller. The downsizing and renaming of the repository suggests that the developers have isolated the project's core ( commit 966 ), which is 108,287 LOC long. The analyzer issued 153 warnings: 33 first-level and 70 second-level ones. We don't typically include third-level warnings, and I'm going to stick to the tradition. V3110 Possible infinite recursion inside 'TryValidateModel' method. PrefixedModuleUpdater.cs 48Let's start off with an infinite recursion bug, as we did in the first article. This time the exact intentions of the developer aren't clear, but I noticed that themethod had an overloaded version with one parameter:I think that, just like in the case of the overloaded version, the developer intended to call the method throughThe compiler didn't notice the mistake;is of type, and the current class also implements this interface. Since the method doesn't include any check against,it was probably never called, though I wouldn't count on that. If my assumption is correct, the fixed version should look like this: V3008 The 'content' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 197, 190. DynamicCacheTagHelper.cs 197The analyzer detected two assignments to the local variableis a library method that is used too rarely for us to take the trouble to examine and annotate it. So, I'm afraid, neither we, nor the analyzer know anything about the method's return object and side effects. But we know for sure that assigning the return value tomakes no sense if it's not used further in the code. Perhaps it's just a redundant operation rather than a mistake. I can't say how exactly this should be fixed, so I leave it to the developers. V3080 Possible null dereference. Consider inspecting 'itemTag'. CoreShapes.cs 92The analyzer detected an unsafe dereference of. This snippet is a good example of how a static analysis tool is different from a human developer doing code review. The method has a parameter namedand a local variable named. No need to tell you it makes a huge difference to the compiler! These are two different, even though related, variables. The way they are related is through a third variable,Here's the sequence of steps leading to the possible exception: if theargument is equal to "-", no value will be assigned to, so it will remain a null reference, and if it's a null reference, then the local variablewill turn into a null reference too. In my opinion, it's better to have an exception thrown following the string check. V3095 The 'remoteClient' object was used before it was verified against null. Check lines: 49, 51. ImportRemoteInstanceController.cs 49The analyzer detected a dereference offollowed by a null check a couple of lines later. This is indeed a potentialas themethod may return a default value (which isfor reference types). I guess this snippet can be fixed by simply moving the check up so that is precedes the dereference operation:Or perhaps it should be fixed by replacingwithand removing the check altogether.By now, we have annotated all of'smethods. This information will be used by the new diagnostic we are working on: it detects cases where the values returned by these methods are dereferenced without a prior check. Eachmethod has a counterpart that throws an exception if no matching element has been found. This exception will be more helpful in tracking down the problem than the abstractI can't but share the results I've got from this diagnostic on the Orchard project. There are 27 potentially dangerous spots. Here are some of them:ContentTypesAdminNodeNavigationBuilder.cs 71:ListPartDisplayDriver.cs 217:ContentTypesAdminNodeNavigationBuilder.cs 113: V3080 Possible null dereference of method return value. Consider inspecting: CreateScope(). SetupService.cs 136The analyzer mentioned a dereference of the value returned by themethod.is a tiny method, so here's its complete implementation:As you can see, there are two cases where it can return. The analyzer doesn't know which branch the flow of execution will follow, so it plays safe and reports the code as suspicious. If I were to write code like that, I'd write a null check right away.Perhaps my opinion is biased, but I believe that every asynchronous method should be protected fromas much as possible because debugging stuff like that is far from enjoyable.In this particular case, themethod is called four times: two of those calls are accompanied by checks and the other two are not. The latter two calls (without checks) seem to be copy-paste clones (same class, same method, same way of dereferencing the result to call UsingAsync). The first of those two calls was shown above, and you may be sure the second one triggered the same warning: V3080 Possible null dereference of method return value. Consider inspecting: CreateScope(). SetupService.cs 192 V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler.cs 52That's a classic copy-paste mistake.was checked twice, whilewas not checked at all. Obviously, this is fixed as follows: V3139 Two or more case-branches perform the same actions. SerialDocumentExecuter.cs 23Another copy-paste bug. For clarity, here's the complete class implementation (it's small).The analyzer didn't like the two identicalbranches. Indeed, the class has three entities, while the switch statement returns only two of them. If this behavior is intended and the third entity isn't actually meant to be used, the code can be improved by removing the third branch having merged the two of them as follows:If this is a copy-paste bug, the first of the duplicate return fields should be fixed as follows:Or it should be the second case branch. I don't know the project's details and therefore can't determine the correlation between the names of the operation types and strategies. V3080 Possible null dereference. Consider inspecting 'request'. GraphQLMiddleware.cs 148Thevariable is assigned a value different fromseveral times in the firstblock, but each time with nested conditions. Including all those conditions would make the example too long, so we'll just go with the first few ones, which check the type of the http methodor. Theclass has nine static methods for checking the query type. Therefore, passing, for example, aorquery to themethod would lead to raising a. Even if such methods are currently not supported at all, it would still be wise to add an exception-throwing check. After all, the system requirements may change. Here's an example of such a check: V3080 Possible null dereference of method return value. Consider inspecting: Get (...). ContentPartHandlerCoordinator.cs 190Most of the V3080 warnings are more convenient to view within the development environment because you need the method navigation, type highlighting, and the friendly atmosphere of the IDE. I'm trying to reduce the text of examples as much as possible to keep them readable. But if I'm not doing it right or if you want to test your programming skill and figure it all out for yourself, I recommend checking out this diagnostic's result on any open-source project or just your own code.The analyzer reports this line. Let's take a look at themethod:It calls its overloaded version. Let's check it too:It turns out that if we get an entity of a type incompatible withfromusing theindexer, themethod will return. I don't know for sure how likely that is because these types are from thelibrary, which I haven't worked much with. But the author of the code was suspecting that the sought element might not exist, so we should keep that in mind when accessing the result of the read operation as well. Personally, I'd have an exception thrown in the very firstif we believe the node must be present, or add a check before the dereference if the node's non-existence doesn't change the overall logic (for example, we get a default value).Solution 1:Solution 2: V3080 Possible null dereference. Consider inspecting 'results'. ContentQueryOrchardRazorHelperExtensions.cs 19This is quite a simple example in comparison with the previous one. The analyzer suspects that themethod might return a null reference. Here's the method's implementation:Sinceis an interface method, you can't be sure about each implementation, especially if we consider that the project also includes the following version:The multiple calls to external functions and cache accesses make the analysis ofdifficult, so let's just say that the check is needed — all the more so because the method is an asynchronous one.

I can't but mention the high quality of Orchard's code! True, there were some other defects, which I didn't discuss here, but I showed you all of the most severe bugs. Of course, this is not to say that checking your source code once in three years is enough. You'll get the most out of static analysis if you use it regularly because this is the way you are guaranteed to catch and fix bugs at the earliest development stages, where bug fixing is cheapest and easiest.Even though one-time checks don't help much, I still encourage you to download PVS-Studio and try it on your project: who knows, maybe you'll find some interesting bugs too.