Azure SDK for .NET: قصة بحث عن خطأ صعب

عندما قررنا البحث عن أخطاء في مشروع Azure SDK for .NET ، فوجئنا بسرورها بحجمه. "ثلاثة ملايين ونصف سطر من الكود" ، واصلنا القول ، ودراسة إحصائيات المشروع. قد يكون هناك الكثير من النتائج. للأسف وقليلا! تحول المشروع إلى أن يكون ماكرة. إذن ما هو الحماس في المشروع وكيف تم فحصه - اقرأ في هذا المقال.

عن المشروع

أكتب إليكم هذه المقالة لمتابعة مقالتي السابقة ، والتي كانت أيضًا عن مشروع متعلق بـ Microsoft Azure: Azure PowerShell: Mostly Harmless . لذا ، هذه المرة كنت أراهن على عدد كبير من الأخطاء المتنوعة والمثيرة للاهتمام. بعد كل شيء ، حجم المشروع هو عامل مهم للغاية من حيث التحليل الثابت ، لا سيما عند التحقق من المشروع لأول مرة. في الواقع ، في الممارسة العملية ، تطبيق الشيكات لمرة واحدة ليس هو النهج الصحيح. ومع ذلك ، إذا استمر المطورون في ذلك ، فسيحدث ذلك فقط في مرحلة تقديم المحلل. في الوقت نفسه ، لا أحد يعمل بعنف على فرز العدد الهائل من التحذيرات وفوقها كدين تقني باستخدام آليات قمع الإنذار الشامل وتخزينها في قواعد خاصة. الحديث عن ذلك ، وجود عدد كبير من التحذيرات على ما يرام عند تشغيل محلل لأول مرة. بالنسبة لنا ، نحن نذهب لفحص لمرة واحدة لأغراض البحث. لهذا السبب ، تكون المشروعات الكبيرة دائمًا أكثر تفضيلًا للتحليل التالي مقارنة بالمشاريع الصغيرة.

ومع ذلك ، فقد أثبت مشروع Azure SDK for .NET على الفور أنه سرير اختبار غير قابل للحياة. حتى حجمها المثير للإعجاب لم يساعد ، ولكن العمل معقد إلى حد ما. ويرد السبب في إحصاءات المشروع التالية:

• ملفات المصدر .cs (لا تشمل الاختبارات): 16،500
• حلول Visual Studio (.sln): 163
• أسطر الكود غير الفارغة: 000 462 3
• من هذه ولدت تلقائيا: حوالي 3300000
• مستودع المشروع متاح على جيثب .

يتم إنشاء 95٪ تقريبًا من الشفرة تلقائيًا ، ويتكرر الكثير من هذا الرمز عدة مرات. عادةً ما يكون التحقق من هذه المشاريع باستخدام محلل ثابت وقتًا طويلاً وعديم الجدوى ، حيث يوجد الكثير من التعليمات العملية وغير المنطقية (على الأقل للوهلة الأولى) والرمز الزائد عن الحاجة. هذا يؤدي إلى عدد كبير من الإيجابيات الخاطئة.

أصبح كل هذا العدد من التعليمات البرمجية المنتشرة في جميع أنحاء 163 حلول Visual Studio "الكرز في القمة". استغرق الأمر بعض الجهود للتحقق من الكود المتبقي (لا يتم إنشاؤه تلقائيًا). ما ساعد حقًا هو حقيقة أنه تم تخزين جميع التعليمات البرمجية التي تم إنشاؤها تلقائيًا في الدلائل الفرعية للحلول عن طريق المسار النسبي "<دليل الحلول> \ src \ Generated". يحتوي أيضًا كل ملف .cs من هذا النوع على تعليق خاص في العلامة <auto-generated> :

// <auto-generated> // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for // license information. // // Code generated by Microsoft (R) AutoRest Code Generator. // Changes may cause incorrect behavior and will be lost if the code is // regenerated. // </auto-generated> 

