Memindai kode Orchard CMS for Bugs

Artikel ini mengulas hasil pemeriksaan kedua proyek Orchard dengan analisa statis PVS-Studio. Orchard adalah sistem pengelola konten sumber terbuka yang dikirimkan sebagai bagian dari Galeri Open Source ASP.NET di bawah Yayasan Outercurve nirlaba. Pemeriksaan hari ini sangat menarik karena baik proyek maupun penganalisa telah berjalan jauh sejak pemeriksaan pertama, dan kali ini kita akan melihat pesan diagnostik baru dan beberapa bug yang bagus.

Tentang Orchard CMS

Kami memeriksa Orchard tiga tahun lalu. Alat analisis C # PVS-Studio telah sangat berkembang sejak saat itu: kami telah meningkatkan analisis aliran data, menambahkan analisis antar-prosedur dan diagnostik baru, dan memperbaiki sejumlah kesalahan positif. Lebih dari itu, cek kedua mengungkapkan bahwa pengembang Orchard telah memperbaiki semua bug yang dilaporkan dalam artikel pertama, yang berarti kami telah mencapai tujuan kami, yaitu membuat mereka membuat kode mereka lebih baik.

Saya berharap mereka akan memperhatikan artikel ini juga dan melakukan perbaikan yang diperlukan atau, lebih baik, mengadopsi PVS-Studio untuk digunakan secara teratur. Sebagai pengingat, kami menyediakan pengembang open-source dengan lisensi gratis. Ngomong-ngomong, ada opsi lain yang bisa dinikmati proyek berpemilik juga.

Kode sumber Orchard tersedia untuk diunduh di sini . Deskripsi proyek lengkap ada di sini . Jika Anda belum memiliki salinan PVS-Studio, Anda dapat mengunduh versi uji coba. Saya menggunakan PVS-Studio 7.05 Beta dan akan menyertakan beberapa peringatannya di artikel ini. Saya harap ulasan ini akan meyakinkan Anda bahwa PVS-Studio adalah alat yang berguna. Perlu diingat bahwa ini dimaksudkan untuk digunakan secara teratur .

Hasil analisis

Berikut adalah beberapa angka dari cek pertama Orchard sehingga Anda tidak perlu beralih di antara dua artikel untuk perbandingan.

Selama pemeriksaan sebelumnya, "kami melakukan analisis terhadap semua file kode sumber (3739 item) dengan ekstensi .cs. Secara total ada 214.564 baris kode. Hasil cek itu 137 peringatan. Untuk lebih tepatnya, ada 39 peringatan tingkat pertama (tinggi). Ada juga peringatan tingkat 60 (menengah) kedua. "

Versi Orchard saat ini terdiri dari 2.767 file .cs, yaitu sekitar seribu file lebih kecil. Perampingan dan penggantian nama repositori menunjukkan bahwa pengembang telah mengisolasi inti proyek ( commit 966 ), yang merupakan 108.287 LOC. Penganalisa mengeluarkan 153 peringatan: 33 tingkat pertama dan 70 tingkat kedua. Kami biasanya tidak memasukkan peringatan tingkat ketiga, dan saya akan tetap berpegang pada tradisi.

Pesan diagnostik PVS-Studio: V3110 Kemungkinan rekursi tak terbatas di dalam metode 'TryValidateModel'. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); } 

Mari kita mulai dengan bug rekursi yang tak terbatas, seperti yang kita lakukan di artikel pertama. Kali ini niat pasti pengembang tidak jelas, tetapi saya perhatikan bahwa metode TryValidateModel memiliki versi kelebihan beban dengan satu parameter:

 public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); } 

Saya pikir, seperti halnya pada versi overload, pengembang bermaksud memanggil metode tersebut melalui _updateModel. Kompiler tidak memperhatikan kesalahan; _updateModel bertipe IUpdateModel , dan kelas saat ini juga mengimplementasikan antarmuka ini. Karena metode ini tidak menyertakan pemeriksaan terhadap StackOverflowException , itu mungkin tidak pernah dipanggil, meskipun saya tidak akan mengandalkan itu. Jika asumsi saya benar, versi tetap akan terlihat seperti ini:

 public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); } 

Pesan diagnostik PVS-Studio: V3008 Variabel 'konten' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 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); .... } 

Penganalisa mendeteksi dua tugas untuk konten variabel lokal . GetChildContentAsync adalah metode pustaka yang terlalu jarang digunakan bagi kami untuk mengambil kesulitan untuk memeriksa dan membubuhi keterangan. Jadi, saya khawatir, baik kita maupun analis tidak tahu apa-apa tentang objek pengembalian metode dan efek sampingnya. Tapi kami tahu pasti bahwa menetapkan nilai kembali ke konten tidak masuk akal jika tidak digunakan lebih lanjut dalam kode. Mungkin itu hanya operasi yang berlebihan daripada kesalahan. Saya tidak bisa mengatakan bagaimana tepatnya ini harus diperbaiki, jadi saya serahkan kepada pengembang.

Pesan diagnostik PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa '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; } 

