Mainkan "osu!", Jangan lupa tentang kesalahan

Kami menyambut semua pecinta kode eksotis dan tidak terlalu kesalahan. Hari ini di bangku tes PVS-Studio adalah tamu yang cukup langka - permainan dalam C #. Yaitu, osu! Seperti biasa: cari kesalahan, pikirkan, mainkan.

Permainan

Osu! - Sebuah permainan irama musik open source. Dilihat oleh informasi dari situs web game , itu cukup populer, karena lebih dari 15 juta pemain terdaftar diklaim. Proyek ini ditandai oleh permainan gratis, desain penuh warna dengan kemampuan untuk menyesuaikan kartu, fitur-fitur canggih untuk menyusun peringkat pemain online, kehadiran multi pemain, serangkaian komposisi musik yang besar. Saya tidak akan menjelaskan game secara detail, mereka yang tertarik akan dengan mudah menemukan semua informasi di jaringan. Misalnya di sini .

Saya lebih tertarik pada kode sumber proyek, yang tersedia untuk diunduh dari GitHub . Sejumlah besar komitmen (lebih dari 24 ribu) ke repositori segera menarik perhatian, yang menunjukkan perkembangan aktif proyek, yang berlanjut hingga hari ini (permainan dirilis pada 2007, tetapi pekerjaan itu mungkin dimulai sebelumnya). Pada saat yang sama, tidak ada banyak kode sumber - 1813 file .cs yang berisi 135 ribu baris kode, tidak termasuk yang kosong. Kode ini berisi tes yang biasanya tidak saya perhitungkan dalam cek. Kode uji terkandung dalam 306 file .cs dan, karenanya, 25 ribu baris kode, tidak termasuk yang kosong. Ini adalah proyek kecil: untuk perbandingan, inti C # dari analisa PVS-Studio berisi sekitar 300 ribu baris kode.

Secara total, untuk memeriksa kesalahan pada game, saya menggunakan proyek non-tes yang mengandung 1507 file kode sumber dan 110 ribu baris. Namun, hasilnya sebagian menyenangkan saya, karena ada beberapa kesalahan menarik yang saya buru-buru ceritakan.

Kesalahan

V3001 Ada hasil sub-ekspresi yang identik '== HitResult.Perfect' ke kiri dan ke kanan '||' operator. DrawableHoldNote.cs 266

protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); } 

Contoh yang bagus dari pemrograman berorientasi copy-paste. Istilah komik yang baru-baru ini digunakan oleh kolega saya Valery Komarov (diperkenalkan) dalam artikelnya " 10 Kesalahan Teratas di Proyek Java untuk 2019 ".

Jadi, dua pemeriksaan identik mengikuti satu demi satu. Salah satu cek kemungkinan besar berisi konstanta enumerasi HitResult lainnya:

 public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, } 

Konstanta khusus mana yang perlu digunakan, atau apakah pemeriksaan kedua tidak diperlukan sama sekali? Pertanyaan yang hanya bisa dijawab pengembang. Bagaimanapun, kesalahan telah dibuat yang mendistorsi logika program.

V3001 Ada sub-ekspresi identik 'family! = GetFamilyString (TournamentTypeface.Aquatico)' di sebelah kiri dan di sebelah kanan operator '&&'. TournamentFont.cs 64

 public static string GetWeightString(string family, FontWeight weight) { .... if (weight == FontWeight.Regular && family != GetFamilyString(TournamentTypeface.Aquatico) && family != GetFamilyString(TournamentTypeface.Aquatico)) weightString = string.Empty; .... } 

Dan lagi copy-paste. Saya memformat kode, sehingga kesalahan mudah terlihat. Dalam versi aslinya, seluruh kondisi ditulis dalam satu baris. Sulit juga mengatakan bagaimana kode dapat diperbaiki. Enumerasi TournamentTypeface berisi konstanta tunggal:

 public enum TournamentTypeface { Aquatico } 

Kondisi ini mungkin telah menggunakan variabel keluarga dua kali secara tidak sengaja, tetapi ini tidak akurat.

