Geekly articles weekly

扫描Orchard CMS的代码以查找错误

图片6

本文回顾了使用PVS-Studio静态分析器对Orchard项目进行第二次检查的结果。 Orchard是一个开源内容管理器系统，作为非营利组织Outercurve Foundation的ASP.NET Open Source Gallery的一部分提供。 今天的检查特别有趣，因为自第一次检查以来，项目和分析器都已经走了很长一段路，这次我们将查看新的诊断消息和一些不错的错误。

关于果园CMS

我们三年前检查过果园。 从那时起，PVS-Studio的C＃分析器得到了长足的发展：我们改进了数据流分析，增加了过程间分析和新的诊断方法，并修复了一些误报。 不仅如此，第二次检查还显示Orchard的开发人员已修复了第一篇文章中报告的所有错误，这意味着我们已经实现了我们的目标，即让他们改进了他们的代码。

我希望他们也注意本文，并进行必要的修复，或者更好的是，定期采用PVS-Studio。 提醒一下，我们为开源开发人员提供了免费许可证。 顺便说一下，专有项目还有其他选择 。

Orchard的源代码可在此处下载。 完整的项目描述可在此处找到。 如果您还没有PVS-Studio副本，则可以下载试用版。 我使用了PVS-Studio 7.05 Beta，并将在本文中包含一些警告。 我希望这篇评论能使您相信PVS-Studio是一个有用的工具。 请记住，它应该定期使用。

分析结果

以下是从Orchard的第一次检查中获得的一些数据，因此您不必在两篇文章之间进行切换即可进行比较。

在上一次检查中，“我们对所有扩展名为.cs的源代码文件（3739个项目）进行了分析。 总共有214,564行代码。 检查的结果为137警告。 更准确地说，有39个第一（高级）警告。 也有60秒（中）级别的警告。”

当前版本的Orchard由2767个.cs文件组成，也就是说，该文件要小1000个左右。 存储库的缩小和重命名表明开发人员已经隔离了项目的核心（ 提交966 ），该核心的长度为108287 LOC。 分析仪发出了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 (....) { .... } .... }

或者，也许应该通过将FirstOrDefault替换为First并完全取消检查来解决此问题。

PVS-Studio 7.05 Beta发出的警告：

到目前为止，我们已经注释了所有LINQ的orDefault方法。 此信息将由我们正在使用的新诊断程序使用：它检测在没有事先检查的情况下取消引用了这些方法返回的值的情况。 每个orDefault方法都有一个对应的对象，如果找不到匹配的元素，则该对象将引发异常。 与抽象的NullReferenceException相比，此异常将更有助于跟踪问题。

我不能不分享在Orchard项目中从该诊断程序获得的结果。 有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 ....; } } }

分析仪不喜欢两个相同的案例分支。 实际上，该类具有三个实体，而switch语句仅返回其中两个。 如果此行为是有意的，并且实际上并不打算使用第三个实体，则可以通过删除合并了两个实体的第三个分支来改进代码，如下所示：

 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块中多次为请求变量分配了一个不同于null的值，但每次都有嵌套条件。 包括所有这些条件会使示例变得太长，因此我们仅介绍前几个条件，它们将检查http方法IsGet或IsPost的类型。 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中引发异常，或者如果该节点的不存在不会改变整体逻辑，则在取消引用之前添加一个检查（例如，默认值）。

解决方案1：

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

解决方案2：

 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


结论

我不得不提到Orchard的高质量代码！ 确实，还有一些其他缺陷，我在这里没有讨论，但是我向您展示了所有最严重的错误。 当然，这并不是说三年检查一次源代码就足够了。 如果定期使用静态分析，您将获得最大的收益，因为这样可以保证在最早的开发阶段就捕获并修复错误，在这种情况下，错误修复最便宜，最容易。

即使一次检查并没有多大帮助，我仍然鼓励您下载PVS-Studio并在您的项目上进行尝试：谁知道，也许您还会发现一些有趣的错误。

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

More articles:


All Articles