Hai, kalian semua pengoleksi serangga eksotis dan polos! Kami punya spesimen langka di bangku tes PVS-Studio kami hari ini - permainan yang disebut "osu!", Ditulis dalam C #. Seperti biasa, kami akan mencari bug, menganalisisnya, dan bermain.
Permainan
Osu! adalah game rhythm open-source. Menurut
situs web gim, ini cukup populer, dengan lebih dari 15 juta akun pemain. Proyek ini menampilkan permainan gratis, desain penuh warna, kustomisasi peta, sistem peringkat pemain online tingkat lanjut, mode multi-pemain, dan sekumpulan karya musik yang kaya. Tidak ada gunanya menjabarkan lebih lanjut tentang game; Anda dapat membaca semua tentang itu di Internet. Mulai dengan
halaman ini .
Saya lebih tertarik pada kode sumber proyek, yang tersedia di
GitHub . Satu hal yang segera menarik perhatian Anda adalah banyaknya jumlah tempat penyimpanan (lebih dari 24 ribu), yang merupakan pertanda perkembangan yang intens dan berkelanjutan (permainan ini pertama kali dirilis pada 2007, tetapi pekerjaan itu harus sudah dimulai lebih awal). Proyek ini tidak besar: hanya 1813 file .cs dengan total 135 ribu LOC tidak kosong. Jumlah ini juga termasuk tes, yang biasanya tidak saya perhitungkan saat menjalankan cek. Tes terdiri dari 306 file .cs dengan 25 ribu LOC. Proyek ini memang kecil: misalnya, inti C # dari PVS-Studio adalah sekitar 300 ribu LOC.
Menyisakan file uji, saya memeriksa 1507 file, 110 ribu LOC. Cek itu mengungkapkan beberapa bug menarik, yang saya ingin tunjukkan kepada Anda.
Bug
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; .... }); }
Ini adalah contoh yang bagus dari pemrograman berorientasi copy-paste, yang merupakan istilah lucu yang baru-baru ini digunakan oleh rekan kerja saya Valeriy Komarov dalam artikelnya "
Top 10 Bug yang Ditemukan di Proyek Java pada 2019 ".
Bagaimanapun, dua pemeriksaan identik dieksekusi berturut-turut. Salah satunya mungkin dimaksudkan untuk memeriksa beberapa konstanta lain dari penghitungan
HitResult :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
Konstanta mana yang dimaksudkan untuk diperiksa? Atau mungkin cek kedua tidak seharusnya ada di sana? Ini adalah pertanyaan yang hanya bisa dijawab oleh penulis. Bagaimanapun, ini adalah kesalahan yang mengubah logika eksekusi 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; .... }
Copy-paste lagi. Saya refactored kodenya sehingga kesalahan mudah dilihat sekarang tetapi awalnya telah ditulis dalam satu baris. Sama seperti pada contoh sebelumnya, saya tidak bisa mengatakan dengan pasti bagaimana tepatnya yang ini harus diperbaiki.
Enumerasi TournamentTypeface hanya berisi satu konstanta:
public enum TournamentTypeface { Aquatico }
Mungkin kesalahannya adalah tentang memeriksa variabel
keluarga dua kali, tetapi saya mungkin salah.
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 mengembalikan
false setiap kali. Dalam kasus seperti ini, saya biasanya akan memeriksa pemanggilan fungsi, karena Anda mungkin sering menemukan bahwa pemanggil tidak menggunakan nilai balik, yang berarti tidak ada masalah (selain gaya buruk). Seperti inilah panggilan dalam kasus ini:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Seperti yang Anda lihat, pemanggil tidak menggunakan nilai yang dikembalikan oleh metode
OnPressed . Karena nilai itu selalu
salah , penelepon itu sendiri selalu mengembalikan
false juga. Kode ini sangat mungkin mengandung kesalahan dan harus direvisi.
Bug 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
Variabel
val.NewValue ditangani dengan cara yang berbahaya dalam kondisi
?: Operator. Apa yang membuat penganalisa berpikir demikian adalah kenyataan bahwa kemudian di cabang
kemudian , variabel yang sama ditangani dengan cara yang aman menggunakan operator akses bersyarat:
val.NewValue? .Substring (....) .
Bug 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();
Yang ini lebih ambigu, tapi saya percaya itu bug juga. Programmer menciptakan objek bertipe
ActionableInfo . Bidang
Tindakan diinisialisasi dengan menggunakan fungsi lambda, yang menangani
api referensi yang berpotensi nol dengan cara yang berbahaya. Penganalisa menganggap pola ini sebagai kesalahan karena variabel
api ditangani dengan cara yang aman nanti, ketika menginisialisasi parameter
Nilai . Saya menyebut kasus ini ambigu karena kode pada fungsi lambda menyiratkan eksekusi yang tertunda, pada saat itu pengembang mungkin dapat menjamin bahwa referensi
api tidak akan nol. Tapi saya tidak yakin tentang itu karena tubuh fungsi lambda tampaknya tidak menggunakan penanganan referensi yang aman seperti cek sebelumnya.
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 argumen metode
Atan2 dilewatkan dalam urutan yang salah. Ini adalah deklarasi metode:
Nilai diteruskan dalam urutan terbalik. Saya tidak yakin apakah ini bug karena metode
UpdateProgress mengandung cukup banyak perhitungan nontrivial; Saya hanya menyebut itu sebagai bug yang mungkin.
V3080 [CWE-476] Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'Beatmap'. WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
Penganalisa menunjukkan kemungkinan null dereference dari
Beatmap :
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Nah, alat analisa itu benar.
Untuk mempelajari lebih lanjut tentang bagaimana PVS-Studio mendeteksi bug seperti ini, dan tentang fitur baru yang ditambahkan dalam C # 8.0 yang ada hubungannya dengan penanganan referensi yang berpotensi nol, lihat artikel "
Jenis referensi yang dapat dihapus 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); } .... }
Ini adalah kesalahan kecil dan cukup umum. Pelanggan dapat berhenti berlangganan dari acara antara cek nol dan permintaan acara, yang mengakibatkan crash. Ini adalah salah satu cara untuk memperbaiki bug:
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(); }
Iterasi atas koleksi
kolom dilakukan dengan cara yang berbahaya. Pengembang mengasumsikan bahwa referensi
kolom bisa nol, yang ditunjukkan dengan penggunaan operator akses bersyarat untuk mengakses koleksi lebih lanjut dalam kode.
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 mengatakan berbahaya untuk menggunakan acara yang ditimpa atau virtual. Lihat
deskripsi diagnostik untuk penjelasan. Saya juga menulis artikel tentang topik ini: "
Peristiwa virtual di C #: ada yang salah ".
Berikut ini konstruksi serupa lainnya yang tidak aman:
- 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, berikut adalah contoh sintetis yang menunjukkan logika asli kode ini:
x = (c * a) ?? b;
Bug berasal dari fakta bahwa prioritas operator "*" lebih tinggi daripada "??" operator. Ini adalah kode tetap yang seharusnya (dengan tanda kurung ditambahkan):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Bug serupa lainnya:
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; } }
Seperti dalam kasus sebelumnya, programmer memiliki asumsi yang salah tentang prioritas operator. Ekspresi asli yang diteruskan ke metode
Math.Abs mengevaluasi sebagai berikut:
(a – b) ?? 0
Inilah cara memperbaikinya:
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)
Penganalisa percaya kode penangan
OnPressed tidak dapat dijangkau dimulai dengan pernyataan
if kedua. Ini mengikuti dari fakta bahwa kondisi pertama selalu benar, yaitu bahwa metode
base.OnPressed akan selalu mengembalikan
false . Mari kita lihat
basisnya. Metode yang
ditekan :
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Dan sekarang di 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 masalah di sini karena logika metode
UpdateResult menyiratkan bahwa pernyataan
pengembalian terakhir setara dengan yang berikut ini:
return false;
Ini berarti metode
UpdateResult akan mengembalikan
false setiap saat, sehingga mengarah ke masalah kode yang tidak dapat dijangkau sebelumnya di stack.
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()
Penganalisa percaya panggilan
ruleset.CreateInstance () menjadi tidak aman. Sebelum panggilan ini, variabel
ruleset diberi nilai sebagai hasil memanggil metode
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Seperti yang Anda lihat, peringatan itu valid karena urutan panggilan menyertakan metode
FirstOrDefault , yang dapat mengembalikan
nol .
Kesimpulan
Tidak ada banyak bug dalam kode "osu!", Dan itu bagus. Tetapi saya masih merekomendasikan agar penulis memeriksa masalah yang dilaporkan oleh penganalisa. Saya harap ini akan membantu mempertahankan kualitas tinggi dan permainan akan terus membawa sukacita bagi para pemain.
Sebagai pengingat, PVS-Studio adalah pilihan yang baik jika Anda suka mengutak-atik kode sumber. Alat analisis tersedia untuk
diunduh di situs web resmi. Hal lain yang saya ingin Anda ingat adalah bahwa pemeriksaan satu kali seperti ini tidak ada hubungannya dengan penggunaan normal analisis statis dalam proses pengembangan nyata. Ini paling efektif hanya jika digunakan secara teratur baik pada server build maupun pada komputer pengembang (ini disebut analisis tambahan). Tujuan utama Anda adalah mencegah bug masuk ke sistem kontrol versi dengan menangkapnya pada tahap pengkodean.
Semoga berhasil, dan tetap kreatif!
Referensi
Ini adalah artikel pertama kami di tahun 2020. Sementara kami membahasnya, berikut adalah tautan ke cek proyek C # yang dilakukan selama setahun terakhir: