0 AD adalah permainan tiga dimensi dalam genre strategi historis secara real time, yang dikembangkan oleh komunitas sukarelawan. Ukuran basis kode kecil dan saya memutuskan untuk mengecek permainan sebagai istirahat dari proyek-proyek besar seperti Android dan XNU Kernel. Jadi, sebelum kita adalah proyek yang berisi 165.000 baris kode dalam C ++. Mari kita lihat apa yang menarik di dalamnya yang dapat ditemukan menggunakan analisa statis PVS-Studio.
0 AD
0 AD (0 A.D.) adalah game tiga dimensi gratis dalam genre strategi historis secara real time, dikembangkan oleh komunitas sukarelawan (pengembang utama dipersatukan dalam tim Wildfire Games). Permainan ini memungkinkan Anda untuk mengontrol peradaban yang ada pada periode 500 SM. e. - 1 tahun SM. e. Pada musim panas 2018, proyek ini dalam versi alpha. [
Keterangan diambil dari Wikipedia].
Kenapa tepatnya 0 AD?
Saya meminta seorang rekan dari
Egor Bredikhin (
EgorBredikhin ) untuk memilih dan memeriksa saya beberapa proyek terbuka kecil yang dengannya saya dapat mengerjakan di antara tugas-tugas lain. Dia mengirimi saya log untuk proyek 0 M. Untuk pertanyaan: "Mengapa proyek ini?" - ada jawaban: "Ya, saya baru saja memainkan game ini, strategi real-time yang bagus." Ok, maka itu akan menjadi 0 AD :).
Kerapatan kesalahan
Saya ingin memuji penulis 0 AD untuk kualitas yang baik dari kode C ++. Bagus sekali, Anda jarang berhasil memenuhi kepadatan kesalahan yang begitu rendah. Tentu saja, ini bukan semua kesalahan, tetapi yang bisa dideteksi menggunakan PVS-Studio. Seperti yang saya katakan, meskipun PVS-Studio tidak menemukan semua kesalahan, namun demikian, kita dapat dengan aman berbicara tentang hubungan antara kepadatan kesalahan dan kualitas kode secara keseluruhan.
Beberapa angka. Jumlah total baris kode yang tidak kosong adalah 231270. Dari jumlah tersebut, 28,7% adalah komentar. Secara total, 165.000 baris kode C ++ murni.
Jumlah pesan yang dikeluarkan oleh penganalisa kecil, dan setelah melihat semuanya, saya menulis 19 kesalahan. Saya akan mempertimbangkan semua kesalahan ini nanti di artikel. Mungkin saya melewatkan sesuatu, menganggap kesalahan itu sebagai kode ceroboh yang tidak berbahaya. Namun, secara umum, ini tidak mengubah gambar.
Jadi, saya menemukan 19 kesalahan pada 165.000 baris kode. Kami menghitung kepadatan kesalahan: 19 * 1000/165000 = 0,115.
Untuk kesederhanaan, kami membulatkan dan menganggap bahwa penganalisa PVS-Studio mendeteksi 0,1 kesalahan per 1000 baris kode dalam kode permainan.
Hasil yang bagus! Sebagai perbandingan, dalam
artikel saya yang terbaru
tentang Android, saya menghitung bahwa saya mendeteksi setidaknya 0,25 kesalahan per 1000 baris kode. Bahkan, kepadatan kesalahan bahkan lebih besar, saya hanya tidak menemukan kekuatan untuk menganalisis seluruh laporan dengan cermat.
Atau ambil EFL Core Libraries sebagai contoh, yang saya teliti
pelajari dan hitung jumlah cacatnya. Di dalamnya, PVS-Studio mendeteksi 0,71 kesalahan per 1000 baris kode.
Jadi, penulis 0 AD adalah orang yang baik, namun, demi keadilan, harus dicatat bahwa sejumlah kecil kode yang ditulis dalam C ++ membantu mereka. Sayangnya, semakin besar proyek, semakin cepat kompleksitasnya tumbuh, dan kepadatan kesalahan meningkat secara non-linear (
lebih ).
Kesalahan
Sekarang mari kita lihat 19 kesalahan yang saya temukan di game. Untuk menganalisis proyek, saya menggunakan
PVS-Studio versi 6.24. Saya sarankan Anda mencoba mengunduh versi demo dan memeriksa proyek yang sedang Anda kerjakan.
Catatan Kami memposisikan PVS-Studio sebagai solusi B2B. Untuk proyek kecil dan pengembang individual, kami memiliki opsi lisensi gratis: "
Cara menggunakan PVS-Studio gratis ."
Kesalahan N1Mari kita mulai dengan melihat kesalahan kompleks. Sebenarnya, ini tidak rumit, tetapi Anda harus membiasakan diri dengan sepotong kode yang cukup besar.
void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb;
Peringatan PVS-Studio:
V547 CWE-570 Ekspresi 'nbNeighb> = 2' selalu salah. WaterManager.cpp 581
Sekilas, pesan penganalisa tampak aneh. Mengapa kondisi
nbNeighb> = 2 selalu salah? Memang, dalam tubuh loop ada peningkatan variabel
nbNeighb !
Lihat di bawah dan Anda akan melihat pernyataan
break yang mengganggu eksekusi loop. Oleh karena itu, jika variabel
nbNeighb ditingkatkan, siklus akan dihentikan. Dengan demikian, nilai variabel
nbNeighb tidak akan pernah mencapai nilai lebih dari 1.
Kode jelas mengandung beberapa jenis kesalahan logis.
Kesalahan N2 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... }
PVS-Studio Peringatan:
V783 CWE-119
Dereferencing iterator yang tidak valid '
segment.end ()' mungkin terjadi. CCmpRallyPointRenderer.cpp 1290
Kode yang sangat, sangat aneh. Mungkin programmer ingin menghapus elemen terakhir dari wadah. Dalam hal ini, kode harus seperti ini:
segments.erase(segments.end() - 1);
Meskipun, maka akan lebih mudah untuk menulis:
segments.pop_back();
Sejujurnya, tidak sepenuhnya jelas bagi saya apa yang seharusnya ditulis di sini.
Kesalahan N3, N4Saya memutuskan untuk mempertimbangkan dua kesalahan bersama-sama, karena terkait dengan kebocoran sumber daya dan mengharuskan terlebih dahulu untuk menunjukkan apa itu makro
WARN_RETURN .
#define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0)
Jadi, seperti yang Anda lihat, makro
WARN_RETURN menyebabkan fungsi untuk keluar dari tubuh. Sekarang mari kita lihat cara yang tidak akurat untuk menggunakan makro ini.
Fragmen pertama.
Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; }
Peringatan PVS-Studio:
V773 CWE-401 Fungsi ini keluar tanpa melepaskan pegangan 'f'. Kebocoran sumber daya mungkin terjadi. unix.cpp 332
Jika fungsi
ketakutan tidak dapat membaca data, maka fungsi
sys_generate_random_bytes akan keluar tanpa melepaskan deskriptor file. Dalam praktiknya, ini hampir tidak mungkin. Sangat diragukan bahwa Anda tidak akan dapat membaca data dari "/ dev / urandom". Namun, kode itu ceroboh.
Fragmen kedua.
Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; }
Peringatan PVS-Studio: V773 CWE-401 Fungsi ini keluar tanpa melepaskan pointer 'impl'. Kebocoran memori dimungkinkan. x.cpp 421
Jika kursor gagal dimuat, kebocoran memori terjadi.
Kesalahan N5 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... }
Peringatan PVS-Studio:
V554 CWE-762 Penggunaan shared_ptr salah. Memori yang dialokasikan dengan 'baru [] akan dibersihkan menggunakan' hapus '. MapIO.cpp 54
Opsi yang benar:
shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);
Kesalahan N6 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; }
PVS-Studio Warning:
V570 Variabel 'ptr' ditugaskan untuk dirinya sendiri. FUTracker.h 122
Kesalahan N7, N8 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; }
Peringatan PVS-Studio:
V576 CWE-628 Format salah. Pertimbangkan untuk memeriksa argumen aktual kelima dari fungsi 'swprintf_s'. Argumen tipe char diharapkan. trace.cpp 93
Di sini kita menemukan sejarah yang bingung dan tidak jelas implementasi alternatif dari fungsi
swprintf dalam Visual C ++. Saya tidak akan
menceritakannya kembali , tetapi merujuk pada dokumentasi untuk diagnostik
V576 (lihat bagian "Garis lebar").
Dalam hal ini, kemungkinan besar, kode ini akan berfungsi dengan benar ketika dikompilasi dalam Visual C ++ untuk Windows dan salah ketika membangun untuk Linux atau macOS.
Kesalahan serupa: V576 CWE-628 Format salah. Pertimbangkan untuk memeriksa argumen aktual keempat dari fungsi 'swprintf_s'. Argumen tipe char diharapkan. vfs_tree.cpp 211
Kesalahan N9, N10, N11Klasik Pada awalnya, pointer digunakan, dan baru dicentang.
static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val);
Peringatan PVS-Studio:
V595 CWE-476 Pointer 'dst' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 140, 143. test_secure_crt.h 140
Saya pikir kesalahan tidak memerlukan penjelasan. Peringatan serupa:
- V595 CWE-476 Pointer 'dst' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 150, 153. test_secure_crt.h 150
- V595 CWE-476 Pointer 'dst' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 314, 317. test_secure_crt.h 314
Kesalahan N12 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... }
V674 CWE-682 '0,5' literal dari tipe 'ganda' dibandingkan dengan nilai dari tipe 'int'. Pertimbangkan untuk memeriksa ekspresi 'bIsOrientationPreservasi> 0,5'. MikktspaceWrap.cpp 137
Tidak masuk akal untuk membandingkan variabel tipe
int dengan konstanta 0,5. Selain itu, dari segi makna, ini umumnya merupakan variabel dari tipe boolean, yang berarti membandingkannya dengan 0,5 terlihat sangat aneh. Asumsikan bahwa alih-alih
bIsOrientationPreserving variabel lain harus digunakan di sini.
Kesalahan N13 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE);
Peringatan PVS-Studio:
V749 CWE-675 Destructor dari objek 's' akan dipanggil untuk kedua kalinya setelah meninggalkan ruang lingkup objek. vfs.cpp 165
Sebelum membuat file, diperlukan objek bertipe
ScopedLock "membuka" objek. Untuk ini, sebuah destructor secara eksplisit disebut. Masalahnya adalah destruktor untuk objek
s akan dipanggil secara otomatis lagi ketika fungsi keluar. Yaitu destructor akan dipanggil dua kali. Saya belum mempelajari perangkat kelas
ScopedLock , tetapi dalam hal apa pun, Anda sebaiknya tidak melakukan ini. Seringkali, panggilan ganda kepada destruktor menyebabkan perilaku yang tidak terdefinisi atau kesalahan tidak menyenangkan lainnya. Bahkan jika kodenya berfungsi dengan baik sekarang, sangat mudah untuk memecahkan semuanya dengan mengubah implementasi kelas
ScopedLock .
Kesalahan N14, N15, N16, N17 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... }
Peringatan PVS-Studio:
V668 CWE-570 Tidak ada gunanya menguji pointer 'pEvent' terhadap null, karena memori dialokasikan menggunakan operator 'baru'. Pengecualian akan dihasilkan jika terjadi kesalahan alokasi memori. fsm.cpp 259
Memeriksa pointer tidak masuk akal, karena jika terjadi kesalahan alokasi memori, pengecualian
std :: bad_alloc akan
dibuang .
Jadi, pemeriksaannya berlebihan, tetapi kesalahannya tidak serius. Namun, semuanya jauh lebih buruk ketika beberapa logika dieksekusi di tubuh
pernyataan if . Pertimbangkan kasus berikut:
CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL;
Peringatan Analyzer: V668 CWE-570 Tidak ada gunanya menguji pointer 'pNewTransition' terhadap nol, karena memori dialokasikan menggunakan operator 'baru'. Pengecualian akan dihasilkan jika terjadi kesalahan alokasi memori. fsm.cpp 289
Suatu upaya dilakukan untuk membebaskan memori yang alamatnya disimpan dalam pointer
pEvent . Secara alami, ini tidak akan terjadi dan kebocoran memori akan terjadi.
Bahkan, ketika saya mulai berurusan dengan kode ini, ternyata semuanya lebih rumit dan mungkin tidak ada satu kesalahan, tetapi dua. Sekarang saya akan menjelaskan apa yang salah dengan kode ini. Untuk melakukan ini, kita perlu membiasakan diri dengan perangkat fungsi
AddEvent .
CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL;
Perhatikan bahwa fungsi tidak selalu mengembalikan pointer ke objek baru yang dibuat menggunakan operator
baru . Terkadang mengambil objek yang sudah ada dari wadah
m_Events . Dan penunjuk ke objek yang baru dibuat, omong-omong, juga akan ditempatkan di
m_Events .
Dan di sini muncul pertanyaan: siapa yang memiliki dan harus menghancurkan benda-benda yang petunjuknya disimpan dalam wadah
m_Events ? Saya tidak terbiasa dengan proyek, tetapi kemungkinan besar di suatu tempat ada kode yang menghancurkan semua objek ini. Kemudian menghapus objek di dalam fungsi
CFsm :: AddTransition umumnya berlebihan.
Saya mendapat kesan bahwa Anda cukup menghapus fragmen kode berikut:
if ( !pNewTransition ) { delete pEvent; return NULL; }
Kesalahan lain:
- V668 CWE-571 Tidak ada gunanya menguji pointer 'ret' terhadap null, karena memori dialokasikan menggunakan operator 'baru'. Pengecualian akan dihasilkan jika terjadi kesalahan alokasi memori. TerrainTextureEntry.cpp 120
- V668 CWE-571 Tidak ada gunanya menguji pointer 'answer' terhadap null, karena memori dialokasikan menggunakan operator 'baru'. Pengecualian akan dihasilkan jika terjadi kesalahan alokasi memori. SoundManager.cpp 542
Kesalahan N18, N19 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) {
Peringatan PVS-Studio:
V701 CWE-401 realloc () kemungkinan kebocoran: ketika realloc () gagal mengalokasikan memori, pointer asli 'dsd-> entri' hilang. Pertimbangkan untuk menetapkan realloc () ke pointer sementara. mongoose.cpp 2462
Jika ukuran array menjadi tidak mencukupi, maka memori dialokasikan menggunakan fungsi
realloc . Kesalahannya adalah bahwa nilai pointer ke blok memori asli segera ditimpa dengan nilai baru yang dikembalikan oleh fungsi
realokasi .
Jika alokasi memori gagal, fungsi
realloc akan mengembalikan NULL, dan NULL ini akan ditulis ke variabel
entri dsd-> . Setelah itu tidak mungkin untuk membebaskan blok memori yang alamatnya sebelumnya disimpan dalam
entri dsd-> . Kebocoran memori akan terjadi.
Kesalahan lain: V701 CWE-401 realloc () kemungkinan kebocoran: ketika realloc () gagal mengalokasikan memori, penunjuk asli 'Buffer' hilang. Pertimbangkan untuk menetapkan realloc () ke pointer sementara. Preprocessor.cpp 84
Kesimpulan
Saya tidak bisa mengatakan bahwa kali ini artikelnya ternyata menarik, atau saya berhasil menunjukkan banyak kesalahan besar. Apa yang harus dilakukan, sekali waktu tidak perlu.
Apa yang saya lihat, kemudian saya menulis .
Terima kasih atas perhatian anda Untuk perubahan, saya akan mengakhiri artikel dengan undangan untuk mengikuti saya di Instagram
@pvs_studio_unicorn dan di Twitter
@Code_Analysis .
Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Andrey Karpov.
Kerja bagus, penulis game 0 AD!