Geekly articles weekly

CMake: Kasus ketika Kualitas Proyek tidak dapat dimaafkan

Gambar 1

CMake adalah sistem lintas platform untuk mengotomatisasi pembuatan proyek. Sistem ini jauh lebih tua daripada penganalisis kode statis PVS-Studio, tetapi tidak ada yang mencoba menerapkan penganalisa pada kodenya dan meninjau kesalahannya. Ternyata, ada banyak dari mereka. Audiens CMake sangat besar. Proyek baru dimulai dan yang lama porting. Saya ngeri memikirkan berapa banyak pengembang yang bisa mendapatkan kesalahan.

Pendahuluan


CMake adalah sistem lintas platform untuk mengotomatisasi pembuatan perangkat lunak dari kode sumber. CMake tidak dimaksudkan secara langsung untuk membangun, itu hanya menghasilkan file untuk mengontrol membangun dari file CMakeLists.txt. Rilis pertama dari program ini terjadi pada tahun 2000. Sebagai perbandingan, analisa PVS-Studio hanya muncul pada tahun 2008. Pada saat itu, program ini bertujuan untuk mencari bug yang dihasilkan dari porting sistem 32-bit ke sistem 64-bit. Pada 2010, set pertama tujuan umum diagnostik muncul (V501- V545 ). Omong-omong, kode CMake memiliki beberapa peringatan dari set pertama ini.

Kesalahan yang tidak bisa dimaafkan


V1040 Kemungkinan salah ketik dalam ejaan nama makro yang telah ditentukan sebelumnya. Makro '__MINGW32_' mirip dengan '__MINGW32__'. winapi.h 4112

/* from winternl.h */ #if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_) #define __UNICODE_STRING_DEFINED #endif

Diagnostik V1040 diimplementasikan belum lama ini. Kemungkinan besar, pada saat memposting artikel, artikel itu belum dirilis, namun, kami sudah menemukan kesalahan keren dengan bantuannya.

Ada kesalahan ketik yang dibuat atas nama __MINGW32_ . Pada akhirnya, satu karakter garis bawah hilang. Jika Anda mencari kode dengan nama ini, Anda dapat melihat bahwa versi dengan dua karakter garis bawah di kedua sisi digunakan dalam proyek:

Gambar 3

V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 558

 bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, const std::string& regKeyBase, std::string& nextAvailableSubKeyName) { .... if (ERROR_SUCCESS == result) { wchar_t subkeyname[256]; // <= DWORD cch_subkeyname = sizeof(subkeyname) * sizeof(subkeyname[0]); // <= wchar_t keyclass[256]; DWORD cch_keyclass = sizeof(keyclass) * sizeof(keyclass[0]); FILETIME lastWriteTime; lastWriteTime.dwHighDateTime = 0; lastWriteTime.dwLowDateTime = 0; while (ERROR_SUCCESS == RegEnumKeyExW(hkey, index, subkeyname, &cch_subkeyname, 0, keyclass, &cch_keyclass, &lastWriteTime)) { .... } .... }

Untuk array yang dinyatakan secara statis, sizeof operator akan menghitung ukuran dalam byte, dengan mempertimbangkan jumlah elemen dan ukurannya. Saat mengevaluasi nilai variabel cch_subkeyname , pengembang tidak memperhitungkannya dan mendapat nilai 4 kali lebih besar dari yang dimaksudkan. Mari kita jelaskan dari mana "empat kali" berasal.

Array dan ukurannya yang salah diteruskan ke fungsi RegEnumKeyExW :

 LSTATUS RegEnumKeyExW( HKEY hKey, DWORD dwIndex, LPWSTR lpName, // <= subkeyname LPDWORD lpcchName, // <= cch_subkeyname LPDWORD lpReserved, LPWSTR lpClass, LPDWORD lpcchClass, PFILETIME lpftLastWriteTime );

Pointer lpcchName harus mengarah ke variabel, berisi ukuran buffer dalam karakter: "Sebuah pointer ke variabel yang menentukan ukuran buffer yang ditentukan oleh parameter lpClass , dalam karakter". Ukuran array subkeyname adalah 512 byte dan dapat menyimpan 256 karakter dari tipe wchar_t (di Windows, wchar_t adalah 2 byte). Ini adalah 256 yang harus diteruskan ke fungsi. Sebaliknya, 512 dikalikan dengan 2 dan kami mendapatkan 1024.

Saya pikir, sekarang sudah jelas bagaimana cara memperbaiki kesalahan ini. Anda perlu menggunakan pembagian alih-alih perkalian:

 DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]);

Ngomong-ngomong, kesalahan yang sama terjadi ketika mengevaluasi nilai variabel cch_keyclass .

Kesalahan yang dijelaskan berpotensi menyebabkan buffer overflow. Semua fragmen seperti itu pasti harus diperbaiki:

  • V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 556
  • V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 572
  • V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 621
  • V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 622
  • V531 Aneh bahwa operator sizeof () dikalikan dengan sizeof (). cmGlobalVisualStudioGenerator.cxx 649

V595 Pointer 'this-> BuildFileStream' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 133, 134. cmMakefileTargetGenerator.cxx 133

 void cmMakefileTargetGenerator::CreateRuleFile() { .... this->BuildFileStream->SetCopyIfDifferent(true); if (!this->BuildFileStream) { return; } .... }

Pointer ini-> BuildFileStream dereferenced tepat sebelum memeriksa validitasnya. Bukankah itu menimbulkan masalah bagi siapa pun? Di bawah ini ada contoh lain dari cuplikan tersebut. Itu dibuat seperti salinan karbon. Tetapi pada kenyataannya, ada banyak peringatan V595 dan kebanyakan dari mereka tidak begitu jelas. Dari pengalaman saya, saya dapat mengatakan bahwa mengoreksi peringatan diagnostik ini memakan waktu paling lama.

  • V595 Pointer 'this-> FlagFileStream' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 303, 304. cmMakefileTargetGenerator.cxx 303

V614 Uninitialized pointer 'str' digunakan. cmVSSetupHelper.h 80

 class SmartBSTR { public: SmartBSTR() { str = NULL; } SmartBSTR(const SmartBSTR& src) { if (src.str != NULL) { str = ::SysAllocStringByteLen((char*)str, ::SysStringByteLen(str)); } else { str = ::SysAllocStringByteLen(NULL, 0); } } .... private: BSTR str; };

Analyzer mendeteksi penggunaan penunjuk str yang tidak diinisialisasi. Itu muncul karena kesalahan ketik biasa. Saat memanggil fungsi SysAllocStringByteLen , seseorang harus menggunakan pointer src.str .

V557 Array overrun dimungkinkan. Nilai indeks 'lensymbol' bisa mencapai 28. archive_read_support_format_rar.c 2749

 static int64_t expand(struct archive_read *a, int64_t end) { .... if ((lensymbol = read_next_symbol(a, &rar->lengthcode)) < 0) goto bad_data; if (lensymbol > (int)(sizeof(lengthbases)/sizeof(lengthbases[0]))) goto bad_data; if (lensymbol > (int)(sizeof(lengthbits)/sizeof(lengthbits[0]))) goto bad_data; len = lengthbases[lensymbol] + 2; if (lengthbits[lensymbol] > 0) { if (!rar_br_read_ahead(a, br, lengthbits[lensymbol])) goto truncated_data; len += rar_br_bits(br, lengthbits[lensymbol]); rar_br_consume(br, lengthbits[lensymbol]); } .... }

Bagian kode ini menyembunyikan beberapa masalah sekaligus. Saat mengakses array basis panjang dan panjang , indeks array mungkin keluar dari batas, karena pengembang menulis operator '>' alih-alih '> =' di atas. Pemeriksaan ini mulai kehilangan satu nilai yang tidak dapat diterima. Di sini kita tidak memiliki apa pun kecuali pola kesalahan klasik yang disebut Kesalahan Off-by-one .

Inilah seluruh daftar operasi akses array oleh indeks yang tidak valid:

  • V557 Array overrun dimungkinkan. Nilai indeks 'lensymbol' bisa mencapai 28. archive_read_support_format_rar.c 2750
  • V557 Array overrun dimungkinkan. Nilai indeks 'lensymbol' bisa mencapai 28. archive_read_support_format_rar.c 2751
  • V557 Array overrun dimungkinkan. Nilai indeks 'lensymbol' bisa mencapai 28. archive_read_support_format_rar.c 2753
  • V557 Array overrun dimungkinkan. Nilai indeks 'lensymbol' bisa mencapai 28. archive_read_support_format_rar.c 2754
  • V557 Array overrun dimungkinkan. Nilai indeks 'offssymbol' bisa mencapai 60. archive_read_support_format_rar.c 2797

Kebocoran memori


V773 Fungsi itu keluar tanpa melepaskan pointer 'testRun'. Kebocoran memori dimungkinkan. cmCTestMultiProcessHandler.cxx 193

 void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner, bool started) { .... delete runner; if (started) { this->StartNextTests(); } } bool cmCTestMultiProcessHandler::StartTestProcess(int test) { .... cmCTestRunTest* testRun = new cmCTestRunTest(*this); // <= .... if (testRun->StartTest(this->Completed, this->Total)) { return true; // <= } } this->FinishTestProcess(testRun, false); // <= return false; }

Analyzer mendeteksi kebocoran memori. Memori oleh pointer testRun tidak dirilis, jika fungsi testRun-> StartTest mengembalikan true . Ketika menjalankan cabang kode lain, memori ini akan dirilis dalam fungsi ini-> FinishTestProcess.

Kebocoran sumber daya


V773 Fungsi ini keluar tanpa menutup file yang direferensikan oleh pegangan 'fd'. Kebocoran sumber daya mungkin terjadi. rhash.c 450

 RHASH_API int rhash_file(....) { FILE* fd; rhash ctx; int res; hash_id &= RHASH_ALL_HASHES; if (hash_id == 0) { errno = EINVAL; return -1; } if ((fd = fopen(filepath, "rb")) == NULL) return -1; if ((ctx = rhash_init(hash_id)) == NULL) return -1; // <= fclose(fd); ??? res = rhash_file_update(ctx, fd); fclose(fd); rhash_final(ctx, result); rhash_free(ctx); return res; }

Logika Aneh dalam Kondisi


V590 Pertimbangkan untuk memeriksa ekspresi '* s! =' \ 0 '&& * s ==' ''. Ekspresi berlebihan atau mengandung kesalahan cetak. archive_cmdline.c 76

 static ssize_t get_argument(struct archive_string *as, const char *p) { const char *s = p; archive_string_empty(as); /* Skip beginning space characters. */ while (*s != '\0' && *s == ' ') s++; .... }

* Perbandingan karakter dengan null adalah mubazir. Kondisi loop sementara hanya bergantung pada apakah karakter sama dengan spasi atau tidak. Ini bukan kesalahan, tetapi komplikasi kode yang tidak perlu.

V592 Ekspresi tertutup oleh tanda kurung dua kali: ((ekspresi)). Sepasang tanda kurung tidak perlu atau salah cetak ada. cmCTestTestHandler.cxx 899

 void cmCTestTestHandler::ComputeTestListForRerunFailed() { this->ExpandTestsToRunInformationForRerunFailed(); ListOfTests finalList; int cnt = 0; for (cmCTestTestProperties& tp : this->TestList) { cnt++; // if this test is not in our list of tests to run, then skip it. if ((!this->TestsToRun.empty() && std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end())) { continue; } tp.Index = cnt; finalList.push_back(tp); } .... }

Penganalisa memperingatkan bahwa operasi negasi mungkin harus dikeluarkan dari kurung. Tampaknya tidak ada bug di sini - hanya kurung ganda yang tidak perlu. Tetapi kemungkinan besar, ada kesalahan logika dalam kode.

Operator lanjutan dieksekusi hanya dalam kasus jika daftar tes ini-> TestsToRun tidak kosong dan cnt tidak ada di dalamnya. Masuk akal untuk mengasumsikan bahwa jika daftar tes kosong, tindakan yang sama perlu dilakukan. Kemungkinan besar, kondisinya adalah sebagai berikut:

 if (this->TestsToRun.empty() || std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end()) { continue; }

V592 Ekspresi tertutup oleh tanda kurung dua kali: ((ekspresi)). Sepasang tanda kurung tidak perlu atau salah cetak ada. cmMessageCommand.cxx 73

 bool cmMessageCommand::InitialPass(std::vector<std::string> const& args, cmExecutionStatus&) { .... } else if (*i == "DEPRECATION") { if (this->Makefile->IsOn("CMAKE_ERROR_DEPRECATED")) { fatal = true; type = MessageType::DEPRECATION_ERROR; level = cmake::LogLevel::LOG_ERROR; } else if ((!this->Makefile->IsSet("CMAKE_WARN_DEPRECATED") || this->Makefile->IsOn("CMAKE_WARN_DEPRECATED"))) { type = MessageType::DEPRECATION_WARNING; level = cmake::LogLevel::LOG_WARNING; } else { return true; } ++i; } .... }

Ini adalah contoh yang serupa, tetapi kali ini saya lebih yakin bahwa kesalahan terjadi. Fungsi IsSet ("CMAKE_WARN_DEPRECATED") memeriksa bahwa nilai CMAKE_WARN_DEPRECATED diatur secara global, dan fungsi IsOn ("CMAKE_WARN_DEPRECATED) memeriksa bahwa nilai tersebut diatur dalam konfigurasi proyek. Kemungkinan besar, operator komplementer itu berlebihan, karena dalam kedua kasus, benar untuk menetapkan nilai tipe dan level yang sama.

V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. cmCTestRunTest.cxx 151

 bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started) { .... } else if ((success && !this->TestProperties->WillFail) || (!success && this->TestProperties->WillFail)) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } .... }

Kode ini bisa lebih sederhana. Seseorang dapat menulis ulang ekspresi kondisional dengan cara berikut:

 } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; }