V3009 [CWE-393] Aneh bahwa metode ini selalu mengembalikan satu dan nilai 'false' yang sama. KeyCounterAction.cs 19

 public bool OnPressed(T action, bool forwards) { if (!EqualityComparer<T>.Default.Equals(action, Action)) return false; IsLit = true; if (forwards) Increment(); return false; } 

Metode ini akan selalu mengembalikan false . Untuk kesalahan seperti itu, saya biasanya memeriksa kode panggilan, karena mereka sering tidak menggunakan nilai pengembalian di mana saja, maka tidak ada kesalahan (kecuali untuk gaya pemrograman yang jelek). Dalam hal ini, saya menemukan kode berikut:

 public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0)); 

Seperti yang Anda lihat, hasil yang dikembalikan oleh metode OnPressed digunakan. Dan karena selalu salah , hasil pemanggilan OnPressed juga akan selalu salah . Saya pikir Anda harus memeriksa ulang kode ini lagi, karena ada kemungkinan kesalahan tinggi.

Kesalahan serupa lainnya:

• V3009 [CWE-393] Aneh bahwa metode ini selalu mengembalikan satu dan nilai 'false' yang sama. KeyCounterAction.cs 30

V3042 [CWE-476] Kemungkinan NullReferenceException. '?.' dan '.' operator digunakan untuk mengakses anggota objek 'val.NewValue' TournamentTeam.cs 41

 public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2 // <= ? val.NewValue?.Substring(0, 2).ToUpper() : string.Empty; }; .... } 

Dalam kondisi ?: Operator, variabel val.NewValue tidak aman. Penganalisa membuat kesimpulan seperti itu, sejak saat itu di cabang kemudian, opsi aman digunakan untuk mengakses variabel melalui pernyataan akses bersyarat val.NewValue?. Substring (....) .

Kesalahan serupa lainnya:

• V3042 [CWE-476] Kemungkinan NullReferenceException. '?.' dan '.' operator digunakan untuk mengakses anggota objek 'val.NewValue' TournamentTeam.cs 48

V3042 [CWE-476] Kemungkinan NullReferenceException. '?.' dan '.' operator digunakan untuk mengakses anggota objek 'api' SetupScreen.cs 77

 private void reload() { .... new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout(); // <= .... }, Value = api?.LocalUser.Value.Username, .... }, .... } private class ActionableInfo : LabelledDrawable<Drawable> { .... public Action Action; .... } 

Kode ini kurang jelas, tetapi saya pikir kesalahannya masih ada. Buat objek bertipe ActionableInfo . Bidang Tindakan diinisialisasi dengan lambda, di badan yang tidak aman untuk bekerja dengan api referensi yang berpotensi nol. Penganalisa menganggap pola ini sebagai kesalahan, sejak saat itu, ketika menginisialisasi parameter Nilai , variabel api bekerja dengan aman. Saya menyebut kesalahan ambigu, karena kode lambda mengasumsikan eksekusi tertunda dan kemudian, mungkin, pengembang entah bagaimana menjamin nilai non-nol dari tautan api . Tapi ini hanya asumsi, karena tubuh lambda tidak mengandung tanda-tanda pekerjaan yang aman dengan tautan (misalnya, pemeriksaan pendahuluan).

V3066 [CWE-683] Kemungkinan urutan argumen yang salah diteruskan ke metode 'Atan2': 'diff.X' dan 'diff.Y'. SliderBall.cs 182

 public void UpdateProgress(double completionProgress) { .... Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .... } 

Penganalisa menduga bahwa ketika bekerja dengan metode Atan2 dari kelas Matematika , pengembang mencampur urutan argumen. Atan2 Ad :

 // Parameters: // y: // The y coordinate of a point. // // x: // The x coordinate of a point. public static double Atan2(double y, double x); 

Seperti yang Anda lihat, nilai-nilai ditransfer dalam urutan terbalik. Saya tidak dapat menilai apakah ini kesalahan, karena metode UpdateProgress berisi banyak perhitungan nontrivial. Saya hanya mencatat fakta kemungkinan masalah dalam kode.

V3080 [CWE-476] Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'Beatmap'. WorkingBeatmap.cs 57

 protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... } 

