Azure SDK untuk .NET: Kisah tentang Pencarian Kesalahan yang Sulit

Ketika kami memutuskan untuk mencari kesalahan di Azure SDK untuk proyek .NET, kami sangat terkejut dengan ukurannya. "Tiga setengah juta baris kode," kami terus berkata, mempelajari statistik proyek. Mungkin ada begitu banyak temuan. Aduh dan mundur! Proyek itu ternyata licik. Jadi apa semangat proyek dan bagaimana diperiksa - baca di artikel ini.

Tentang proyek

Saya menulis artikel ini menindaklanjuti yang sebelumnya, yang juga tentang proyek yang berkaitan dengan Microsoft Azure: Azure PowerShell: Kebanyakan Tidak Berbahaya . Jadi, kali ini saya bertaruh pada sejumlah kesalahan yang beragam dan menarik. Bagaimanapun, ukuran proyek adalah faktor yang sangat penting dalam hal analisis statis, khususnya ketika memeriksa proyek untuk pertama kalinya. Bahkan, dalam praktiknya, aplikasi pemeriksaan satu kali bukan pendekatan yang tepat. Namun demikian, jika pengembang mendukungnya, itu hanya terjadi pada tahap pengantar analisa. Pada saat yang sama, tidak ada seorang pun yang berhasil memilah-milah sejumlah besar peringatan dan hanya menghapusnya sebagai hutang teknis menggunakan mekanisme penekan peringatan massal dan menyimpannya di pangkalan-pangkalan khusus. Omong-omong, memiliki sejumlah besar peringatan baik-baik saja ketika menjalankan analisa untuk pertama kalinya. Adapun kami, kami pergi untuk pemeriksaan satu kali untuk tujuan penelitian. Untuk alasan ini, proyek besar selalu lebih disukai untuk analisis berikut dibandingkan dengan yang kecil.

Namun, Azure SDK untuk proyek .NET segera terbukti menjadi test bed yang tidak dapat dijalankan. Bahkan ukurannya yang mengesankan tidak membantu, tetapi agak rumit mengerjakannya. Alasannya diberikan dalam statistik proyek berikut:

• File sumber .cs (tidak termasuk tes): 16.500
• Solusi Visual Studio (.sln): 163
• Baris kode yang tidak kosong: 3 462 000
• Dari yang dihasilkan secara otomatis ini: sekitar 3.300.000
• Repositori proyek tersedia di GitHub .

Sekitar 95% kode dihasilkan secara otomatis, dan banyak dari kode itu diulang berkali-kali. Memeriksa proyek-proyek seperti itu dengan analisa statis biasanya memakan waktu dan tidak berguna, karena ada banyak yang bisa diterapkan, tetapi tidak logis (setidaknya pada pandangan pertama) dan kode berlebihan. Ini mengarah pada sejumlah besar kesalahan positif.

Semua jumlah kode yang tersebar di 163 solusi Visual Studio menjadi "cherry on top". Butuh beberapa upaya untuk memeriksa kode yang tersisa (tidak dihasilkan secara otomatis). Apa yang benar-benar membantu adalah kenyataan bahwa semua kode yang dibuat secara otomatis disimpan dalam subdirektori solusi oleh jalur relatif "<Direktori solusi> \ src \ Generated". Juga setiap file .cs dari jenis tersebut berisi komentar khusus dalam tag <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> 

Untuk kemurnian percobaan, saya memeriksa sekitar sepuluh solusi yang dibuat secara acak secara acak. Saya akan ceritakan hasilnya nanti.

Jadi, meskipun jumlah kode “jujur” yang tersisa sedikit, saya masih berhasil menemukan sejumlah kesalahan dari yang tersisa. Kali ini saya tidak akan mengutip peringatan dalam urutan kode diagnostik PVS-Studio. Sebagai gantinya, saya akan mengelompokkan pesan pada solusi yang mereka temukan.

Baiklah, mari kita lihat apa yang berhasil saya temukan di Azure SDK untuk kode .NET.

Microsoft.Azure.Management.Advisor

Ini adalah salah satu dari banyak solusi yang berisi kode yang dibuat secara otomatis. Seperti yang saya katakan sebelumnya, saya secara acak memeriksa sekitar selusin solusi semacam itu. Dalam setiap kasus, peringatan sama, dan, seperti yang diharapkan, tidak berguna. Berikut ini beberapa contohnya.

Ekspresi V3022 'Kredensial! = Null' selalu benar. 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); } } 

Jelas, kode ini berlebihan dan Kredensial! = Pemeriksaan kosong tidak ada gunanya. Namun demikian, kodenya berfungsi. Dan dihasilkan secara otomatis. Untuk alasan ini, tidak ada keluhan di sini.

Ekspresi V3022 '_queryParameters.Count> 0' selalu salah. 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) { .... } .... } 

Sekali lagi, ini tampak seperti konstruksi yang tidak logis. Untuk beberapa alasan, pembuat kode memeriksa ukuran daftar kosong yang baru dibuat. Padahal, itu semua benar. Pada titik ini, cek tidak masuk akal, tetapi jika pengembang menambahkan daftar generasi, misalnya, berdasarkan koleksi lain, cek pasti akan bernilai sementara. Lagi - tidak ada klaim kode, tentu saja, berkaitan dengan asalnya.

Ratusan peringatan serupa telah dikeluarkan untuk setiap solusi yang dibuat secara otomatis. Mengingat kesia-siaan mereka, saya pikir tidak ada gunanya membahas lebih lanjut kasus-kasus seperti itu. Selanjutnya, hanya kesalahan nyata dalam kode "normal" yang akan dipertimbangkan.

Azure. Skor

V3001 Ada sub-ekspresi identik 'buffer.Length' ke kiri dan ke kanan operator '<'. 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."); .... } 

Kesalahan dalam kondisi itu mungkin hasil dari copy-paste. Menurut fakta bahwa buffer disalin dalam array , cek akan terlihat seperti:

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

Lagi pula, seperti yang selalu saya katakan, pembuat kode harus berurusan dengan memperbaiki kesalahan tersebut.

V3083 Doa yang tidak aman dari acara '_onChange', NullReferenceException dimungkinkan. Pertimbangkan menugaskan acara ke variabel lokal sebelum menjalankannya. ClientOptionsMonitor.cs 44

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

Tidak kritis, tetapi ada kesalahan di sini. Konsumen mungkin berhenti berlangganan dari acara antara memeriksa acara untuk nol dan permintaannya. Kemudian variabel _onChange akan menjadi nol dan pengecualian akan dibuang. Kode ini harus ditulis ulang dengan cara yang lebih aman. Misalnya, sebagai berikut:

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

Azure.Messaging.EventHubs

V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa '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); } 

Mari kita lihat apa yang terjadi dengan nilai variabel eventPropertyValue dalam fragmen kode yang diberikan. Variabel diberikan nol pada awal metode. Selanjutnya, dalam salah satu kondisi sakelar pertama, variabel diinisialisasi, setelah itu metode keluar. Blok switch kedua berisi banyak kondisi, di mana masing-masing variabel juga menerima nilai baru. Sedangkan di blok default , variabel eventPropertyValue digunakan tanpa centang, yang merupakan kesalahan, karena variabelnya nol pada saat ini.

V3066 Kemungkinan urutan argumen yang salah diteruskan ke konstruktor 'EventHubConsumer': 'partisiId' dan 'consumerGroup'. TrackOneEventHubClient.cs 394

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

Penganalisa curiga urutan argumen ketiga dan keempat saat memanggil konstruktor kelas EventHubConsumer . Jadi mari kita periksa deklarasi konstruktor ini:

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

Memang, argumen tercampur aduk. Saya berani menyarankan bagaimana kesalahan itu dibuat. Mungkin, salah memformat kode adalah kesalahan di sini. Lihat lagi deklarasi konstruktor EventHubConsumer . Karena fakta bahwa parameter transportConsumer pertama berada di baris yang sama dengan nama kelas, mungkin tampak bahwa parameter partisiId berada di tempat ketiga, bukan yang keempat (komentar saya dengan nomor parameter tidak tersedia dalam kode asli) . Itu hanya dugaan, tetapi saya akan mengubah pemformatan kode konstruktor sebagai berikut:

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

Azure. Penyimpanan

V3112 Kelainan dalam perbandingan serupa. Mungkin salah ketik ada di dalam ekspresi '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; } 

Sebuah kesalahan yang dilakukan karena kurangnya perhatian. Menemukan kesalahan dengan ulasan kode cukup sulit. Ini versi kode yang benar:

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

V3112 Kelainan dalam perbandingan serupa. Mungkin salah ketik ada di dalam ekspresi '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 ; 

Ada kesalahan yang persis sama dalam bagian kode yang sangat mirip. Kode mungkin telah disalin dan sebagian diubah. Namun kesalahan tetap ada.

Microsoft.Azure.Batch

V3053 Ekspresi yang berlebihan. Periksa substring 'IList' dan 'List'. PropertyData.cs 157

V3053 Ekspresi yang berlebihan. Periksa substring 'Daftar' dan '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"); // <= } 