Beberapa tempat lagi untuk disederhanakan:

  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A && B) || Ekspresi (! A &&! B) 'sama dengan ekspresi' bool (A) == bool (B) '. cmCTestTestHandler.cxx 702
  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. digest_sspi.c 443
  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. tcp.c 1295
  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. testDynamicLoader.cxx 58
  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. testDynamicLoader.cxx 65
  • V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. testDynamicLoader.cxx 72

Berbagai peringatan


V523 Pernyataan 'maka' setara dengan fragmen kode berikutnya. archive_read_support_format_ar.c 415

 static int _ar_read_header(struct archive_read *a, struct archive_entry *entry, struct ar *ar, const char *h, size_t *unconsumed) { .... /* * "__.SYMDEF" is a BSD archive symbol table. */ if (strcmp(filename, "__.SYMDEF") == 0) { archive_entry_copy_pathname(entry, filename); /* Parse the time, owner, mode, size fields. */ return (ar_parse_common_header(ar, entry, h)); } /* * Otherwise, this is a standard entry. The filename * has already been trimmed as much as possible, based * on our current knowledge of the format. */ archive_entry_copy_pathname(entry, filename); return (ar_parse_common_header(ar, entry, h)); }

Ekspresi dalam kondisi terakhir mirip dengan dua baris terakhir dari fungsi. Pengembang dapat menyederhanakan kode ini dengan menghapus ketentuan, atau ada kesalahan dalam kode dan harus diperbaiki.

