Geekly articles weekly

我们搜索并分析Orchard CMS代码中的错误

图片6

本文是使用PVS-Studio静态分析器重新检查Orchard项目的结果。 Orchard是一个开源内容管理系统，隶属于Outercurve Foundation（非营利性ASP.NET项目画廊）的一部分。 该测试很有趣，因为自首次分析以来，该项目和分析仪已显着增长。 新的积极因素和有趣的错误正等着您。

关于果园CMS

三年前，我们在PVS-Studio上测试了 Orchard。 从那时起，C＃分析器得到了长足的发展：我们改进了数据流分析，开发了过程间分析，添加了新的诊断方法并修复了一些误报。 此外，分析表明，上一篇文章中的所有评论均已得到纠正。 这意味着目标得以实现-代码变得更好。

我想相信开发人员将关注本文并进行必要的更改。 如果您介绍常规使用PVS-Studio的做法，那就更好了。 让我提醒您，对于开源项目，我们提供了许可证的免费版本。 顺便说一下，还有其他适合封闭项目的选项 。

可以像我一样从这里下载Orchard项目代码。 您可以在此处找到该项目的完整说明。 如果您还没有我们的PVS-Studio分析仪，则可以从此处下载试用版。 我使用的是PVS-Studio版本7.05 Beta。 我将分享她的工作成果。 我希望您同意使用PVS-Studio的有用性。 最主要的是定期使用分析仪。

验证结果

我将为您提供上一篇文章中的一些数据，以便您不必切换比较。

“所有扩展名为.cs的文件（3739个）都参与了最后一次检查。 它们总共包含214,564行代码。 根据审核结果，收到了137条警告。 在第一个（高）置信度下，收到39个警告。 在第二级（平均），收到了60条警告。”

目前，该项目有2767个.cs文件，也就是说，该项目已少于一千个文件。 通过减少文件数量和更改存储库名称来判断，已从项目中分配了内核（ 提交966 ）。 内核中有108,287行代码，分析器发出153条警告，其中33条为高级别，平均70条。 我们通常不考虑三级警告，我没有打破传统。

PVS-Studio警告 ： V3110 “ TryValidateModel”方法内可能存在无限递归。 PrefixedModuleUpdater.cs 48

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

与上一篇文章一样，我们使用无限递归打开错误列表。 在这种情况下，很难说开发商究竟想做什么。 但是我注意到TryValidateModel方法有一个参数重载，如下所示：

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

我认为，与重载方法一样，开发人员希望通过_updateModel调用方法。 编译器没有看到错误， _updateModel是IUpdateModel类型，并且当前类也实现了此接口。 由于该方法中没有单一条件可以防止StackOverflowException ，因此该方法甚至可能没有被调用过一次，但我不会指望它。 如果假设正确，则校正后的版本将如下所示：

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

PVS-Studio警告： V3008 “内容”变量已连续两次分配值。 也许这是一个错误。 检查行：197、190。DynamicCacheTagHelper.cs 197

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

分析器看到了两个对局部变量内容的分配。 GetChildContentAsync是库中的一种方法，它不足以让我们决定对其进行注释。 因此，不幸的是，我们和分析人员都对方法的返回对象和副作用一无所知。 但是我们可以肯定地说，在内容中分配结果是没有意义的。 也许这根本不是一个错误，只是一个额外的操作。 关于修复它的方式，我不能得出明确的结论。 我将把这个决定留给开发商。

PVS-Studio警告 ： V3080可能取消空引用。 考虑检查“ itemTag”。 CoreShapes.cs 92

 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; }

在此代码中，分析器检测到危险的取消引用itemTag 。 静态分析仪和测试人员之间差异的一个很好的例子。 该方法具有一个名为ItemTag的参数和一个名为itemTag的局部变量。 如您所知，差异对于编译器而言是巨大的！ 这是两个不同的尽管相关的变量。 连接通过第三个变量itemTagName。 引发异常的情况是这样的： ItemTag参数为“-”，未为itemTagName分配值，它将保留为空引用，如果为空引用，则本地itemTag也将变为空引用。 我认为最好在检查字符串后在此处引发异常。

 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; }

PVS-Studio警告： V3095在对null进行验证之前，已使用'remoteClient'对象。 检查行：49，51。ImportRemoteInstanceController.cs 49

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

分析器看到以下对remoteClient的取消引用和空检查。 实际上，这是一个潜在的NullReferenceException ，因为FirstOrDefault方法可以返回默认值（对于引用类型，它为null ）。 我想，要修复代码，只需转移上面的检查：

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

虽然，可能有必要用First代替FirstOrDefault并完全取消检查。

从PVS-Studio 7.05 Beta开始：

目前，我们已经注释了LINQ的所有orDefault方法。 此信息将由新的诊断程序使用，请注意在不检查的情况下取消调用这些方法的结果。 对于每个orDefault方法，如果找不到合适的元素，则有一个引发异常的类比。 与抽象的NullReferenceException相比，此异常将更有助于理解问题。

我不能不分享该项目开发的诊断结果。 27个潜在危险的地方。 以下是其中一些：

ContentTypesAdminNodeNavigationBuilder.cs 71：

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

ListPartDisplayDriver.cs 217：

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