Penganalisa menunjukkan bahaya akses melalui tautan null Beatmap . Ini dia apa adanya:

 public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } } 

Nah, alat analisa itu benar.

Untuk informasi lebih lanjut tentang bagaimana PVS-Studio menemukan kesalahan seperti itu, serta tentang inovasi C # 8.0 yang terkait dengan topik yang serupa (bekerja dengan referensi yang berpotensi nol), lihat artikel " Jenis Referensi yang Tidak Dapat Diperbolehkan dalam C # 8.0 dan Analisis Statis ".

V3083 [CWE-367] Doa yang tidak aman dari acara 'ObjectConverted', NullReferenceException dimungkinkan. Pertimbangkan menugaskan acara ke variabel lokal sebelum menjalankannya. BeatmapConverter.cs 82

 private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... } 

Kesalahan yang tidak kritis dan sangat umum. Antara memeriksa acara untuk null dan permintaannya, mereka dapat berhenti berlangganan dari acara tersebut, yang akan menyebabkan program macet.
Salah satu perbaikannya:

 private List<T> convertHitObjects(....) { .... converted = converted.ToList(); ObjectConverted?.Invoke(obj, converted); .... } 

V3095 [CWE-476] Objek 'kolom' digunakan sebelum diverifikasi terhadap nol. Periksa baris: 141, 142. SquareGraph.cs 141

 private void redrawProgress() { for (int i = 0; i < ColumnCount; i++) columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed; columns?.ForceRedraw(); } 

Tidak aman untuk mem-bypass koleksi kolom dalam satu lingkaran. Pada saat yang sama, pengembang mengasumsikan bahwa tautan kolom mungkin nol, karena nanti dalam kode, operator akses bersyarat digunakan untuk mengakses koleksi.

V3119 Memanggil acara yang diganti 'OnNewResult' dapat menyebabkan perilaku yang tidak dapat diprediksi. Pertimbangkan untuk menerapkan pengakses acara secara eksplisit atau gunakan kata kunci 'tersegel'. DrawableRuleset.cs 256

 private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult; 

Penganalisa memperingatkan bahaya menggunakan acara yang ditimpa atau virtual. Apa sebenarnya bahayanya - saya sarankan membaca dalam deskripsi untuk diagnosis. Juga, pada suatu waktu saya menulis artikel tentang topik ini, " Acara Virtual di C #: Ada yang salah ."

Konstruk serupa lainnya yang tidak aman dalam kode:

• V3119 Memanggil acara yang diganti dapat menyebabkan perilaku yang tidak terduga. Pertimbangkan untuk menerapkan pengakses acara secara eksplisit atau gunakan kata kunci 'tersegel'. DrawableRuleset.cs 257

V3123 [CWE-783] Mungkin '??' Operator bekerja dengan cara yang berbeda dari yang diharapkan. Prioritasnya lebih rendah daripada prioritas operator lain di bagian kirinya. OsuScreenStack.cs 45

 private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } 

Untuk pemahaman yang lebih baik tentang masalah ini - saya akan memberikan contoh sintetis tentang bagaimana kode bekerja sekarang:

 x = (c * a) ?? b; 

Kesalahan dibuat karena fakta bahwa * operator memiliki prioritas lebih tinggi daripada operator. Versi kode yang diperbaiki (tanda kurung ditambahkan):

 private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); } 

Kesalahan serupa lainnya dalam kode:

V3123 [CWE-783] Mungkin '??' Operator bekerja dengan cara yang berbeda dari yang diharapkan. Prioritasnya lebih rendah daripada prioritas operator lain di bagian kirinya. FramedReplayInputHandler.cs 103

 private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } } 

Di sini, seperti pada fragmen kode sebelumnya, prioritas operator tidak diperhitungkan. Sekarang ekspresi yang diteruskan ke metode Math.Abs dievaluasi sebagai berikut:

 (a – b) ?? 0 