V535 Variabel 'i' digunakan untuk loop ini dan untuk loop luar. Periksa baris: 2220, 2241. multi.c 2241

 static CURLMcode singlesocket(struct Curl_multi *multi, struct Curl_easy *data) { .... for(i = 0; (i< MAX_SOCKSPEREASYHANDLE) && // <= (curraction & (GETSOCK_READSOCK(i) | GETSOCK_WRITESOCK(i))); i++) { unsigned int action = CURL_POLL_NONE; unsigned int prevaction = 0; unsigned int comboaction; bool sincebefore = FALSE; s = socks[i]; /* get it from the hash */ entry = sh_getentry(&multi->sockhash, s); if(curraction & GETSOCK_READSOCK(i)) action |= CURL_POLL_IN; if(curraction & GETSOCK_WRITESOCK(i)) action |= CURL_POLL_OUT; actions[i] = action; if(entry) { /* check if new for this transfer */ for(i = 0; i< data->numsocks; i++) { // <= if(s == data->sockets[i]) { prevaction = data->actions[i]; sincebefore = TRUE; break; } } } .... }

Variabel i digunakan sebagai penghitung lingkaran di loop luar dan dalam. Pada saat yang sama, nilai penghitung kembali dimulai dari nol di loop dalam. Mungkin ini bukan bug di sini, tetapi kodenya mencurigakan.

V519 Variabel 'tagString' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 84, 86. cmCPackLog.cxx 86

 void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, size_t length) { .... if (tag & LOG_OUTPUT) { output = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "VERBOSE"; } } if (tag & LOG_WARNING) { warning = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "WARNING"; } } .... }