ContentTypesAdminNodeNavigationBuilder.cs 113：

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

PVS-Studio警告 ： V3080方法返回值可能会空引用。 考虑检查：CreateScope（）。 SetupService.cs 136

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

因此，分析器注意到调用CreateScope方法的结果已取消引用。 CreateScope方法很小，我将全部介绍它：

 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; }

如您所见，它在两种情况下可以返回null 。 分析器无法说出程序将经过哪个分支，因此将代码标记为可疑。 在我的代码中，我将立即添加一个空检查。

也许我认为有偏见，但是所有异步方法都应尽可能地针对NullReferenceException进行保险。 调试这些东西是非常可疑的乐趣。

在这种情况下， CreateScope方法有四个调用，在两个调用中有一个检查，但在另外两个中则丢失了。 此外，没有验证的一对类似于复制粘贴（一个类，一个方法，已取消引用以调用UsingAsync）。 我已经从这对发出了第一个电话，当然，第二个电话也有类似的分析器警告：

V3080方法返回值的空null取消引用。 考虑检查：CreateScope（）。 SetupService.cs 192

PVS-Studio警告： V3127找到两个相似的代码片段。 也许，这是一个错字，应该使用“ AccessTokenSecret”变量代替“ ConsumerSecret” TwitterClientMessageHandler.cs 52

 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); .... }

经典复制粘贴。 ConsumerSecret进行了两次检查，而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); .... }

PVS-Studio警告： V3139两个或更多案例分支执行相同的操作。 SerialDocumentExecuter.cs 23

另一个复制粘贴错误。 为了更清楚一点，我将全班讨论，因为它很小。

 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 ....; } } }

分析人员认为两个相同的案例分支可疑。 确实，该班级中有3个实体，其中2个在走动时返回，一个没有。 如果计划这样做并且放弃了第三个选项，则可以通过将两个分支组合如下来将其删除：

 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 ....; }

PVS-Studio警告 ： V3080可能取消空引用。 考虑检查“请求”。 GraphQLMiddleware.cs 148

 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; .... }

在第一个if块中， request变量在几个地方（但在有嵌套条件的地方）获得的值不是null 。 我没有列举所有这些条件，因为该示例太麻烦了。 检查IsGet或IsPost方法的http类型的第一个条件就 足够了 。 Microsoft.AspNetCore.Http.HttpMethods类具有九种用于检查请求类型的静态方法。 这意味着，例如，如果Delete或Set请求进入ExecuteAsync方法，则将引发NullReferenceException 。 即使目前根本不支持这种方法，最好还是进行例外检查。 毕竟，对系统的要求可能会发生变化。 以下验证示例：

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

PVS-Studio警告 ： V3080方法返回值可能会空引用。 考虑检查：获取<ContentPart>（...）。 ContentPartHandlerCoordinator.cs 190

在开发环境中，大多数V3080诊断触发器都很容易看到。 需要方法导航，突出显示类型，令人鼓舞的IDE氛围。 我尝试使示例尽可能简短，以使您更轻松地阅读。 但是，如果这对我来说不可行，或者您想检查自己的编程水平并自己弄清楚，我建议您看看此诊断如何在任何打开的项目或您自己的项目中起作用。

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

分析仪在这条线上发誓。 让我们看一下Get方法：

 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; } .... }

事实证明，如果在通过名称索引器从Data接收到元素时，我们得到的类型与JObject不兼容的实体，则Get方法将返回null 。 我不会冒险判断这种可能性，因为这些类型来自Newtonsoft.Json库，并且我对此有一点经验。 但是开发人员怀疑所需的项目可能不是。 因此，参考阅读结果时不要忘记这种可能性。 如果我们认为该节点应为第一个Get，则会在第一个Get上添加异常引发，或者在取消引用之前进行检查，如果不存在该节点不会改变常规逻辑（例如，采用默认值）。

选项一：

 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()); .... }

PVS-Studio警告 ： V3080可能取消空引用。 考虑检查“结果”。 ContentQueryOrchardRazorHelperExtensions.cs 19

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

一个相对于前一个例子的相当简单的例子。 分析器认为调用QueryAsync的结果可能是空引用。 方法如下：

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

这里GetQueryAsync是一个接口方法，您不能确定每个实现。 而且，在同一项目中有这样的选择：

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

由于对外部函数和缓存访问的调用很多，因此对GetDocumentAsync方法的分析变得很复杂。 让我们详细讨论一下值得添加一张支票的事实。 而且，该方法是异步的。

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

图片14


结论

我不禁注意到代码的高质量！ 是的，在本文中我没有提到其他缺点，但是仍然考虑了最严重的缺点。 当然，这并不意味着每三年检查一次就足够了。 定期使用静态分析仪可获得最大的好处，因为这种方法使您可以在最小的编辑成本和复杂性的情况下尽早发现并修复问题。

尽管一次性检查效果不佳，但我还是建议您下载并尝试在项目中使用PVS-Studio-如果您能找到有趣的东西怎么办？



如果您想与讲英语的读者分享本文，请使用翻译链接：Alexander Senichkin。 扫描Orchard CMS的代码中的错误 。

Source: https://habr.com/ru/post/zh-CN472362/

More articles:


All Articles