Kami mencari dan menganalisis kesalahan dalam kode CMS Orchard

Artikel ini adalah hasil dari pengecekan ulang proyek Orchard menggunakan analisa statis PVS-Studio. Orchard adalah sistem manajemen konten sumber terbuka yang merupakan bagian dari Outercurve Foundation, galeri proyek ASP.NET nirlaba. Tes ini menarik karena proyek dan analisa telah tumbuh secara signifikan sejak analisis pertama. Positif baru dan bug menarik sedang menunggu Anda.

Tentang Orchard CMS

Kami menguji Orchard dengan PVS-Studio tiga tahun lalu. Alat analisis C # telah berkembang pesat: kami telah meningkatkan analisis aliran data, mengembangkan analisis antar-prosedur, menambahkan diagnostik baru dan memperbaiki sejumlah kesalahan positif. Selain itu, analisis menunjukkan bahwa semua komentar dari artikel sebelumnya telah diperbaiki. Ini berarti tujuan tercapai - kode menjadi lebih baik.

Saya ingin percaya bahwa pengembang akan memperhatikan artikel ini dan melakukan perubahan yang diperlukan. Bahkan lebih baik, jika Anda memperkenalkan praktik penggunaan PVS-Studio secara teratur. Biarkan saya mengingatkan Anda bahwa untuk proyek - proyek sumber terbuka kami menyediakan versi gratis dari lisensi. Omong-omong, ada opsi lain yang cocok untuk proyek tertutup.

Kode proyek Orchard dapat diunduh dari sini , seperti yang saya lakukan. Deskripsi lengkap tentang proyek ini dapat ditemukan di sini . Jika Anda belum memiliki alat analisa PVS-Studio kami, Anda dapat mengunduh versi uji coba dari sini . Saya menggunakan versi PVS-Studio versi 7.05 Beta. Saya akan membagikan hasil pekerjaannya. Saya harap Anda setuju dengan kegunaan menggunakan PVS-Studio. Hal utama adalah menggunakan alat analisa secara teratur .

Hasil Validasi

Saya akan memberi Anda beberapa angka dari artikel terakhir sehingga Anda tidak perlu beralih untuk membandingkan.

“Semua file (3739 lembar) dengan ekstensi .cs ikut serta dalam pemeriksaan terakhir. Secara total, mereka berisi 214.564 baris kode. Berdasarkan hasil audit, 137 peringatan diterima. Pada tingkat kepercayaan pertama (tinggi), 39 peringatan diterima. Di level kedua (rata-rata), 60 peringatan diterima. "

Saat ini, proyek ini memiliki 2767 file .cs, yaitu, proyek tersebut telah menjadi kurang dari seribu file. Dilihat oleh penurunan jumlah file dan perubahan nama repositori, kernel dialokasikan dari proyek ( komit 966 ). Ada 108.287 baris kode di kernel dan penganalisis mengeluarkan 153 peringatan, 33 di tingkat tinggi, 70 di rata-rata. Kami biasanya tidak mempertimbangkan peringatan tingkat ketiga, saya tidak melanggar tradisi.

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

Seperti pada artikel sebelumnya, kami membuka daftar kesalahan dengan rekursi tak terbatas. Sulit untuk mengatakan apa sebenarnya yang ingin dilakukan pengembang dalam kasus ini. Tapi saya perhatikan bahwa metode TryValidateModel memiliki argumen berlebih yang terlihat seperti ini:

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

Saya berasumsi bahwa, seperti halnya metode kelebihan beban, pengembang ingin memanggil metode melalui _updateModel. Compiler tidak melihat kesalahan, _updateModel bertipe IUpdateModel dan kelas saat ini juga mengimplementasikan antarmuka ini. Karena tidak ada kondisi tunggal dalam metode untuk melindungi terhadap StackOverflowException , metode ini mungkin tidak pernah dipanggil sekali pun, tapi saya tidak akan mengandalkannya. Jika asumsi ini benar, versi yang diperbaiki akan terlihat seperti ini:

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