Variabel tagString ditimpa dengan nilai baru di semua tempat. Sulit mengatakan apa masalahnya atau mengapa mereka melakukannya. Mungkin, operator '=' dan '+ =' kacau.

Seluruh daftar tempat-tempat tersebut:

  • V519 Variabel 'tagString' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 94, 96. cmCPackLog.cxx 96
  • V519 Variabel 'tagString' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 104, 106. cmCPackLog.cxx 106
  • V519 Variabel 'tagString' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 114, 116. cmCPackLog.cxx 116
  • V519 Variabel 'tagString' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 125, 127. cmCPackLog.cxx 127

V519 Variabel 'aes-> aes_set' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 4052, 4054. archive_string.c 4054

 int archive_mstring_copy_utf8(struct archive_mstring *aes, const char *utf8) { if (utf8 == NULL) { aes->aes_set = 0; // <= } aes->aes_set = AES_SET_UTF8; // <= .... return (int)strlen(utf8); }

Pengaturan paksa nilai AES_SET_UTF8 terlihat mencurigakan. Saya pikir kode seperti itu akan membingungkan pengembang mana pun, yang datang untuk memperbaiki fragmen ini.

Kode ini disalin ke tempat lain:

  • V519 Variabel 'aes-> aes_set' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 4066, 4068. archive_string.c 4068

Cara Menemukan Bug dalam Proyek di CMake


Pada bagian ini, saya akan secara singkat memberi tahu Anda cara memeriksa proyek-proyek CMake dengan PVS-Studio semudah satu-dua-tiga.

Windows / Visual Studio

Untuk Visual Studio, Anda dapat membuat file proyek menggunakan CMake GUI atau perintah berikut:

 cmake -G "Visual Studio 15 2017 Win64" ..

Selanjutnya, Anda dapat membuka file .sln dan memeriksa proyek menggunakan plugin untuk Visual Studio.

Linux / macOS

File compile_commands.json digunakan untuk memeriksa sistem ini. Omong-omong, itu dapat dihasilkan dalam sistem pembangunan yang berbeda. Ini adalah bagaimana Anda melakukannya di CMake:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Hal terakhir yang harus dilakukan adalah menjalankan alat analisa dalam direktori dengan file .json:

 pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/project.log -e /path/to/exclude-path -j<N>

Kami juga telah mengembangkan modul untuk proyek-proyek CMake. Beberapa orang suka menggunakannya. Modul CMake dan contoh penggunaannya dapat ditemukan di repositori kami di GitHub: pvs-studio-cmake-example .

Kesimpulan


Sejumlah besar pengguna CMake hebat untuk menguji proyek, tetapi banyak masalah dapat dicegah sebelum rilis dengan menggunakan alat analisis kode statis, seperti PVS-Studio .

Jika Anda menyukai hasil analisis, tetapi proyek Anda tidak ditulis dalam C dan C ++, saya ingin mengingatkan bahwa analis juga mendukung analisis proyek di C # dan Java. Anda dapat menguji penganalisa pada proyek Anda dengan membuka halaman ini.

Source: https://habr.com/ru/post/id464145/

More articles:


All Articles