VVVVVV ??? VVVVVV !!! :)

Jika Anda membaca teks ini, itu berarti Anda berpikir ada sesuatu yang salah dengan judul artikel tersebut, atau Anda melihat nama permainan komputer yang akrab di dalamnya. VVVVVV adalah game platformer indie yang telah memenangkan hati banyak pemain dengan kesederhanaan eksternal yang menyenangkan dan kompleksitas internal yang sama-sama menyenangkan. Beberapa hari yang lalu VVVVVV berusia 10 tahun, dan penulis permainan - Terry Cavanagh - merayakan liburan ini dengan mempublikasikan kode sumbernya. Apa yang "enak" dapat ditemukan di dalamnya? Baca jawabannya di artikel ini.

Pendahuluan

Oh, VVVVVVV ... Saya ingat bagaimana saya menemukannya sesaat setelah rilis, dan, sebagai penggemar berat game retro pixel, saya dengan senang hati memasangnya di komputer saya. Saya ingat kesan pertama saya: “Jadi, apakah hanya itu? Lari saja di sekitar ruangan persegi? ”Saya berpikir setelah beberapa menit pertandingan. Saya belum tahu apa yang menunggu saya. Segera setelah saya meninggalkan lokasi awal, saya mendapati diri saya berada di dunia dua dimensi yang kecil namun membingungkan dan penuh hiasan lanskap dan artefak piksel yang tidak diketahui oleh saya.

Game ini menyeretku. Meskipun kompleksitasnya tinggi, dengan terampil dikalahkan oleh kontrol yang tidak biasa pada waktu itu (karakter utama tidak tahu cara melompat, tetapi mampu membalikkan arah vektor gravitasi untuk saya sendiri), saya benar-benar melewatinya. Saya tidak tahu berapa kali karakter saya mati, tetapi saya yakin bahwa jumlah kematian diukur dalam puluhan ratusan. Meski begitu, ada beberapa pesona unik dalam game ini :)

Mari kita kembali ke kode sumber yang ditata untuk menghormati hari jadi permainan .

Saat ini, saya C ++ - pengembang di tim PVS-Studio - penganalisa kode statis untuk C, C ++, C # dan Java. Selain pengembangan itu sendiri, kami juga terlibat dalam promosi produk kami. Bagi kami, salah satu cara terbaik untuk melakukan ini adalah menulis artikel tentang memeriksa proyek sumber terbuka. Pembaca kami menerima artikel menarik tentang topik pemrograman, dan kami mendapat kesempatan untuk menunjukkan dengan jelas kemampuan PVS-Studio. Karena itu, ketika saya mengetahui tentang pembukaan kode sumber VVVVVV, saya tidak bisa melewatinya.

Pada artikel ini kita akan melihat beberapa kesalahan menarik yang ditemukan oleh penganalisa PVS-Studio dalam kode VVVVVV, serta melakukan analisis terperinci atas kesalahan ini. Kembalikan vektor gravitasi ke posisi "turun" dan duduk: kita mulai!

Ikhtisar peringatan yang dikeluarkan oleh penganalisa

Peringatan 1

V512 Panggilan fungsi 'sprintf' akan menyebabkan meluapnya buffer 'fileSearch'. FileSystemUtils.cpp 307

#define MAX_PATH 260 .... void PLATFORM_migrateSaveData(char *output) { char oldLocation[MAX_PATH]; char newLocation[MAX_PATH]; char oldDirectory[MAX_PATH]; char fileSearch[MAX_PATH]; .... /* Same place, different layout. */ strcpy(oldDirectory, output); sprintf(fileSearch, "%s\\*.vvvvvv", oldDirectory); .... } 

Seperti yang Anda lihat, baris pencarian File dan oldDirectory berukuran sama: 260 karakter. String pemformatan (argumen ketiga ke sprintf ) setelah mengganti konten string oldDirectory ke dalamnya akan terlihat seperti ini:

 <i>_oldDirectory\*.vvvvvv</i> 

String ini lebih panjang 9 karakter dari pada direktori old original. Urutan karakter inilah yang akan ditulis lebih lanjut ke fileSearch . Apa yang terjadi jika string oldDirectory lebih panjang dari 251? String yang dihasilkan akan lebih panjang daripada yang bisa ditampung oleh fileSearch , yang akan mengarah pada penulisan di luar array. Jenis data apa dalam RAM yang mungkin rusak dan apa akibatnya adalah pertanyaan retoris :)

