نحن نبحث ونحلل الأخطاء في كود Orchard CMS

هذه المقالة هي نتيجة إعادة فحص مشروع Orchard باستخدام محلل ثابت PVS-Studio. Orchard هو نظام إدارة محتوى مفتوح المصدر يمثل جزءًا من Outercurve Foundation ، وهو معرض ASP.NET غير ربحي للمشاريع. الاختبار مثير للاهتمام في أن المشروع والمحلل قد نما بشكل ملحوظ منذ التحليل الأول. إيجابيات جديدة والبق مثيرة للاهتمام في انتظاركم.

حول بستان سم

اختبرنا Orchard مع PVS-Studio منذ ثلاث سنوات. منذ ذلك الحين ، تطور محلل C # بشكل كبير: قمنا بتحسين تحليل تدفق البيانات ، وقمنا بتحليل interprocedural ، وأضفنا تشخيصات جديدة وأصلنا عددًا من الإيجابيات الخاطئة. بالإضافة إلى ذلك ، أظهر التحليل أنه تم تصحيح جميع التعليقات الواردة في المقال السابق. هذا يعني أن الهدف قد تحقق - أصبح الكود أفضل.

أريد أن أصدق أن المطورين سوف ينتبهون إلى هذه المقالة وإجراء التغييرات اللازمة. أفضل من ذلك ، إذا قمت بتقديم ممارسة الاستخدام المنتظم لـ PVS-Studio. اسمحوا لي أن أذكركم بأننا نقدم لمشاريع مفتوحة المصدر نسخة مجانية من الترخيص. بالمناسبة ، هناك خيارات أخرى مناسبة للمشاريع المغلقة.

يمكن تنزيل رمز مشروع Orchard من هنا ، كما فعلت. يمكن العثور على وصف كامل للمشروع هنا . إذا لم يكن لديك محلل PVS-Studio الخاص بنا حتى الآن ، يمكنك تنزيل إصدار تجريبي من هنا . لقد استخدمت إصدار PVS-Studio الإصدار 7.05 Beta. سوف أشارك نتائج عملها. آمل أن تتفق مع فائدة استخدام PVS-Studio. الشيء الرئيسي هو استخدام محلل بانتظام .

نتائج التحقق من الصحة

سأعطيك بعض الأرقام من المقال الأخير حتى لا تضطر إلى التبديل للمقارنة.

"شاركت جميع الملفات (3739 قطعة) ذات الملحق .cs في الاختبار الأخير. في المجموع ، أنها تحتوي على 214.564 سطر من التعليمات البرمجية. بناءً على نتائج التدقيق ، تم استلام 137 تحذيرًا. في مستوى الثقة (الأول) الأول ، تم تلقي 39 تحذيرًا. في المستوى الثاني (المتوسط) ، تم تلقي 60 تحذيرًا. "

في الوقت الحالي ، يحتوي المشروع على 2767 ملف. cs ، أي أن المشروع أصبح أقل من ألف ملف. استنادا إلى هذا الانخفاض في عدد الملفات وتغيير اسم المستودع ، تم تخصيص نواة من المشروع ( الالتزام 966 ). يوجد 108287 سطرًا من التعليمات البرمجية في النواة وأصدر المحلل 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 dereference null. النظر في فحص "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 . كما تعلمون ، الفرق كبير بالنسبة للمترجم! هذان متغيرين مختلفين ، وإن كانا مرتبطين. ينتقل الاتصال عبر المتغير الثالث ، 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 تم استخدام كائن '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 || ....) { .... } .... } 

رأى المحلل أن إلغاء التسجيل عن بُعد والتحقق من عدم وجوده أدناه. هذا حقًا NullReferenceException محتمل ، لأن أسلوب FirstOrDefault يمكنه إرجاع القيمة الافتراضية ( خالية لأنواع المرجع). أفترض ، لإصلاح الكود ، ما عليك سوى نقل الشيك أعلاه:

 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 وإزالة التحقق تمامًا.

من الإصدار 7.05 من PVS-Studio:

في الوقت الحالي ، قمنا بتعليق جميع أساليب orDefault من LINQ . سيتم استخدام هذه المعلومات عن طريق التشخيصات الجديدة ، مع الإشارة إلى إلغاء تسجيل نتيجة استدعاء هذه الطرق دون التحقق. لكل طريقة من طرق 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 Warning : V3080 dereference null of value value method. النظر في التفتيش: 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; } 

كما ترون ، يمكن إرجاع فارغة في حالتين. لا يمكن للمحلل تحديد الفرع الذي سيخوضه البرنامج ، وبالتالي فإنه يصنف الرمز على أنه مشبوه. في الكود ، أود أن أضيف على الفور فحصًا فارغًا .

ربما أكون متحيزًا ، ولكن يجب تأمين كل الطرق غير المتزامنة ضد NullReferenceException بأفضل طريقة ممكنة. تصحيح مثل هذه الأشياء هو متعة مشكوك فيها للغاية.

في هذه الحالة ، يحتوي الأسلوب CreateScope على أربعة مكالمات ، وهناك اختباران في اثنين ، ولكن في الحالتين الأخريين مفقود. علاوة على ذلك ، يشبه الزوج الذي لم يتم التحقق منه لصق النسخ (فئة واحدة ، طريقة واحدة ، تم رفضها للاتصال بـ UsingAsync). لقد أجريت بالفعل أول مكالمة من هذا الزوج ، وبطبيعة الحال ، تحتوي المكالمة الثانية أيضًا على تحذير محلل مماثل:

V3080 dereference خالية محتملة لقيمة إرجاع الطريقة. النظر في التفتيش: CreateScope (). SetupService.cs 192

PVS-Studio Warning: 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 (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 dereference null. النظر في فحص "طلب". 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 block ، يحصل متغير الطلب على قيمة غير فارغة في عدة أماكن ، ولكن في كل مكان بشروط متداخلة. لم أذكر كل هذه الشروط ، لأن المثال كان مرهقًا جدًا. الشروط الأولى التي تتحقق من نوع http لطريقة IsGet أو IsPost كافية . لدى فئة Microsoft.AspNetCore.Http.HttpMethods تسعة أساليب ثابتة للتحقق من نوع الطلب. هذا يعني أنه ، على سبيل المثال ، إذا وصل طلب حذف أو تعيين إلى أسلوب ExecuteAsync ، فسيتم طرح NullReferenceException . حتى لو كانت هذه الطرق غير مدعومة على الإطلاق في الوقت الحالي - من الأفضل إجراء فحص مع استثناء. بعد كل شيء ، قد تتغير متطلبات النظام. مثال التحقق أدناه:

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

PVS-Studio Warning : V3080 dereference null of value value method. النظر في التفتيش: الحصول على <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 فارغة . لن أخوض في الحكم على احتمال حدوث ذلك ، لأن هذه الأنواع من مكتبة Newtonsoft.Json ، ولدي خبرة قليلة في ذلك. ولكن المطور يشتبه في أن العنصر المطلوب قد لا يكون. لذلك ، لا تنسى هذا الاحتمال عند الإشارة إلى نتائج القراءة. أود أن أضيف رمية استثناء إلى 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 dereference null. النظر في فحص "النتائج". 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) { .... } .... } 

استنتاج

لا يسعني إلا ملاحظة الجودة العالية للرمز! نعم ، كانت هناك أوجه قصور أخرى لم أتطرق إليها في هذه المقالة ، لكن الأخطر منها تم النظر فيها. بالطبع ، هذا لا يعني أن الفحص كل ثلاث سنوات يكفي. يتم تحقيق الفائدة القصوى من خلال الاستخدام المنتظم للمحلل الثابت ، حيث أن هذا النهج هو الذي يتيح لك اكتشاف المشكلات وحلها في المراحل المبكرة ، عندما تكون تكلفة التعديل وتعقيده في حده الأدنى.

على الرغم من أن الاختبارات التي تتم لمرة واحدة ليست فعالة بقدر الإمكان ، إلا أنني أحثك ​​على تنزيل برنامج PVS-Studio وتجربته في مشروعك - ماذا لو وجدت شيئًا مثيرًا للاهتمام؟

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فالرجاء استخدام الرابط الخاص بالترجمة: Alexander Senichkin. مسح رمز Orchard CMS for Bugs .