Kode yang diperbaiki:

 private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } } 

V3142 [CWE-561] Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. DrawableHoldNote.cs 214

 public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss) // <= holdNote.hasBroken = true; .... } 

Penganalisa mengklaim bahwa kode handler OnPressed , dimulai dengan pernyataan if kedua, tidak dapat dijangkau. Ini mengikuti dari asumsi bahwa kondisi pertama selalu benar, yaitu basis. Metode yang ditekan akan selalu kembali salah . Lihatlah basisnya. Metode yang ditekan :

 public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); } 

Kami melanjutkan ke metode UpdateResult :

 protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; } 

Perhatikan bahwa implementasi properti yang dinilai tidak penting di sini, karena logika metode UpdateResult menyiratkan bahwa pernyataan pengembalian terakhir setara dengan ini:

 return false; 

Dengan demikian, metode UpdateResult akan selalu mengembalikan false , yang akan menyebabkan kesalahan dengan kode yang tidak terjangkau dalam kode di atas tumpukan.

V3146 [CWE-476] Kemungkinan null dereference dari 'ruleset'. 'FirstOrDefault' dapat mengembalikan nilai nol default. APILegacyScoreInfo.cs 24

 public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance() // <= .GetAllMods().Where(....) .ToArray() : Array.Empty<Mod>(); .... } 

Penganalisa menganggap panggilan ruleset.CreateInstance () tidak aman. Variabel ruleset sebelumnya mendapatkan nilai sebagai hasil dari memanggil GetRuleset :

 public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....); 

Seperti yang Anda lihat, peringatan penganalisa dibenarkan, karena rantai panggilan berisi FirstOrDefault , yang dapat mengembalikan nol .

Kesimpulan

Secara umum, proyek game "osu!" Senang dengan sejumlah kecil kesalahan. Namun demikian, saya merekomendasikan pengembang untuk memperhatikan masalah yang terdeteksi. Dan biarkan game terus menyenangkan para penggemarnya.

Dan bagi mereka yang suka menggali kode, saya mengingatkan Anda bahwa penganalisa PVS-Studio, yang mudah diunduh dari situs resmi, akan sangat membantu. Saya juga mencatat bahwa pemeriksaan proyek satu kali, seperti yang dijelaskan di atas, tidak ada hubungannya dengan menggunakan penganalisa statis dalam pekerjaan nyata. Efisiensi maksimum dalam memerangi kesalahan hanya dapat dicapai dengan menggunakan alat secara rutin baik pada server build maupun langsung di komputer pengembang (analisis tambahan). Tujuan maksimum adalah untuk mencegah kesalahan memasuki sistem kontrol versi, memperbaiki kerusakan sudah pada tahap penulisan kode.

Semoga sukses dan sukses!

Referensi

Ini adalah publikasi pertama pada tahun 2020. Mengambil kesempatan ini, saya akan memberikan tautan ke artikel tentang memeriksa proyek C # tahun lalu:

• Mencari bug di Amazon Web Services SDK untuk kode sumber .NET
• Memeriksa kode sumber Roslyn
• Jenis Referensi Nullable dalam C # 8.0 dan Analisis Statis
• WinForms: Kesalahan, Holmes
• Kisah bagaimana PVS-Studio menemukan kesalahan di perpustakaan yang digunakan di ... PVS-Studio
• Verifikasi kode sumber pustaka .NET Core oleh penganalisa statis PVS-Studio
• Memeriksa analis Roslyn
• Periksa Telerik UI untuk UWP untuk berkenalan dengan PVS-Studio
• Azure PowerShell: "kebanyakan tidak berbahaya"
• Kami mencari dan menganalisis kesalahan dalam kode CMS Orchard
• Memeriksa bungkus OpenCvSharp melalui OpenCV dengan PVS-Studio
• Azure SDK untuk .NET: Kisah Pencari Bug yang Sulit
• SARIF SDK dan kesalahannya
• 10 bug teratas dalam proyek C # untuk 2019

Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Sergey Khrenov. Mainkan "osu!", But Watch Out for Bugs .