"Petualangan" dengan klien email Mozilla Thunderbird dimulai dengan peningkatan otomatis ke versi 68.0. Fitur utama dari versi ini adalah ini: lebih banyak teks ditambahkan ke pemberitahuan pop-up dan tema gelap secara default. Ada kesalahan yang ingin saya coba deteksi menggunakan analisis statis. Ini adalah kesempatan untuk sekali lagi memeriksa kode sumber proyek menggunakan PVS-Studio. Ternyata pada saat analisis kesalahan sudah diperbaiki. Tetapi karena kami menarik perhatian pada proyek ini, kami dapat menulis tentang cacat lain yang ditemukan di dalamnya.
Pendahuluan
Tema gelap versi baru Thunderbird terlihat cukup cantik. Saya suka tema gelap. Sudah beralih ke mereka dalam pesan instan, Windows, macOS. Segera, iPhone akan ditingkatkan ke iOS 13, di mana tema gelap telah muncul. Untuk ini, saya bahkan harus mengubah iPhone 5S saya ke model yang lebih baru. Dalam praktiknya, ternyata tema gelap membutuhkan lebih banyak upaya bagi pengembang untuk memilih warna antarmuka. Tidak semua orang mengatasi ini pertama kali.
Jadi tag standar saya di Thunderbird mulai terlihat seperti:
Saya menggunakan enam tag (5 standar + 1 custom) untuk menandai email. Menjadi mustahil untuk menonton setengah dari mereka setelah pembaruan, dan saya memutuskan untuk mengubah warna menjadi lebih terang di pengaturan. Tapi di sini saya menemukan bug:
Anda tidak dapat mengubah warna tag !!! Lebih tepatnya, itu mungkin, tetapi editor tidak akan membiarkannya disimpan, merujuk pada nama yang ada (WTF ???).
Manifestasi lain dari bug adalah tidak adanya tombol OK, jika Anda mencoba mengubah nama, karena Anda tidak dapat menyimpan dengan nama ini. Anda tidak dapat mengganti nama juga.
Akhirnya, Anda mungkin memperhatikan bahwa tema gelap tidak menyentuh pengaturan, yang juga tidak terlalu indah.
Setelah perjuangan panjang dengan sistem build di Windows, masih mungkin untuk merakit Thunderbird dari source. Versi terbaru dari klien surat ternyata jauh lebih baik daripada rilis terbaru. Di dalamnya, tema gelap sampai ke pengaturan, dan bug ini dengan editor tag juga menghilang. Tetapi agar pekerjaan perakitan proyek tidak sia-sia, penganalisa kode statis
PVS-Studio diluncurkan.
Catatan Kode sumber Thunderbird entah bagaimana tumpang tindih dengan basis kode Firefox. Oleh karena itu, analisis mencakup kesalahan dari komponen yang berbeda, yang patut dicermati pengembang tim yang berbeda.
Catatan 2 Sementara artikel sedang ditulis, pembaruan Thunderbird 68.1 keluar dengan perbaikan untuk bug ini:
comm
comm-central adalah repositori Mercurial dari kode ekstensi Thunderbird, SeaMonkey, dan Lightning.
V501 Ada sub-ekspresi identik '(! Strcmp (header, "Balas-Ke"))' di sebelah kiri dan di sebelah kanan '||' operator. nsEmitterUtils.cpp 28
extern "C" bool EmitThisHeaderForPrefSetting(int32_t dispType, const char *header) { .... if (nsMimeHeaderDisplayTypes::NormalHeaders == dispType) { if ((!strcmp(header, HEADER_DATE)) || (!strcmp(header, HEADER_TO)) || (!strcmp(header, HEADER_SUBJECT)) || (!strcmp(header, HEADER_SENDER)) || (!strcmp(header, HEADER_RESENT_TO)) || (!strcmp(header, HEADER_RESENT_SENDER)) || (!strcmp(header, HEADER_RESENT_FROM)) || (!strcmp(header, HEADER_RESENT_CC)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_REFERENCES)) || (!strcmp(header, HEADER_NEWSGROUPS)) || (!strcmp(header, HEADER_MESSAGE_ID)) || (!strcmp(header, HEADER_FROM)) || (!strcmp(header, HEADER_FOLLOWUP_TO)) || (!strcmp(header, HEADER_CC)) || (!strcmp(header, HEADER_ORGANIZATION)) || (!strcmp(header, HEADER_REPLY_TO)) || (!strcmp(header, HEADER_BCC))) return true; else return false; .... }
Baris
tajuk dibandingkan dengan
HEADER_REPLY_TO konstan dua kali. Mungkin di tempatnya seharusnya ada konstanta lain.
V501 Ada sub-ekspresi identik 'obj-> options-> header! = MimeHeadersCitation' di sebelah kiri dan di sebelah kanan operator '&&'. mimemsig.cpp 536
static int MimeMultipartSigned_emit_child(MimeObject *obj) { .... if (obj->options && obj->options->headers != MimeHeadersCitation && obj->options->write_html_p && obj->options->output_fn && obj->options->headers != MimeHeadersCitation && sig->crypto_closure) { .... } .... }
Perbandingan aneh lain dari variabel dengan nama yang mirip adalah
header . Seperti biasa, ada dua penjelasan yang memungkinkan: pemeriksaan tambahan atau salah ketik.
V517 Penggunaan
pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 1306, 1308. MapiApi.cpp 1306
void CMapiApi::ReportLongProp(const char *pTag, LPSPropValue pVal) { if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_LONG)) { nsCString num; nsCString num2; num.AppendInt((int32_t)pVal->Value.l); num2.AppendInt((int32_t)pVal->Value.l, 16); MAPI_TRACE3("%s %s, 0x%s\n", pTag, num, num2); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_NULL)) { MAPI_TRACE1("%s {NULL}\n", pTag); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_ERROR)) {
Rangkaian ekspresi kondisional dipercepat dengan menekan Ctrl + C dan Ctrl + V. Akibatnya, salah satu cabang tidak pernah dieksekusi.
V517 Penggunaan
pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 777, 816. nsRDFContentSink.cpp 777
nsresult RDFContentSinkImpl::GetIdAboutAttribute(const char16_t** aAttributes, nsIRDFResource** aResource, bool* aIsAnonymous) { .... if (localName == nsGkAtoms::about) { .... } else if (localName == nsGkAtoms::ID) { .... } else if (localName == nsGkAtoms::nodeID) { nodeID.Assign(aAttributes[1]); } else if (localName == nsGkAtoms::about) {
Kondisi pertama dan terakhir adalah sama. Kode tersebut menunjukkan bahwa kode tersebut sedang dalam proses penulisan. Aman untuk mengatakan bahwa dengan probabilitas tinggi kesalahan akan muncul dengan sendirinya setelah kode diselesaikan. Seorang programmer dapat mengubah kode yang dikomentari, tetapi ia tidak akan pernah mendapatkan kontrol. Hati-hati dan berhati-hati dengan kode ini.
V522 Dereferencing dari 'baris' penunjuk nol mungkin terjadi. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Dereferencing dari penunjuk
baris nol dimungkinkan pada baris berikut:
morkCell* cell = row->CellAt(ev, pos);
Kemungkinan besar, inisialisasi pointer dilewati sebelum baris ini, misalnya, menggunakan metode
GetRow , dll.
V543 Aneh bahwa nilai '-1' ditugaskan ke variabel 'm_lastError' tipe HRESULT. MapiApi.cpp 1050
class CMapiApi { .... private: static HRESULT m_lastError; .... }; CMsgStore *CMapiApi::FindMessageStore(ULONG cbEid, LPENTRYID lpEid) { if (!m_lpSession) { MAPI_TRACE0("FindMessageStore called before session is open\n"); m_lastError = -1; return NULL; } .... }
Tipe
HRESULT adalah tipe data yang kompleks. Bit variabel yang berbeda dari tipe ini mewakili bidang deskripsi kesalahan yang berbeda. Kode kesalahan harus ditetapkan menggunakan konstanta khusus dari file header sistem.
Beberapa tempat lagi:
- V543 Aneh bahwa nilai '-1' ditugaskan ke variabel 'm_lastError' tipe HRESULT. MapiApi.cpp 817
- V543 Aneh bahwa nilai '-1' ditugaskan ke variabel 'm_lastError' tipe HRESULT. MapiApi.cpp 1749
V579 Fungsi memset menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen ketiga. icalmime.c 195
icalcomponent* icalmime_parse(....) { struct sspm_part *parts; int i, last_level=0; icalcomponent *root=0, *parent=0, *comp=0, *last = 0; if ( (parts = (struct sspm_part *) malloc(NUM_PARTS*sizeof(struct sspm_part)))==0) { icalerror_set_errno(ICAL_NEWFAILED_ERROR); return 0; } memset(parts,0,sizeof(parts)); sspm_parse_mime(parts, NUM_PARTS, icalmime_local_action_map, get_string, data, 0 ); .... }
Variabel
bagian adalah penunjuk ke array struktur. Untuk mereset nilai struktur, mereka menggunakan fungsi
memset , tetapi mereka mentransfer ukuran pointer ke ukuran area memori.
Tempat mencurigakan lainnya:
- V579 Fungsi memset menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen ketiga. icalmime.c 385
- V579 Fungsi memset menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen ketiga. icalparameter.c 114
- V579 Fungsi snprintf menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen kedua. icaltimezone.c 1908
- V579 Fungsi snprintf menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen kedua. icaltimezone.c 1910
- V579 Fungsi strncmp menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen ketiga. sspm.c 707
- V579 Fungsi strncmp menerima pointer dan ukurannya sebagai argumen. Itu mungkin sebuah kesalahan. Periksa argumen ketiga. sspm.c 813
V595 Pointer 'aValues' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 553, 555. nsLDAPMessage.cpp 553
NS_IMETHODIMP nsLDAPMessage::GetBinaryValues(const char *aAttr, uint32_t *aCount, nsILDAPBERValue ***aValues) { .... *aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; } .... }
Diagnostik
V595 biasanya menemukan kesalahan khas penunjuk null pointer. Tetapi ada ditemukan kasus yang sangat menarik, layak mendapat perhatian khusus.
Secara teknis, penganalisa benar bahwa pointer
aValues pertama kali
direferensikan , kemudian diperiksa, tetapi kesalahannya berbeda. Ini adalah penunjuk ganda, jadi kode yang benar akan terlihat seperti ini:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Tempat lain yang sangat mirip:
- V595 Pointer '_retval' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 357, 358. nsLDAPSyncQuery.cpp 357
V1044 Kondisi break loop tidak tergantung pada jumlah iterasi. mimemoz2.cpp 1795
void ResetChannelCharset(MimeObject *obj) { .... if (cSet) { char *ptr2 = cSet; while ((*cSet) && (*cSet != ' ') && (*cSet != ';') && (*cSet != '\r') && (*cSet != '\n') && (*cSet != '"')) ptr2++; if (*cSet) { PR_FREEIF(obj->options->default_charset); obj->options->default_charset = strdup(cSet); obj->options->override_charset = true; } PR_FREEIF(cSet); } .... }
Kesalahan ini ditemukan menggunakan diagnostik baru, yang akan tersedia di rilis selanjutnya dari penganalisa. Semua variabel yang digunakan dalam kondisi untuk menghentikan
loop sementara tidak dimodifikasi, karena variabel
ptr2 dan
cSet tercampur dalam tubuh fungsi.
netwerk
netwerk berisi antarmuka C dan kode untuk akses tingkat rendah ke jaringan (menggunakan soket dan cache file dan memori) serta akses tingkat lebih tinggi (menggunakan berbagai protokol seperti http, ftp, gopher, castanet). Kode ini juga dikenal dengan nama, "netlib" dan "Necko."
V501 Ada sub-ekspresi identik 'connectStarted' ke kiri dan ke kanan operator '&&'. nsSocketTransport2.cpp 1693
nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) {
Pada awalnya, saya berpikir bahwa menduplikasi variabel
connectStarted hanya kode berlebihan sampai saya melihat melalui seluruh fungsi yang cukup panjang dan menemukan sebuah fragmen yang serupa. Kemungkinan besar, alih-alih variabel tunggal
connectStarted di sini juga harus menjadi variabel
connectCalled .
V611 Memori dialokasikan menggunakan operator 'T baru] tetapi dirilis menggunakan operator' hapus '. Pertimbangkan untuk memeriksa kode ini. Mungkin lebih baik menggunakan 'delete [] mData;'. Periksa baris: 233, 222. DataChannel.cpp 233
BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length];
Pointer
mData menunjuk ke array, bukan objek tunggal. Mereka membuat kesalahan di destruktor kelas, lupa menambahkan tanda kurung untuk operator
hapus .
V1044 Kondisi break loop tidak tergantung pada jumlah iterasi. ParseFTPList.cpp 691
int ParseFTPList(....) { .... pos = toklen[2]; while (pos > (sizeof(result->fe_size) - 1)) pos = (sizeof(result->fe_size) - 1); memcpy(result->fe_size, tokens[2], pos); result->fe_size[pos] = '\0'; .... }
Nilai
pos ditimpa dalam loop dengan jumlah yang sama. Tampaknya diagnostik baru menemukan kesalahan lain.
gfx
gfx berisi antarmuka C dan kode untuk platform independent drawing and imaging. Dapat digunakan untuk menggambar persegi panjang, garis, gambar, dll. Pada dasarnya, ini adalah satu set antarmuka untuk konteks perangkat (platform) independen platform. Itu tidak menangani widget atau rutinitas menggambar tertentu; itu hanya memberikan operasi primitif untuk menggambar.
V501 Ada sub-ekspresi identik ke kiri dan ke kanan '||' operator: mVRSistem || mVRKomposisi || mVRSystem OpenVRSession.cpp 876
void OpenVRSession::Shutdown() { StopHapticTimer(); StopHapticThread(); if (mVRSystem || mVRCompositor || mVRSystem) { ::vr::VR_Shutdown(); mVRCompositor = nullptr; mVRChaperone = nullptr; mVRSystem = nullptr; } }
Dalam kondisi tersebut, variabel
mVRSystem hadir dua kali. Jelas, salah satunya harus diganti dengan
mVRChaperone .
dom
dom berisi antarmuka C dan kode untuk mengimplementasikan dan melacak objek DOM (Document Object Model) dalam Javascript. Ini membentuk substruktur C yang menciptakan, menghancurkan dan memanipulasi objek bawaan dan yang ditentukan pengguna sesuai dengan skrip Javascript.
V570 Variabel 'clonedDoc-> mPreloadReferrerInfo' ditugaskan untuk dirinya sendiri. Document.cpp 12049
already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
Analyzer mendeteksi penugasan variabel untuk dirinya sendiri.
xpcom
xpcom berisi antarmuka C tingkat rendah, kode C, kode C, sedikit kode rakitan dan alat baris perintah untuk mengimplementasikan mesin dasar komponen XPCOM (yang merupakan singkatan dari "Cross Platform Component Object Model"). XPCOM adalah mekanisme yang memungkinkan Mozilla untuk mengekspor antarmuka dan membuatnya tersedia secara otomatis untuk skrip JavaScript, Microsoft COM dan kode Mozilla C biasa.
V611 Memori dialokasikan menggunakan fungsi 'malloc / realloc' tetapi dirilis menggunakan operator 'hapus'. Pertimbangkan untuk memeriksa logika operasi di belakang variabel 'kunci'. Periksa baris: 143, 140. nsINIParser.h 143
struct INIValue { INIValue(const char* aKey, const char* aValue) : key(strdup(aKey)), value(strdup(aValue)) {} ~INIValue() { delete key; delete value; } void SetValue(const char* aValue) { delete value; value = strdup(aValue); } const char* key; const char* value; mozilla::UniquePtr<INIValue> next; };
Setelah memanggil fungsi
strdup ,
Anda perlu membebaskan memori menggunakan fungsi
bebas , bukan operator
hapus .
V716 Konversi tipe mencurigakan dalam inisialisasi: 'HRESULT var = BOOL'. SpecialSystemDirectory.cpp 73
BOOL SHGetSpecialFolderPathW( HWND hwnd, LPWSTR pszPath, int csidl, BOOL fCreate ); static nsresult GetWindowsFolder(int aFolder, nsIFile** aFile) { WCHAR path_orig[MAX_PATH + 3]; WCHAR* path = path_orig + 1; HRESULT result = SHGetSpecialFolderPathW(nullptr, path, aFolder, true); if (!SUCCEEDED(result)) { return NS_ERROR_FAILURE; } .... }
Fungsi
WinGI SHGetSpecialFolderPathW mengembalikan nilai tipe
BOOL , bukan
HRESULT . Memeriksa hasil fungsi harus ditulis ulang ke yang benar.
nsprpub
nsprpub berisi kode C untuk lintas platform "C" Runtime Library. Perpustakaan Runtime āCā berisi fungsi-fungsi C non-visual dasar untuk mengalokasikan dan membatalkan alokasi memori, mendapatkan waktu dan tanggal, membaca dan menulis file, menangani utas dan menangani serta membandingkan string di semua platform
V647 Nilai tipe 'int' ditugaskan ke pointer tipe 'pendek'. Pertimbangkan untuk memeriksa tugas: 'out_flags = 0x2'. prsocket.c 1220
#define PR_POLL_WRITE 0x2 static PRInt16 PR_CALLBACK SocketPoll( PRFileDesc *fd, PRInt16 in_flags, PRInt16 *out_flags) { *out_flags = 0; #if defined(_WIN64) if (in_flags & PR_POLL_WRITE) { if (fd->secret->alreadyConnected) { out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; } } #endif return in_flags; }
Analyzer mendeteksi penetapan konstanta numerik ke pointer
out_flags . Kemungkinan besar, mereka hanya lupa dereferensi padanya:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Kesimpulan
Ini bukan akhirnya. Jadilah ulasan kode baru. Kode Thunderbird dan Firefox mencakup dua perpustakaan utama: Layanan Keamanan Jaringan (NSS) dan WebRTC (Web Real Time Communications). Ada beberapa kesalahan yang sangat menarik. Dalam ulasan ini, saya akan tunjukkan satu per satu.
NssV597 Kompiler dapat menghapus pemanggilan fungsi 'memset', yang digunakan untuk membersihkan buffer 'newdeskey'. Fungsi RtlSecureZeroMemory () harus digunakan untuk menghapus data pribadi. pkcs11c.c 1033
static CK_RV sftk_CryptInit(....) { .... unsigned char newdeskey[24]; .... context->cipherInfo = DES_CreateContext( useNewKey ? newdeskey : (unsigned char *)att->attrib.pValue, (unsigned char *)pMechanism->pParameter, t, isEncrypt); if (useNewKey) memset(newdeskey, 0, sizeof newdeskey); sftk_FreeAttribute(att); .... }
NSS adalah perpustakaan untuk mengembangkan aplikasi klien dan server yang aman, dan di sini DES Key tidak dibersihkan. Kompiler akan menghapus panggilan
memset dari kode, sebagai array
newdeskey tidak lagi digunakan dalam kode di luar lokasi ini.
WebRTCV519 Variabel 'state [state_length - x_length + i]' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 83, 84. filter_ar.c 84
size_t WebRtcSpl_FilterAR(....) { .... for (i = 0; i < state_length - x_length; i++) { state[i] = state[i + x_length]; state_low[i] = state_low[i + x_length]; } for (i = 0; i < x_length; i++) { state[state_length - x_length + i] = filtered[i]; state[state_length - x_length + i] = filtered_low[i];
Pada siklus kedua, data ditulis ke array yang salah, karena penulis menyalin kode dan lupa untuk mengubah nama array dari
state ke
state_low .
Mungkin ada bug yang lebih menarik dalam proyek-proyek ini yang layak disebut. Dan kami akan melakukannya dalam waktu dekat. Sementara itu, cobalah
PVS-Studio di proyek Anda.
Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Svyatoslav Razmyslov.
Tema gelap Thunderbird sebagai alasan untuk menjalankan penganalisis kode .