Alat analisis mendeteksi dereferensi itemTag yang tidak aman. Cuplikan ini adalah contoh yang baik tentang bagaimana alat analisis statis berbeda dari pengembang manusia yang melakukan tinjauan kode. Metode ini memiliki parameter bernama ItemTag dan variabel lokal bernama itemTag . Tidak perlu memberi tahu Anda itu membuat perbedaan besar ke kompiler! Ini adalah dua variabel yang berbeda, meskipun terkait. Cara mereka terkait adalah melalui variabel ketiga, itemTagName. Berikut urutan langkah menuju kemungkinan pengecualian: jika argumen ItemTag sama dengan "-", tidak ada nilai yang akan ditetapkan ke itemTagName , jadi itu akan tetap menjadi referensi nol, dan jika itu adalah referensi nol, maka variabel lokal itemTag akan berubah menjadi referensi nol juga. Menurut pendapat saya, lebih baik untuk memiliki pengecualian yang dilemparkan mengikuti cek string.

 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; } 

Pesan diagnostik PVS-Studio: V3095 Objek 'remoteClient' digunakan sebelum diverifikasi terhadap nol. Periksa baris: 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 || ....) { .... } .... } 

Penganalisa mendeteksi dereference dari remoteClient diikuti oleh pemeriksaan nol beberapa baris kemudian. Ini memang potensi NullReferenceException karena metode FirstOrDefault dapat mengembalikan nilai default (yang nol untuk tipe referensi). Saya kira cuplikan ini dapat diperbaiki dengan hanya memindahkan pemeriksaan sehingga yang mendahului operasi dereference:

 public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... } 

Atau mungkin itu harus diperbaiki dengan mengganti FirstOrDefault dengan First dan menghapus cek sama sekali.

Peringatan oleh PVS-Studio 7.05 Beta:

Sekarang, kami telah memberi anotasi pada semua metode orDefault LINQ . Informasi ini akan digunakan oleh diagnostik baru yang sedang kami kerjakan: mendeteksi kasus-kasus di mana nilai-nilai yang dikembalikan oleh metode-metode ini direferensikan tanpa pemeriksaan sebelumnya. Setiap metode orDefault memiliki rekanan yang melempar pengecualian jika tidak ada elemen yang cocok yang ditemukan. Pengecualian ini akan lebih membantu dalam melacak masalah daripada NullReferenceException abstrak.

Saya tidak bisa tidak membagikan hasil yang saya dapatkan dari diagnostik ini pada proyek Orchard. Ada 27 tempat yang berpotensi berbahaya. Inilah beberapa di antaranya:

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); 

Pesan diagnostik PVS-Studio: V3080 Kemungkinan null dereference nilai pengembalian metode. Pertimbangkan untuk memeriksa: CreateScope (). SetupService.cs 136

 public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... } 

Penganalisa menyebutkan dereferensi nilai yang dikembalikan oleh metode CreateScope . CreateScope adalah metode kecil, jadi inilah implementasinya yang lengkap:

 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; } 

Seperti yang Anda lihat, ada dua kasus di mana ia dapat mengembalikan nol . Analisator tidak tahu cabang mana dari aliran eksekusi yang akan mengikuti, jadi ia bermain aman dan melaporkan kode sebagai mencurigakan. Jika saya menulis kode seperti itu, saya akan segera menulis cek nol.

Mungkin pendapat saya bias, tetapi saya percaya bahwa setiap metode asinkron harus dilindungi dari NullReferenceException sebanyak mungkin karena debugging hal-hal seperti itu jauh dari menyenangkan.

Dalam kasus khusus ini, metode CreateScope disebut empat kali: dua panggilan itu disertai dengan pemeriksaan dan dua lainnya tidak. Dua panggilan terakhir (tanpa pemeriksaan) tampaknya merupakan klon copy-tempel (kelas yang sama, metode yang sama, cara yang sama untuk mendereferensi hasil yang dipanggil dengan MenggunakanAsync) Yang pertama dari dua panggilan itu ditunjukkan di atas, dan Anda mungkin yakin yang kedua memicu peringatan yang sama:

V3080 Kemungkinan nol dereferensi nilai pengembalian metode. Pertimbangkan untuk memeriksa: CreateScope (). SetupService.cs 192

Pesan diagnostik PVS-Studio: V3127 Dua fragmen kode serupa ditemukan. Mungkin, ini adalah kesalahan ketik dan variabel 'AccessTokenSecret' harus digunakan sebagai ganti '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); .... } 

Itu kesalahan salin-tempel yang klasik. ConsumerSecret diperiksa dua kali, sementara AccessTokenSecret tidak diperiksa sama sekali. Jelas, ini diperbaiki sebagai berikut:

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

Pesan diagnostik PVS-Studio: V3139 Dua atau lebih cabang kasus melakukan tindakan yang sama. SerialDocumentExecuter.cs 23

Bug salin-tempel lainnya. Untuk kejelasan, inilah implementasi kelas yang lengkap (ini kecil).

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