PVS-Studio Warning: V3008 Variabel 'content' 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 melihat dua tugas untuk konten variabel lokal . GetChildContentAsync adalah metode dari pustaka yang tidak cukup umum untuk kita putuskan dan anotasi. Jadi, sayangnya, baik kita maupun penganalisa tidak tahu apa-apa tentang objek yang dikembalikan metode dan efek sampingnya. Tetapi kita dapat dengan jelas mengatakan bahwa menetapkan hasil dalam konten tidak masuk akal tanpa digunakan. Mungkin ini bukan kesalahan sama sekali, hanya operasi ekstra. Saya tidak bisa sampai pada kesimpulan yang jelas tentang cara memperbaikinya. Saya akan menyerahkan keputusan ini kepada pengembang.

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

Dalam kode ini, penganalisa mendeteksi itemTere dereferencing berbahaya. Contoh yang baik tentang perbedaan antara penganalisa statis dan orang yang diuji. Metode ini memiliki parameter bernama ItemTag dan variabel lokal yang disebut itemTag . Seperti yang Anda tahu, perbedaannya sangat besar untuk kompiler! Ini adalah dua variabel yang berbeda, meskipun terkait. Koneksi melewati variabel ketiga, itemTagName. Skenario untuk melempar pengecualian adalah ini: argumen ItemTag adalah "-", itemTagName tidak diberi nilai, itu akan tetap menjadi referensi nol, dan jika itu adalah referensi nol, itemTag lokal juga akan menjadi referensi nol. Saya pikir lebih baik untuk melemparkan pengecualian di sini setelah memeriksa 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; } 

Peringatan PVS-Studio: V3095 Objek 'remoteClient' digunakan sebelum diverifikasi dengan null. 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 melihat remoteClient dereferencing dan memeriksa nol di bawah ini. Ini benar-benar NullReferenceException potensial, karena metode FirstOrDefault dapat mengembalikan nilai default ( nol untuk tipe referensi). Saya kira, untuk memperbaiki kodenya, cukup transfer cek di atas:

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

Meskipun, mungkin ada baiknya mengganti FirstOrDefault dengan First dan menghapus centangnya sama sekali.

Dari PVS-Studio 7.05 Beta:

Saat ini, kami telah mencatat semua metode orDefault dari LINQ . Informasi ini akan digunakan oleh diagnostik baru, mencatat dereferencing hasil pemanggilan metode ini tanpa memeriksa. Untuk setiap metode orDefault ada analog yang melempar pengecualian jika elemen yang sesuai tidak ditemukan. Pengecualian ini akan membantu lebih banyak dalam memahami masalah daripada NullReferenceException abstrak.

Saya tidak bisa tidak membagikan hasil diagnosa yang dikembangkan pada proyek ini. 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); 

PVS-Studio Warning : V3080 Kemungkinan null dereference dari 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(....); } .... } 

Jadi, penganalisa mencatat dereferencing dari hasil memanggil metode CreateScope . Metode CreateScope sangat kecil, saya akan memberikannya secara keseluruhan:

 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, ini dapat mengembalikan null dalam dua kasus. Penganalisis tidak dapat mengatakan cabang mana yang akan dilalui program, oleh karena itu ia menandai kode sebagai mencurigakan. Dalam kode saya, saya akan segera menambahkan cek nol .

Mungkin saya menilai bias, tetapi semua metode asinkron harus diasuransikan terhadap NullReferenceException sebaik mungkin. Men-debug hal-hal seperti itu adalah kesenangan yang sangat meragukan.

Dalam hal ini, metode CreateScope memiliki empat panggilan, dan dalam dua ada cek, tetapi di dua lainnya hilang. Selain itu, pasangan tanpa verifikasi mirip dengan salin-tempel (satu kelas, satu metode, dereferenced untuk memanggil UsingAsync). Saya sudah melakukan panggilan pertama dari pasangan ini dan, tentu saja, panggilan kedua juga memiliki peringatan penganalisa yang serupa:

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

Peringatan PVS-Studio: V3127 Ditemukan dua fragmen kode serupa. 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); .... } 

Salin-tempel klasik. ConsumerSecret diperiksa dua kali, dan AccessTokenSecret - tidak sekali. Jelas, Anda perlu memperbaiki:

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

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

Kesalahan salin-tempel lainnya. Untuk membuatnya lebih jelas, saya akan memberikan seluruh kelas, karena itu 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 ....; } } } 

Penganalisa menganggap dua cabang kasus identik mencurigakan. Memang, ada tiga entitas di kelas, dua kembali ketika berkeliling, satu tidak. Jika ini direncanakan dan opsi ketiga ditinggalkan, maka Anda dapat menghapusnya dengan menggabungkan dua cabang sebagai berikut:

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