Penganalisa mengeluarkan dua peringatan tentang cek tidak berguna atau salah. Dalam kasus pertama, mencari substring "Daftar" setelah mencari "IList" tampak berlebihan. Memang benar, kondisi ini:

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

dapat diubah dengan baik untuk yang berikut:

 this.Type.Contains("List") 

Dalam kasus kedua, pencarian untuk substring "IReadOnlyList" tidak ada gunanya, karena sebelumnya substring yang lebih pendek "List" dicari.

Ada juga kemungkinan bahwa substring pencarian sendiri memiliki kesalahan dan harus ada sesuatu yang lain. Bagaimanapun, hanya pembuat kode yang menyarankan versi kode yang benar dengan mempertimbangkan kedua komentar.

V3095 Objek 'httpRequest.Content.Headers' digunakan sebelum diverifikasi terhadap nol. Periksa baris: 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; .... } 

Variabel httpRequest.Content.Headers pertama kali digunakan tanpa pemeriksaan, tetapi kemudian diatasi menggunakan operator akses bersyarat.

V3125 Objek 'omPropertyData' digunakan setelah diverifikasi terhadap null. Periksa baris: 156, 148. CodeGenerationUtilities.cs 156

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

Dan ini adalah situasi sebaliknya. Satu blok kode berisi varian akses aman ke omPropertyData yang berpotensi menjadi referensi nol. Lebih lanjut dalam kode, referensi ini ditangani tanpa pemeriksaan.

V3146 Kemungkinan nol dereferensi 'nilai'. 'FirstOrDefault' dapat mengembalikan nilai nol default. 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(); .... } .... } 

Karena metode FirstOrDefault , jika pencarian gagal, nilai default akan dikembalikan, yang merupakan nol untuk tipe string . Nilai akan ditetapkan ke variabel nilai , yang kemudian digunakan dalam kode dengan metode Ganti tanpa pemeriksaan apa pun. Kode harus dibuat lebih aman. Misalnya, sebagai berikut:

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

Microsoft.Azure.ServiceBus

V3121 Enumerasi 'BlocksUsing' dideklarasikan dengan atribut 'Flags', tetapi tidak mengatur inisialisasi apa pun untuk mengesampingkan nilai default. 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, } .... } .... } 

Enumerasi dideklarasikan dengan atribut Flags . Pada saat yang sama, nilai-nilai konstan dibiarkan secara default ( MonitorEnter = 0 , MonitorWait = 1 , ManualResetEvent = 2 dan seterusnya). Ini dapat menghasilkan kasus berikut: ketika mencoba menggunakan kombinasi bendera, misalnya, konstanta kedua dan ketiga MonitorWait (= 1) | ManualResetEvent (= 2) , bukan nilai unik yang akan diterima, tetapi konstan dengan nilai 3 secara default (AutoResetEvent ). Ini mungkin mengejutkan bagi kode penelepon. Jika enumerasi BlocksUsing benar-benar digunakan untuk mengatur kombinasi flags (bidang bit), konstanta harus diberi nilai, sama dengan angka yang merupakan kekuatan dua.

 [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 Objek 'sesi' digunakan setelah diverifikasi terhadap nol. Periksa baris: 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()); } .... } 

Perhatikan penanganan variabel sesi di blok tangkap . Metode Abort dipanggil dengan aman oleh operator akses bersyarat. Tetapi setelah metode GetInnerException disebut tidak aman. Dengan melakukan hal itu, NullReferenceException mungkin dilemparkan bukan pengecualian dari tipe yang diharapkan. Kode ini harus diperbaiki. Metode AmqpExceptionHelper.GetClientException mendukung melewati nilai nol untuk parameter innerException :

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

Oleh karena itu, seseorang hanya dapat menggunakan operator akses bersyarat saat memanggil sesi. DapatkanInnerException () :

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

Kesimpulan

Seperti yang Anda lihat, ukuran proyek besar tidak selalu menjamin banyak kesalahan. Namun, kami tetap waspada karena kami selalu dapat menemukan sesuatu. Bahkan dalam proyek yang kompleks secara struktural seperti Azure SDK untuk .NET. Menemukan beberapa cacat krusial membutuhkan upaya tambahan. Tetapi semakin banyak kesulitan semakin menyenangkan hasilnya. Di sisi lain, untuk menghindari upaya yang tidak semestinya, kami sarankan menggunakan analisis statis langsung di komputer pengembang saat menulis kode baru. Ini adalah pendekatan yang paling efektif. Unduh dan coba PVS-Studio dalam aksi. Semoga berhasil dalam memerangi serangga!