Alat analisis tidak menyukai dua cabang kasus yang identik. Memang, kelas memiliki tiga entitas, sedangkan pernyataan switch hanya mengembalikan dua dari mereka. Jika perilaku ini dimaksudkan dan entitas ketiga sebenarnya tidak dimaksudkan untuk digunakan, kode dapat ditingkatkan dengan menghapus cabang ketiga setelah menggabungkan keduanya sebagai berikut:

 switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

Jika ini adalah bug salin-tempel, bidang pengembalian duplikat pertama harus diperbaiki sebagai berikut:

 switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

Atau harus cabang kasus kedua. Saya tidak tahu detail proyek dan karena itu tidak dapat menentukan korelasi antara nama-nama jenis operasi dan strategi.

 switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

Pesan diagnostik PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'permintaan'. 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; .... } 

Variabel permintaan diberikan nilai yang berbeda dari nol beberapa kali di blok if pertama, tetapi setiap kali dengan kondisi bersarang. Termasuk semua kondisi itu akan membuat contoh terlalu lama, jadi kita hanya akan pergi dengan yang pertama, yang memeriksa jenis metode http IsGet atau IsPost . Kelas Microsoft.AspNetCore.Http.HttpMethods memiliki sembilan metode statis untuk memeriksa jenis kueri. Oleh karena itu, meneruskan, misalnya, Hapus atau Set kueri ke metode ExecuteAsync akan mengarah pada peningkatan NullReferenceException . Sekalipun metode seperti itu saat ini tidak didukung sama sekali, akan lebih bijak jika menambahkan cek pelempar pengecualian. Bagaimanapun, persyaratan sistem dapat berubah. Berikut ini contoh cek semacam itu:

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

Pesan diagnostik PVS-Studio: V3080 Kemungkinan null dereference nilai pengembalian metode. Pertimbangkan untuk memeriksa: Dapatkan <ContentPart> (...). ContentPartHandlerCoordinator.cs 190

Sebagian besar peringatan V3080 lebih nyaman untuk dilihat dalam lingkungan pengembangan karena Anda memerlukan navigasi metode, penyorotan jenis, dan suasana IDE yang ramah. Saya mencoba mengurangi teks contoh sebanyak mungkin agar tetap dapat dibaca. Tetapi jika saya tidak melakukannya dengan benar atau jika Anda ingin menguji kemampuan pemrograman Anda dan mencari tahu sendiri, saya sarankan memeriksa hasil diagnostik ini pada proyek open-source atau hanya kode Anda sendiri.

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... } 

Penganalisa melaporkan baris ini. Mari kita lihat metode Get :

 public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); } 

Itu menyebut versi kelebihannya. Mari kita periksa juga:

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

Ternyata jika kita mendapatkan entitas tipe yang tidak kompatibel dengan JObject dari Data menggunakan pengindeks nama , metode Get akan mengembalikan nol . Saya tidak tahu pasti seberapa mungkin itu karena jenis-jenis ini dari perpustakaan Newtonsoft.Json , yang saya belum banyak bekerja. Tetapi pembuat kode curiga bahwa elemen yang dicari mungkin tidak ada, jadi kita harus mengingatnya ketika mengakses hasil operasi baca juga. Secara pribadi, saya akan memiliki pengecualian yang dilemparkan di Dapatkan pertama jika kami percaya simpul harus ada, atau tambahkan cek sebelum dereferensi jika ketidakhadiran simpul tidak mengubah logika keseluruhan (misalnya, kami mendapatkan nilai default).

Solusi 1:

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

Solusi 2:

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... } 

Pesan diagnostik PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'hasil'. ContentQueryOrchardRazorHelperExtensions.cs 19

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... } 

Ini adalah contoh yang cukup sederhana dibandingkan dengan yang sebelumnya. Analisator curiga bahwa metode QueryAsync mungkin mengembalikan referensi nol. Inilah implementasi metode ini:

 public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... } 

Karena GetQueryAsync adalah metode antarmuka, Anda tidak dapat memastikan setiap implementasi, terutama jika kami menganggap bahwa proyek ini juga menyertakan versi berikut:

 public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; } 

Beberapa panggilan ke fungsi eksternal dan akses cache membuat analisis GetDocumentAsync sulit, jadi anggap saja pemeriksaan diperlukan - terlebih lagi karena metode ini merupakan metode yang tidak sinkron.

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... } 

Kesimpulan

Saya tidak bisa tidak menyebutkan kualitas kode Orchard yang tinggi! Benar, ada beberapa cacat lain, yang tidak saya diskusikan di sini, tetapi saya menunjukkan semua bug yang paling parah. Tentu saja, ini bukan untuk mengatakan bahwa memeriksa kode sumber Anda sekali dalam tiga tahun sudah cukup. Anda akan mendapatkan hasil maksimal dari analisis statis jika Anda menggunakannya secara teratur karena ini adalah cara Anda dijamin untuk menangkap dan memperbaiki bug pada tahap pengembangan paling awal, di mana perbaikan bug termurah dan termudah.

Meskipun pemeriksaan satu kali tidak banyak membantu, saya tetap mendorong Anda untuk mengunduh PVS-Studio dan mencobanya di proyek Anda: siapa tahu, mungkin Anda akan menemukan beberapa bug yang menarik juga.