تستعرض هذه المقالة نتائج الفحص الثاني لمشروع Orchard مع محلل ثابت PVS-Studio. Orchard هو نظام إدارة محتوى مفتوح المصدر يتم تقديمه كجزء من معرض ASP.NET Open Source تحت مؤسسة Outercurve Foundation غير الربحية. الاختيار اليوم مثير للاهتمام بشكل خاص لأن كل من المشروع والمحلل قطعتا شوطًا طويلاً منذ الفحص الأول ، وهذه المرة سننظر في رسائل تشخيصية جديدة وبعض الأخطاء الجيدة.
حول بستان سمفحصنا بستان قبل ثلاث سنوات. تطورت محلل C # الخاص بـ PVS-Studio بشكل كبير منذ ذلك الحين: لقد قمنا بتحسين تحليل تدفق البيانات ، وإضافة تحليل interprocedural وتشخيصات جديدة ، وإصلاح عدد من الإيجابيات الخاطئة. أكثر من ذلك ، كشف الفحص الثاني أن مطوري Orchard قاموا بإصلاح جميع الأخطاء التي تم الإبلاغ عنها في المقالة الأولى ، مما يعني أننا حققنا هدفنا ، أي جعلهم جعلوا الكود أفضل.
آمل أن ينتبهوا إلى هذه المقالة أيضًا وأن يقوموا بالإصلاحات الضرورية ، أو الأفضل من ذلك ، اعتماد PVS-Studio للاستخدام بشكل منتظم. للتذكير ، نحن نوفر للمطورين ذوي
المصادر المفتوحة ترخيص مجاني. بالمناسبة ، هناك
خيارات أخرى قد تتمتع بها مشاريع الملكية أيضًا.
شفرة مصدر بستان متاحة للتحميل
هنا . الوصف الكامل للمشروع موجود
هنا . إذا لم يكن لديك نسخة PVS-Studio حتى الآن ، يمكنك
تنزيل الإصدار التجريبي. استخدمت PVS-Studio 7.05 Beta وسوف أدرج بعض التحذيرات الخاصة به في هذه المقالة. آمل أن يقنعك هذا الاستعراض بأن PVS-Studio أداة مفيدة. فقط ضع في اعتبارك أنه من المفترض استخدامه
بانتظام .
نتائج التحليلفيما يلي بعض الأشكال من الفحص الأول لأوركارد حتى لا تضطر إلى التبديل بين المادتين للمقارنة.
أثناء الفحص السابق ، "قمنا بتحليل جميع ملفات التعليمات البرمجية المصدر (3739 عنصرًا) بامتداد .cs. في المجموع ، كان هناك 214.564 سطرًا من التعليمات البرمجية. وكانت نتيجة الشيك 137 تحذيرا. لكي نكون أكثر دقة ، كان هناك 39 تحذيرات من المستوى الأول (عالية). كان هناك أيضا 60 تحذيرات من المستوى الثاني (المتوسط). "
يتكون الإصدار الحالي من Orchard من 2767 ملف. cs ، أي حوالي ألف ملف أصغر. يشير تقليص حجم المستودع وإعادة تسميته إلى أن المطورين قاموا بعزل جوهر المشروع (
الالتزام 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 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 و متغير محلي باسم
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 || ....) { .... } .... }
اكتشف المحلل وجود
تردد في
remoteClient متبوعًا بفحص فارغ
لسطرين لاحقًا. هذا بالفعل
NullReferenceException محتمل لأن الأسلوب
FirstOrDefault قد ترجع قيمة افتراضية (والتي هي
فارغة لأنواع المرجع). أعتقد أنه يمكن إصلاح هذا المقتطف ببساطة عن طريق تحريك الفحص بحيث يسبق عملية dereference:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
أو ربما يجب إصلاحه عن طريق استبدال
FirstOrDefault بـ "
أولاً" وإزالة التحقق تمامًا.
تحذيرات 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 dereference خالية محتملة لقيمة إرجاع الطريقة. النظر في التفتيش: 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);
كما ترون ، هناك حالتان يمكنهما فيه العودة. لا يعرف المحلل الفرع الذي سيتبعه تدفق التنفيذ ، لذلك يتم تشغيله بشكل آمن ويبلغ عن الكود المشبوه. إذا كنت أكتب رمزًا كهذا ، سأكتب لك فحصًا فارغًا على الفور.
ربما يكون رأيي منحازًا ، لكنني أعتقد أن كل طريقة غير متزامنة يجب حمايتها من
NullReferenceException قدر الإمكان لأن تصحيح الأخطاء مثل هذه الأشياء بعيدًا عن المتعة.
في هذه الحالة بالذات ، يتم استدعاء الأسلوب
CreateScope أربع مرات: اثنين من تلك المكالمات مصحوبة بشيكات
والآخران لا. يبدو أن الاستدعاءات الأخيرة (بدون الاختبارات) عبارة عن نسخ نسخ لصق (نفس الفئة ونفس الطريقة وطريقة أخرى لإلغاء تحديد النتيجة لاستدعاء UsingAsync). تم عرض أول مكالمة من هاتين المكالمتين أعلاه ، وقد تكون متأكدًا من أن المكالمة الثانية قد أطلقت نفس التحذير:
V3080 dereference خالية محتملة لقيمة إرجاع الطريقة. النظر في التفتيش: CreateScope (). SetupService.cs 192
رسالة تشخيص PVS-Studio: V3127 تم العثور على شظايا رمز مشابه. ربما ، هذا خطأ مطبعي ويجب استخدام متغير "AccessTokenSecret" بدلاً من TwitterClientMessageHandler.cs 52 "ConsumerSecret".
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; .... }
يتم تعيين متغير
الطلب قيمة مختلفة عن
فارغة عدة مرات في أول كتلة
إذا كان ، ولكن في كل مرة مع الظروف المتداخلة. سيؤدي تضمين كل هذه الشروط إلى جعل المثال طويلاً ، لذا سننتقل فقط مع الحالات القليلة الأولى ، التي تتحقق من طريقة 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: V3080 dereference خالية محتملة لقيمة إرجاع الطريقة. النظر في التفتيش: الحصول على <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; } .... }
اتضح أنه إذا حصلنا على كيان من نوع غير متوافق مع
JObject from
Data باستخدام مفهرس
الاسم ،
فستعود طريقة
Get فارغة . لا أعرف على وجه اليقين مدى احتمال حدوث ذلك لأن هذه الأنواع من مكتبة
Newtonsoft.Json ، التي لم أعمل معها كثيرًا. لكن كاتب الشفرة كان يشك في أن العنصر المطلوب قد لا يكون موجودًا ، لذلك يجب أن نضع ذلك في الاعتبار عند الوصول إلى نتيجة عملية القراءة أيضًا. شخصياً ، سيكون لدي استثناء في الحالة الأولى
احصل على إذا كنا نعتقد أن العقدة يجب أن تكون موجودة ، أو أضف علامة تحقق قبل تحديد الاتجاه إذا كان عدم وجود العقدة لا يغير المنطق العام (على سبيل المثال ، نحصل على القيمة الافتراضية).
الحل 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 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 وتجربته في مشروعك: من يدري ، ربما ستجد بعض الأخطاء المثيرة للاهتمام أيضًا.