من أجل نقاء التجربة ، راجعت بشكل تقريبي حوالي عشرة حلول تم إنشاؤها تلقائيًا بشكل عشوائي. سأقول عن النتيجة في وقت لاحق.

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

حسنًا ، دعنا نرى ما تمكنت من العثور عليه في Azure SDK لرمز .NET.

Microsoft.Azure.Management.Advisor

هذا أحد الحلول الكثيرة التي تحتوي على تعليمات برمجية تم إنشاؤها تلقائيًا. كما قلت سابقًا ، راجعت بشكل عشوائي نحو عشرة من هذه الحلول. في كل حالة ، كانت التحذيرات هي نفسها ، وكما هو متوقع ، عديمة الفائدة. هنا بعض الأمثلة.

V3022 التعبير "بيانات الاعتماد! = لاغ " صحيح دائمًا. AdvisorManagementClient.cs 204

 // Code generated by Microsoft (R) AutoRest Code Generator. .... public ServiceClientCredentials Credentials { get; private set; } .... public AdvisorManagementClient(ServiceClientCredentials credentials, params DelegatingHandler[] handlers) : this(handlers) { if (credentials == null) { throw new System.ArgumentNullException("credentials"); } Credentials = credentials; if (Credentials != null) // <= { Credentials.InitializeServiceClient(this); } } 

من الواضح أن هذا الكود لا لزوم له وبيانات الاعتماد! = التحقق من الصحة لا معنى له. ومع ذلك ، يعمل الرمز. ويتم إنشاؤه تلقائيًا. لهذا السبب ، لا شكاوى هنا.

تعبير V3022 '_queryParameters.Count> 0' غير صحيح دائمًا. ConfigurationsOperations.cs 871

 // Code generated by Microsoft (R) AutoRest Code Generator. .... public async Task<AzureOperationResponse<IPage<ConfigData>>> ListBySubscriptionNextWithHttpMessagesAsync(....) { .... List<string> _queryParameters = new List<string>(); if (_queryParameters.Count > 0) { .... } .... } 

مرة أخرى ، يبدو وكأنه بناء غير منطقي. لسبب ما ، يتحقق مؤلفو الكود من حجم القائمة الفارغة التي تم إنشاؤها حديثًا. في الواقع ، كل هذا صحيح. في هذه المرحلة ، لن يكون هناك أي معنى للشيك ، ولكن في حالة إضافة المطورين إلى إنشاء قائمة ، على سبيل المثال ، استنادًا إلى مجموعة أخرى ، سيكون التحقق بالتأكيد جديراً بالاهتمام. مرة أخرى - لا توجد مطالبات إلى الكود ، بالطبع ، فيما يتعلق بأصله.

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

Azure.Core