Peringatan 2

V519 Variabel 'latar belakang' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 1367, 1373. Map.cpp 1373

 void mapclass::loadlevel(....) { .... case 4: //The Warpzone tmap = warplevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); roomname = warplevel.roomname; tileset = 1; background = 3; // <= dwgfx.rcol = warplevel.rcol; dwgfx.backgrounddrawn = false; warpx = warplevel.warpx; warpy = warplevel.warpy; background = 5; // <= if (warpy) background = 4; if (warpx) background = 3; if (warpx && warpy) background = 5; break; .... } 

Satu dan variabel yang sama diberi nilai dua kali berturut-turut. Selain itu, antara penugasan, variabel ini tidak digunakan di mana pun. Aneh ... Mungkin urutan ini tidak melanggar logika program, tetapi penugasan semacam itu sendiri berbicara tentang beberapa kebingungan saat menulis kode. Apakah ini sebenarnya sebuah kesalahan hanya bisa dikatakan oleh penulis. Meskipun ada contoh yang lebih terang dari kesalahan ini dalam kode:

 void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... } 

Sudah jelas di sini bahwa di suatu tempat di sini ada kesalahan dalam logika, atau setidaknya tugas yang tidak perlu. Mungkin baris kedua ditulis sementara untuk debugging, dan kemudian mereka lupa untuk menghapusnya. Secara total, PVS-Studio mengeluarkan 8 peringatan tentang situasi seperti itu.

Peringatan 3