Jika ini adalah kesalahan salin-tempel, Anda harus memperbaiki bidang yang dikembalikan seperti ini:

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

Atau sebaliknya. Saya tidak terbiasa dengan proyek dan tidak dapat menghubungkan 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 ....; } 

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

Di blok if pertama, variabel permintaan mendapatkan nilai selain nol di beberapa tempat, tetapi di mana-mana dengan kondisi bersarang. Saya tidak mengutip semua kondisi ini, karena contohnya akan terlalu rumit. Kondisi pertama yang memeriksa jenis http dari metode IsGet atau IsPost sudah cukup . Kelas Microsoft.AspNetCore.Http.HttpMethods memiliki sembilan metode statis untuk memeriksa jenis permintaan. Ini berarti bahwa jika, misalnya, permintaan Hapus atau Setel masuk ke metode ExecuteAsync , NullReferenceException akan dilempar. Sekalipun saat ini metode seperti itu tidak didukung sama sekali - lebih baik melakukan pemeriksaan dengan pengecualian. Bagaimanapun, persyaratan untuk sistem dapat berubah. Contoh verifikasi di bawah:

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

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

Kebanyakan pemicu diagnostik V3080 lebih mudah dilihat di lingkungan pengembangan. Perlu navigasi metode, penyorotan jenis, suasana IDE yang membesarkan hati. Saya mencoba menyimpan contoh sesingkat mungkin agar Anda lebih nyaman membaca. Tetapi jika itu tidak berhasil untuk saya, atau jika Anda ingin memeriksa tingkat pemrograman Anda dan mencari tahu sendiri, saya menyarankan Anda untuk melihat bagaimana diagnostik ini bekerja pada setiap proyek terbuka atau Anda sendiri.

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

Penganalisa bersumpah pada 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); } 

Ini menyebabkan kelebihannya. Mari kita lihat dia:

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

Ternyata jika setelah menerima elemen dari Data oleh pengindeks nama kita mendapatkan entitas tipe yang tidak kompatibel dengan JObject , metode Get akan mengembalikan nol . Saya tidak akan berani menilai kemungkinan ini, karena jenis ini dari perpustakaan Newtonsoft.Json , dan saya punya sedikit pengalaman dengannya. Tetapi pengembang curiga bahwa barang yang diinginkan mungkin tidak. Jadi, jangan lupa tentang kemungkinan ini ketika merujuk pada hasil bacaan. Saya akan menambahkan pengecualian melempar ke Get pertama, jika kita menganggap bahwa harus ada node, atau periksa sebelum dereferencing, jika tidak adanya node tidak mengubah logika umum (nilai default diambil, misalnya).

Opsi satu:

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

Opsi dua:

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

Peringatan 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) { .... } .... } 

Contoh yang cukup sederhana dibandingkan dengan contoh sebelumnya. Penganalisa menganggap bahwa hasil pemanggilan QueryAsync mungkin merupakan referensi nol. Inilah metodenya:

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

Di sini GetQueryAsync adalah metode antarmuka, Anda tidak dapat memastikan setiap implementasi. Selain itu, dalam proyek yang sama ada opsi seperti itu:

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

Analisis metode GetDocumentAsync diperumit dengan banyaknya panggilan ke fungsi eksternal dan akses cache. Mari kita memikirkan fakta bahwa ada baiknya menambahkan verifikasi. Selain itu, metode ini 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 memperhatikan kualitas kode yang tinggi! Ya, ada beberapa kekurangan lain yang tidak saya sentuh dalam artikel ini, tetapi yang paling serius tetap dipertimbangkan. Tentu saja, ini tidak berarti bahwa cek setiap tiga tahun sudah cukup. Manfaat maksimum dicapai dengan penggunaan penganalisis statis secara teratur , karena pendekatan inilah yang memungkinkan Anda mendeteksi dan memperbaiki masalah pada tahap paling awal, saat biaya dan kerumitan pengeditan minimal.

Meskipun pemeriksaan satu kali tidak seefektif mungkin, saya mendorong Anda untuk mengunduh dan mencoba PVS-Studio pada proyek Anda - bagaimana jika Anda dapat menemukan sesuatu yang menarik?

Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Alexander Senichkin. Memindai kode Orchard CMS for Bugs .