V3001 هناك تعبيرات فرعية مماثلة "buffer.Length" إلى اليسار وإلى يمين المشغل "<". AzureBaseBuffersExtensions.cs 30

 public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....) { byte[]? array = null; .... if (array == null || buffer.Length < buffer.Length) // <= { if (array != null) ArrayPool<byte>.Shared.Return(array); array = ArrayPool<byte>.Shared.Rent(buffer.Length); } if (!buffer.TryCopyTo(array)) throw new Exception("could not rent large enough buffer."); .... } 

ربما كان الخطأ في الشرط هو نتيجة لصق النسخ. وفقًا لحقيقة أنه يتم نسخ المخزن المؤقت في صفيف ، يجب أن يبدو الاختيار كما يلي:

 if (array == null || array.Length < buffer.Length) 

على أي حال ، كما أقول دائمًا ، يجب أن يتعامل مؤلف التعليمات البرمجية مع إصلاح هذه الأخطاء.

V3083 الاحتجاج غير الآمن للحدث "_onChange" ، NullReferenceException ممكن. النظر في تعيين الحدث إلى متغير محلي قبل استدعاء ذلك. ClientOptionsMonitor.cs 44

 private event Action<TOptions, string> _onChange; .... private void InvokeChanged(....) { .... if (_onChange != null) { _onChange.Invoke(options, name); } } 

ليست حرجة ، ولكن هناك خطأ هنا. يمكن للمستهلك إلغاء الاشتراك من الحدث بين التحقق من الحدث لإلغاء الدعوى واستدعائه. بعد ذلك سيكون متغير _onChange خاليًا وسيتم طرح استثناء. يجب إعادة كتابة هذا الرمز بطريقة أكثر أمانًا. على سبيل المثال ، كما يلي:

 private void InvokeChanged(....) { .... _onChange?.Invoke(options, name); } 

Azure.Messaging.EventHubs

V3080 dereference ممكن. النظر في فحص "eventPropertyValue". AmqpMessageConverter.cs 650

 private static bool TryCreateEventPropertyForAmqpProperty( object amqpPropertyValue, out object eventPropertyValue) { eventPropertyValue = null; .... switch (GetTypeIdentifier(amqpPropertyValue)) { case AmqpProperty.Type.Byte: .... case AmqpProperty.Type.String: eventPropertyValue = amqpPropertyValue; return true; .... } .... switch (amqpPropertyValue) { case AmqpSymbol symbol: eventPropertyValue = ....; break; case byte[] array: eventPropertyValue = ....; break; case ArraySegment<byte> segment when segment.Count == segment.Array.Length: eventPropertyValue = ....; break; case ArraySegment<byte> segment: .... eventPropertyValue = ....; break; case DescribedType described when (described.Descriptor is AmqpSymbol): eventPropertyValue = ....; break; default: var exception = new SerializationException( string.Format(...., eventPropertyValue.GetType().FullName)); // <= .... } return (eventPropertyValue != null); } 

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

V3066 تم ترتيب الوسيطات غير الصحيحة المحتملة إلى مُنشئ "EventHubConsumer": "partitionId" و "ConsumerGroup". TrackOneEventHubClient.cs 394

 public override EventHubConsumer CreateConsumer(....) { return new EventHubConsumer ( new TrackOneEventHubConsumer(....), TrackOneClient.EventHubName, partitionId, // <= 3 consumerGroup, // <= 4 eventPosition, consumerOptions, initialRetryPolicy ); } 

يشتبه المحلل في الترتيب الخلط بين الوسيطتين الثالثة والرابعة عند استدعاء مُنشئ فئة EventHubConsumer . لذلك دعونا نتحقق من إعلان المنشئ:

 internal EventHubConsumer(TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, // <= 3 string partitionId, // <= 4 EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... } 

في الواقع ، يتم خلط الحجج. كنت أجازف أن أقترح كيف تم ارتكاب الخطأ. ربما ، تنسيق رمز غير صحيح هو المسؤول هنا. مجرد إلقاء نظرة أخرى على إعلان منشئ EventHubConsumer . نظرًا لحقيقة أن المعلمة الأولى transportConsumer موجودة على نفس السطر مع اسم الفئة ، فقد يبدو أن المعلمة partitionId في المرتبة الثالثة وليست الرابعة (تعليقاتي مع أرقام المعلمات غير متوفرة في الكود الأصلي) . هذا مجرد تخمين ، لكنني أود تغيير تنسيق كود المنشئ إلى ما يلي:

 internal EventHubConsumer ( TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, string partitionId, EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... } 

Azure.Storage

V3112 خلل في مقارنات مماثلة. من الممكن أن يكون هناك خطأ مطبعي موجود داخل التعبير "ContentLanguage == other.ContentEncoding". BlobSasBuilder.cs 410

 public struct BlobSasBuilder : IEquatable<BlobSasBuilder> { .... public bool Equals(BlobSasBuilder other) => BlobName == other.BlobName && CacheControl == other.CacheControl && BlobContainerName == other.BlobContainerName && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding && // <= ContentLanguage == other.ContentEncoding && // <= ContentType == other.ContentType && ExpiryTime == other.ExpiryTime && Identifier == other.Identifier && IPRange == other.IPRange && Permissions == other.Permissions && Protocol == other.Protocol && StartTime == other.StartTime && Version == other.Version; } 

خطأ ارتكبه الغفلة. العثور على مثل هذا الخطأ في مراجعة الكود أمر صعب للغاية. هنا هو الإصدار الصحيح من التعليمات البرمجية:

  .... ContentEncoding == other.ContentEncoding && ContentLanguage == other.ContentLanguage && .... 

V3112 خلل في مقارنات مماثلة. من الممكن أن يكون هناك خطأ مطبعي موجود داخل التعبير "ContentLanguage == other.ContentEncoding". FileSasBuilder.cs 265

 public struct FileSasBuilder : IEquatable<FileSasBuilder> { .... public bool Equals(FileSasBuilder other) => CacheControl == other.CacheControl && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding // <= && ContentLanguage == other.ContentEncoding // <= && ContentType == other.ContentType && ExpiryTime == other.ExpiryTime && FilePath == other.FilePath && Identifier == other.Identifier && IPRange == other.IPRange && Permissions == other.Permissions && Protocol == other.Protocol && ShareName == other.ShareName && StartTime == other.StartTime && Version == other.Version ; 

يوجد بالضبط نفس الخطأ في جزء مشابه جدًا من التعليمات البرمجية. ربما تم نسخ الكود وتغييره جزئيًا. لكن الخطأ بقي.

Microsoft.Azure.Batch

V3053 تعبير مفرط. فحص سلاسل "IList" و "قائمة". PropertyData.cs 157

V3053 تعبير مفرط. فحص substrings "قائمة" و "IReadOnlyList". PropertyData.cs 158

 public class PropertyData { .... public bool IsTypeCollection => this.Type.Contains("IList") || this.Type.Contains("IEnumerable") || this.Type.Contains("List") || // <= this.Type.Contains("IReadOnlyList"); // <= } 

أصدر المحلل تحذيرين حول الشيكات غير المجدية أو الخاطئة. في الحالة الأولى ، يبدو البحث عن السلسلة الفرعية "List" بعد البحث عن "IList" زائداً عن الحاجة. هذا صحيح ، هذا الشرط:

 this.Type.Contains("IList") || this.Type.Contains("List") 

يمكن تغييره بشكل جيد لما يلي:

 this.Type.Contains("List") 

في الحالة الثانية ، البحث عن السلسلة الفرعية "IReadOnlyList" لا معنى له ، حيث سبق البحث عن سلسلة فرعية أقصر "قائمة".

هناك أيضًا فرصة لحدوث أخطاء في سلاسل البحث نفسها ويجب أن يكون هناك شيء آخر. على أي حال ، فإن مؤلف الشفرة فقط هو اقتراح إصدار الكود الصحيح مع مراعاة كلا التعليقين.

V3095 تم استخدام عنصر 'httpRequest.Content.Headers' قبل أن يتم التحقق منه ضد قيمة خالية. خطوط التحقق: 76 ، 79. BatchSharedKeyCredential.cs 76

 public override Task ProcessHttpRequestAsync( HttpRequestMessage httpRequest, ....) { .... signature.Append(httpRequest.Content != null && httpRequest.Content.Headers.Contains("Content-Language") ? .... : ....; long? contentLength = httpRequest.Content?.Headers?.ContentLength; .... } 

يتم استخدام المتغير httpRequest.Content.Headers أولاً دون أي اختبارات ، ولكن يتم معالجته لاحقًا باستخدام عامل الوصول المشروط.

V3125 تم استخدام كائن "omPropertyData" بعد أن تم التحقق منه مقابل خالية. خطوط التحقق: 156 ، 148. CodeGenerationUtilities.cs 156

 private static string GetProtocolCollectionToObjectModelCollectionString( ...., PropertyData omPropertyData, ....) { if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....)) { .... } if (IsTypeComplex(omPropertyData.GenericTypeParameter)) .... } 

وهنا هو الموقف العكسي. كتلة واحدة من التعليمات البرمجية تحتوي على الوصول الآمن البديل إلى مرجع omPropertyData يحتمل أن تكون خالية. كذلك في الكود ، تتم معالجة هذا المرجع دون أي عمليات تدقيق.

V3146 التراجع المحتمل الفارغ لـ "القيمة". يمكن لـ 'FirstOrDefault' إرجاع القيمة الخالية الافتراضية. BatchSharedKeyCredential.cs 127

 public override Task ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....) { .... foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } .... } 

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

 foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } 

Microsoft.Azure.ServiceBus

V3121 تم إعلان التعداد "BlocksUsing" باستخدام سمة "الإشارات" ، لكنه لا يقوم بتعيين أي مُهيئات لتجاوز القيم الافتراضية. Fx.cs 69

 static class Fx { .... public static class Tag { .... [Flags] public enum BlocksUsing { MonitorEnter, MonitorWait, ManualResetEvent, AutoResetEvent, AsyncResult, IAsyncResult, PInvoke, InputQueue, ThreadNeutralSemaphore, PrivatePrimitive, OtherInternalPrimitive, OtherFrameworkPrimitive, OtherInterop, Other, NonBlocking, } .... } .... } 

يتم تعريف التعداد مع سمة العلامات . في نفس الوقت ، يتم ترك القيم الثابتة بشكل افتراضي ( MonitorEnter = 0 ، MonitorWait = 1 ، ManualResetEvent = 2 وهكذا). قد ينتج عن هذا الحالة التالية: عند محاولة استخدام مجموعة العلامات ، على سبيل المثال ، الثوابت الثانية والثالثة MonitorWait (= 1) | ManualResetEvent (= 2) ، لن يتم تلقي قيمة فريدة ، ولكن الثابت مع القيمة 3 بشكل افتراضي (AutoResetEvent ). قد يكون هذا بمثابة مفاجأة لرمز المتصل. إذا كان حقاً استخدام تعداد BlocksUsing في إعداد مجموعات الإشارات (حقل البت) ، فيجب إعطاء الثوابت قيمًا تساوي الرقم الذي يمثل قوة لاثنين.

 [Flags] public enum BlocksUsing { MonitorEnter = 1, MonitorWait = 2, ManualResetEvent = 4, AutoResetEvent = 8, AsyncResult = 16, IAsyncResult = 32, PInvoke = 64, InputQueue = 128, ThreadNeutralSemaphore = 256, PrivatePrimitive = 512, OtherInternalPrimitive = 1024, OtherFrameworkPrimitive = 2048, OtherInterop = 4096, Other = 8192, NonBlocking = 16384, } 

V3125 تم استخدام عنصر "الجلسة" بعد أن تم التحقق منه ضد قيمة خالية. خطوط الفحص: 69 ، 68. AmqpLinkCreator.cs 69

 public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try { // Create Session .... } catch (Exception exception) { .... session?.Abort(); throw AmqpExceptionHelper.GetClientException(exception, null, session.GetInnerException(), amqpConnection.IsClosing()); } .... } 

إيلاء الاهتمام لمتغير جلسة العمل في كتلة الصيد . يتم استدعاء طريقة إحباط بأمان من قبل مشغل الوصول الشرطي. ولكن بعد استدعاء الأسلوب GetInnerException بطريقة غير آمنة. في القيام بذلك ، قد يتم طرح NullReferenceException بدلاً من استثناء من النوع المتوقع. يجب إصلاح هذا الرمز. يعتمد أسلوب AmqpExceptionHelper.GetClientException تمرير القيمة الخالية للمعلمة innerException :

 public static Exception GetClientException( Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false) { .... } 

لذلك ، يمكن للمرء استخدام عامل الوصول المشروط فقط عند استدعاء session.GetInnerException () :

 public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try { // Create Session .... } catch (Exception exception) { .... session?.Abort(); throw AmqpExceptionHelper.GetClientException(exception, null, session?.GetInnerException(), amqpConnection.IsClosing()); } .... } 

استنتاج

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