Objek 'pKey' V808 dari tipe 'basic_string' telah dibuat tetapi tidak digunakan. editor.cpp 1866

 void editorclass::load(std::string &_path) { .... std::string pKey(pElem->Value()); .... if (pKey == "edEntities") { int i = 0; for (TiXmlElement *edEntityEl = pElem->FirstChildElement(); edEntityEl; edEntityEl = edEntityEl->NextSiblingElement()) { std::string pKey(edEntityEl->Value()); // <= //const char* pText = edEntityEl->GetText() ; if (edEntityEl->GetText() != NULL) { edentity[i].scriptname = std::string(edEntityEl->GetText()); } edEntityEl->QueryIntAttribute("x", &edentity[i].x); edEntityEl->QueryIntAttribute("y", &edentity[i].y); edEntityEl->QueryIntAttribute("t", &edentity[i].t); edEntityEl->QueryIntAttribute("p1", &edentity[i].p1); edEntityEl->QueryIntAttribute("p2", &edentity[i].p2); edEntityEl->QueryIntAttribute("p3", &edentity[i].p3); edEntityEl->QueryIntAttribute("p4", &edentity[i].p4); edEntityEl->QueryIntAttribute("p5", &edentity[i].p5); edEntityEl->QueryIntAttribute("p6", &edentity[i].p6); i++; } EditorData::GetInstance().numedentities = i; } .... } 

Kode yang sangat aneh Penganalisa memperingatkan tentang variabel pKey yang dibuat tetapi tidak digunakan, tetapi sebenarnya masalahnya ternyata lebih menarik. Saya secara khusus ditandai dengan panah garis di mana peringatan itu dikeluarkan, karena fungsi ini berisi lebih dari satu definisi baris dengan nama pKey . Ya, variabel lain yang dideklarasikan di dalam for loop, dan dengan namanya itu tumpang tindih dengan yang dinyatakan di luar loop.

Jadi, jika Anda mengakses nilai string pKey di luar loop for , Anda akan mendapatkan nilai yang sama dengan pElem-> Value () , tetapi jika Anda melakukan hal yang sama di dalam loop, Anda akan mendapatkan nilai yang sama dengan edEntityEl-> Value () . Nama yang tumpang tindih adalah kesalahan yang agak kasar, yang bisa sangat sulit ditemukan sendiri saat Anda meninjau kode.

Peringatan 4

V805 Menurunkan Kinerja. Tidak efisien untuk mengidentifikasi string kosong dengan menggunakan konstruksi 'strlen (str)> 0'. Cara yang lebih efisien adalah dengan memeriksa: str [0]! = '\ 0'. physfs.c 1604

 static char *prefDir = NULL; .... const char *PHYSFS_getPrefDir(const char *org, const char *app) { .... assert(strlen(prefDir) > 0); ... return prefDir; } /* PHYSFS_getPrefDir */ 

Penganalisa menemukan tempat untuk optimasi mikro potensial. Ini menggunakan fungsi strlen untuk memeriksa string C-style untuk membatalkan. Fungsi ini "menjalankan" semua elemen dari string dan memeriksa masing-masing untuk kesetaraan ke terminal nol ('\ 0'). Jika string besar dimasukkan, maka masing-masing karakternya masih akan dibandingkan dengan string nol.

Tetapi kita hanya perlu memeriksa bahwa string tersebut tidak kosong! Untuk melakukan ini, cari tahu apakah karakter pertama dari string adalah nol terminal. Oleh karena itu, untuk mengoptimalkan cek dalam ini, Anda harus menulis:

 str[0] != '\0' 

Ini adalah rekomendasi yang diberikan oleh analisa. Tentu saja, pemanggilan fungsi strlen dalam kondisi makro yang tegas , jadi itu hanya akan dieksekusi dalam versi debug, di mana kecepatan tidak begitu penting. Dalam versi rilis, pemanggilan fungsi akan hilang dan kode akan bekerja dengan cepat. Namun demikian, saya ingin menunjukkan kemampuan penganalisa dalam mengusulkan optimasi mikro.

Peringatan 5

Untuk memperlihatkan kesalahan berikut, saya perlu melampirkan dua potong kode di sini: deklarasi kelas entclass dan konstruktornya. Mari kita mulai dengan pengumuman:

 class entclass { public: entclass(); void clear(); bool outside(); public: //Fundamentals bool active, invis; int type, size, tile, rule; int state, statedelay; int behave, animate; float para; int life, colour; //Position and velocity int oldxp, oldyp; float ax, ay, vx, vy; int cx, cy, w, h; float newxp, newyp; bool isplatform; int x1, y1, x2, y2; //Collision Rules int onentity; bool harmful; int onwall, onxwall, onywall; //Platforming specific bool jumping; bool gravity; int onground, onroof; int jumpframe; //Animation int framedelay, drawframe, walkingframe, dir, actionframe; int yp; int xp; }; 

Konstruktor kelas ini terlihat seperti ini:

 entclass::entclass() { clear(); } void entclass::clear() { // Set all values to a default, // required for creating a new entity active = false; invis = false; type = 0; size = 0; tile = 0; rule = 0; state = 0; statedelay = 0; life = 0; colour = 0; para = 0; behave = 0; animate = 0; xp = 0; yp = 0; ax = 0; ay = 0; vx = 0; vy = 0; w = 16; h = 16; cx = 0; cy = 0; newxp = 0; newyp = 0; x1 = 0; y1 = 0; x2 = 320; y2 = 240; jumping = false; gravity = false; onground = 0; onroof = 0; jumpframe = 0; onentity = 0; harmful = false; onwall = 0; onxwall = 0; onywall = 0; isplatform = false; framedelay = 0; drawframe = 0; walkingframe = 0; dir = 0; actionframe = 0; } 

Bidang yang cukup, bukan? Tidak mengherankan bahwa kesalahan mengintai di sini, di mana PVS-Studio mengeluarkan peringatan:

V730 Ada kemungkinan bahwa tidak semua anggota kelas diinisialisasi di dalam konstruktor. Pertimbangkan untuk memeriksa: oldxp, oldyp. Ent.cpp 3

Seperti yang Anda lihat, dalam daftar yang sedemikian besar, inisialisasi dua bidang kelas hilang. Dengan demikian, nilai-nilai mereka tetap tidak terdefinisi, karena itu, di tempat lain dalam program ini, nilai yang tidak diketahui dapat dibaca dan digunakan dari mereka. Sangat sulit untuk mendeteksi kesalahan seperti itu melalui mata.

Peringatan 6

Pertimbangkan kodenya:

 void mapclass::loadlevel(....) { .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); .... //  tmap    . } 

Peringatan PVS-studio: V688 Variabel lokal 'tmap' memiliki nama yang sama dengan salah satu anggota kelas, yang dapat mengakibatkan kebingungan. Map.cpp 1192

Memang, jika Anda melihat ke dalam kelas mapclass , Anda dapat menemukan di sana vektor yang sama dengan nama yang sama:

 class mapclass { public: .... std::vector <int> roomdeaths; std::vector <int> roomdeathsfinal; std::vector <int> areamap; std::vector <int> contents; std::vector <int> explored; std::vector <int> vmult; std::vector <std::string> tmap; // <= .... }; 

Sayangnya, mendeklarasikan vektor dengan nama yang sama di dalam suatu fungsi membuat vektor yang dinyatakan dalam kelas tidak terlihat. Ternyata di seluruh fungsi tingkat beban, vektor tmap hanya berubah di dalam fungsi. Vektor yang dideklarasikan di kelas tetap tidak berubah!

Menariknya, PVS-Studio menemukan sebanyak 20 fragmen kode seperti itu! Sebagian besar, mereka terkait dengan variabel sementara, yang, untuk kenyamanan, dinyatakan sebagai anggota kelas. Penulis permainan (dan satu-satunya pengembang) sendiri menulis bahwa ia dulu memiliki kebiasaan buruk ini. Anda dapat membaca tentang ini di pos yang saya tautkan pada awal artikel ini.

Dia juga mencatat bahwa penamaan tersebut menyebabkan bug yang berbahaya dan sulit ditangkap. Nah, bug seperti itu benar-benar bisa berbahaya, tetapi menggunakan analisis statis untuk menangkapnya tidak akan sulit :)

Peringatan 7

V601 Tipe integer secara implisit dilemparkan ke tipe char. Game.cpp 4997

 void Game::loadquick(....) { .... else if (pKey == "totalflips") { totalflips = atoi(pText); } else if (pKey == "hardestroom") { hardestroom = atoi(pText); // <= } else if (pKey == "hardestroomdeaths") { hardestroomdeaths = atoi(pText); } .... } 

Untuk memahami apa masalahnya, mari kita lihat definisi variabel dari bagian kode yang diberikan:

 //Some stats: int totalflips; std::string hardestroom; int hardestroomdeaths; 

Variabel totalflip dan hardestroomdeaths adalah tipe integer, dan karenanya menugaskan hasilnya ke fungsi atoi di dalamnya benar-benar normal. Tetapi apa yang terjadi jika Anda menetapkan nilai integer ke std :: string ? Ternyata dari sudut pandang bahasa, penugasan semacam itu benar-benar valid. Akibatnya, beberapa nilai yang tidak dapat dipahami akan ditulis ke dalam variabel hardestroom !

Peringatan 8

V1004 Pointer 'pElem' digunakan dengan tidak aman setelah diverifikasi terhadap nullptr. Periksa baris: 1739, 1744. editor.cpp 1744

 void editorclass::load(std::string &_path) { .... TiXmlHandle hDoc(&doc); TiXmlElement *pElem; TiXmlHandle hRoot(0); version = 0; { pElem = hDoc.FirstChildElement().Element(); // should always have a valid root // but handle gracefully if it does if (!pElem) { printf("No valid root! Corrupt level file?\n"); } pElem->QueryIntAttribute("version", &version); // <= // save this for later hRoot = TiXmlHandle(pElem); } .... } 

Penganalisa memperingatkan bahwa pointer pElem tidak aman digunakan segera setelah diperiksa untuk nullptr . Untuk memastikan bahwa penganalisisnya benar, lihat definisi fungsi Elemen () , nilai balik yang menginisialisasi penunjuk pElem :

 /** @deprecated use ToElement. Return the handle as a TiXmlElement. This may return null. */ TiXmlElement *Element() const { return ToElement(); } 

Seperti yang Anda lihat dari komentar, fungsi ini dapat mengembalikan nol .

Sekarang bayangkan ini benar-benar terjadi. Apa yang akan terjadi dalam kasus ini? Faktanya adalah bahwa situasi seperti itu tidak akan ditangani dengan cara apa pun. Ya, sebuah pesan akan ditampilkan yang menyatakan bahwa ada sesuatu yang salah, tetapi secara harfiah garis di bawah penunjuk yang salah akan disangkal. Dereferencing seperti itu, pada gilirannya, akan menyebabkan crash program atau perilaku yang tidak terdefinisi. Ini adalah kesalahan yang cukup serius.

Peringatan 9

Di bagian kode berikutnya, PVS-Studio mengeluarkan empat peringatan:

• V560 Bagian dari ekspresi kondisional selalu benar: x> = 0. editor.cpp 1137
• V560 Bagian dari ekspresi kondisional selalu benar: y> = 0. editor.cpp 1137
• V560 Bagian dari ekspresi kondisional selalu benar: x <40. editor.cpp 1137
• V560 Bagian dari ekspresi kondisional selalu benar: y <30. editor.cpp 1137

 int editorclass::at( int x, int y ) { if(x<0) return at(0,y); if(y<0) return at(x,0); if(x>=40) return at(39,y); if(y>=30) return at(x,29); if(x>=0 && y>=0 && x<40 && y<30) { return contents[x+(levx*40)+vmult[y+(levy*30)]]; } return 0; } 

Semua peringatan berlaku untuk pernyataan if terakhir. Masalahnya adalah bahwa keempat pemeriksaan yang dilakukan di atasnya akan selalu kembali benar . Saya tidak akan mengatakan bahwa ini adalah kesalahan serius, tetapi ternyata cukup lucu. Penulis memutuskan untuk mengambil fungsi ini dengan serius dan, untuk berjaga-jaga, memeriksa setiap variabel lagi :)

Pemeriksaan ini dapat dihapus, karena utas eksekusi tidak akan pernah mencapai ekspresi " return 0; ". Meskipun ini tidak mengubah logika program, itu akan membebaskannya dari pemeriksaan yang tidak perlu dan kode mati.

Peringatan 10

Dalam artikelnya pada hari peringatan permainan, Terry mengatakan dengan ironi bahwa salah satu elemen yang mengendalikan logika permainan adalah peralihan besar dari fungsi Game :: updatestate () , yang bertanggung jawab langsung untuk sejumlah besar kondisi permainan yang berbeda. Dan sangat diharapkan bahwa saya akan menemukan peringatan berikut:

V2008 Kompleksitas siklus: 548. Pertimbangkan untuk melakukan refactoring pada fungsi 'Game :: updatestate'. Game.cpp 612

Ya, Anda mengerti dengan benar: PVS-Studio memperkirakan kompleksitas siklomatik suatu fungsi pada 548 unit. Lima ratus empat puluh delapan !!! Ini saya mengerti - "kode rapi." Dan ini terlepas dari kenyataan bahwa, pada kenyataannya, tidak ada yang lebih dari ekspresi saklar dalam suatu fungsi. Di sakelar itu sendiri, saya menghitung lebih dari 300 ekspresi kasus.

Di kantor kami ada kompetisi kecil antara penulis untuk artikel terpanjang. Saya dengan senang hati akan membawa semua kode fungsi di sini (3450 baris), tetapi kemenangan seperti itu akan tidak jujur, jadi saya akan membatasi diri hanya dengan merujuk ke saklar besar. Saya merekomendasikan untuk mengikuti dan mengevaluasi seluruh skala sendiri! Omong-omong, selain Game :: updatestate () , PVS-Studio menemukan sebanyak 44 fungsi dengan kompleksitas siklomatik yang berlebihan, 10 di antaranya memiliki kompleksitas lebih dari 200.

Kesimpulan

Saya pikir kesalahan tertulis sudah cukup untuk artikel ini. Ya, ada banyak kesalahan dalam proyek, tetapi ini persis trik: setelah meletakkan kodenya, Terry Cavanagh menunjukkan bahwa tidak perlu menjadi pemrogram yang baik untuk membuat permainan yang baik. Sekarang, setelah 10 tahun, Terry mengingat saat-saat itu dengan ironi. Belajar dari kesalahan Anda adalah penting, dan berlatih adalah cara terbaik untuk melakukan ini. Dan jika latihan Anda masih bisa memunculkan gim seperti VVVVVVV, maka ini umumnya cantik! Ehh ... Saya akan pergi dan saya mungkin akan memainkannya lagi :)

Ini bukan semua kesalahan yang ditemukan dalam kode game. Jika Anda ingin melihat sendiri apa lagi yang bisa Anda temukan, saya sarankan mengunduh dan mencoba PVS-Studio ! Juga jangan lupa bahwa untuk proyek open source kami memberikan lisensi gratis.

Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: George Gribkov. VVVVVV ??? VVVVVV !!! :)