Das „Abenteuer“ mit dem Mozilla Thunderbird-E-Mail-Client begann mit einem automatischen Upgrade auf Version 68.0. Bemerkenswerte Merkmale dieser Version waren: Popup-Benachrichtigungen und einem dunklen Thema werden standardmäßig mehr Text hinzugefügt. Es gab einen Fehler, den ich mithilfe einer statischen Analyse erkennen wollte. Dies war eine Gelegenheit, den Quellcode des Projekts erneut mit PVS-Studio zu überprüfen. Es stellte sich heraus, dass der Fehler zum Zeitpunkt der Analyse bereits behoben war. Da wir jedoch auf dieses Projekt aufmerksam gemacht haben, können wir über andere darin festgestellte Mängel schreiben.
Einführung
Das dunkle Thema der neuen Version von Thunderbird sieht ziemlich hübsch aus. Ich liebe dunkle Themen. Bereits in Instant Messenger, Windows, MacOS auf sie umgestellt. In Kürze wird das iPhone auf iOS 13 aktualisiert, wo ein dunkles Thema angezeigt wurde. Dafür musste ich sogar mein iPhone 5S auf ein neueres Modell umstellen. In der Praxis stellte sich heraus, dass das dunkle Thema für Entwickler mehr Aufwand erfordert, um die Farben der Benutzeroberfläche auszuwählen. Nicht jeder kommt beim ersten Mal damit klar.
Meine Standard-Tags in Thunderbird sahen also so aus:
Ich benutze sechs Tags (5 Standard + 1 benutzerdefinierte), um E-Mails zu markieren. Es wurde unmöglich, die Hälfte von ihnen nach dem Update zu sehen, und ich beschloss, die Farbe in den Einstellungen heller zu ändern. Aber hier bin ich auf einen Fehler gestoßen:
Sie können die Farbe des Tags nicht ändern !!! Genauer gesagt ist es möglich, aber der Editor lässt nicht zu, dass es unter Bezugnahme auf einen vorhandenen Namen (WTF ???) gespeichert wird.
Eine weitere Manifestation des Fehlers ist die Untätigkeit der Schaltfläche "OK", wenn Sie versuchen, den Namen zu ändern, da Sie nicht mit diesem Namen speichern können. Sie können auch nicht umbenennen.
Schließlich können Sie feststellen, dass das dunkle Thema die Einstellungen nicht berührt hat, was auch nicht sehr schön ist.
Nach einem langen Kampf mit dem Build-System in Windows war es immer noch möglich, Thunderbird aus dem Quellcode zusammenzusetzen. Die neueste Version des Mail-Clients erwies sich als viel besser als die neueste Version. Darin kam ein dunkles Thema zu den Einstellungen, und dieser Fehler mit dem Tag-Editor verschwand ebenfalls. Damit die Montagearbeiten des Projekts nicht verschwendet werden, wurde der statische Code-Analysator
PVS-Studio gestartet.
Hinweis Der Thunderbird-Quellcode überschneidet sich irgendwie mit der Firefox-Codebasis. Daher umfasste die Analyse Fehler von verschiedenen Komponenten, die einen genauen Blick auf die Entwickler verschiedener Teams wert sind.
Anmerkung 2 Während der Artikel geschrieben wurde, wurde im Thunderbird 68.1-Update ein Fehler behoben:
comm
comm-central ist ein Mercurial-Repository des Thunderbird-, SeaMonkey- und Lightning-Erweiterungscodes.
V501 Links und rechts vom '||' befinden sich identische Unterausdrücke '(! Strcmp (Header, "Reply-To"))'. Betreiber. 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; .... }
Die
Kopfzeile wurde zweimal mit der Konstanten
HEADER_REPLY_TO verglichen. Vielleicht hätte es an seiner Stelle eine andere Konstante geben sollen.
V501 Links und rechts vom Operator '&&' befinden sich identische Unterausdrücke 'obj-> options-> headers! = MimeHeadersCitation'. 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) { .... } .... }
Ein weiterer seltsamer Vergleich einer Variablen mit einem ähnlichen Namen sind
Header . Wie immer gibt es zwei mögliche Erklärungen: einen zusätzlichen Scheck oder einen Tippfehler.
V517 Die Verwendung des
Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. Überprüfen Sie die Zeilen: 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)) {
Die Kaskade der bedingten Ausdrücke wurde durch Drücken von Strg + C und Strg + V deutlich beschleunigt. Infolgedessen wird einer der Zweige niemals ausgeführt.
V517 Die Verwendung des
Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. Überprüfen Sie die Zeilen: 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) {
Die erste und letzte Bedingung sind gleich. Der Code zeigt an, dass der Code gerade geschrieben wird. Man kann mit Sicherheit sagen, dass sich der Fehler mit hoher Wahrscheinlichkeit nach Abschluss des Codes manifestiert. Ein Programmierer kann den kommentierten Code ändern, erhält jedoch niemals die Kontrolle. Seien Sie vorsichtig und vorsichtig mit diesem Code.
V522 Es kann zu einer Dereferenzierung der Nullzeigerzeile kommen. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Die Dereferenzierung des
Zeilennullzeigers ist in der folgenden Zeile möglich:
morkCell* cell = row->CellAt(ev, pos);
Höchstwahrscheinlich wurde die Zeigerinitialisierung vor dieser Zeile übersprungen, z. B. mithilfe der
GetRow- Methode usw.
V543 Es ist merkwürdig, dass der Variablen 'm_lastError' vom Typ HRESULT der Wert '-1' zugewiesen wird. 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; } .... }
Der
HRESULT- Typ ist ein komplexer Datentyp. Unterschiedliche Bits einer Variablen dieses Typs repräsentieren unterschiedliche Fehlerbeschreibungsfelder. Der Fehlercode muss mithilfe spezieller Konstanten aus den Systemheaderdateien festgelegt werden.
Noch ein paar dieser Orte:
- V543 Es ist merkwürdig, dass der Variablen 'm_lastError' vom Typ HRESULT der Wert '-1' zugewiesen wird. MapiApi.cpp 817
- V543 Es ist merkwürdig, dass der Variablen 'm_lastError' vom Typ HRESULT der Wert '-1' zugewiesen wird. MapiApi.cpp 1749
V579 Die Memset-Funktion empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. 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 ); .... }
Die Teilevariable ist ein Zeiger auf ein Array von Strukturen. Um die Werte von Strukturen zurückzusetzen, verwendeten sie die
Memset- Funktion, übertrugen jedoch die Größe des Zeigers als Größe eines Speicherstücks darauf.
Andere verdächtige Orte:
- V579 Die Memset-Funktion empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. icalmime.c 385
- V579 Die Memset-Funktion empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. icalparameter.c 114
- V579 Die Funktion snprintf empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das zweite Argument. icaltimezone.c 1908
- V579 Die Funktion snprintf empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das zweite Argument. icaltimezone.c 1910
- V579 Die Funktion strncmp empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. sspm.c 707
- V579 Die Funktion strncmp empfängt den Zeiger und seine Größe als Argumente. Es ist möglicherweise ein Fehler. Untersuchen Sie das dritte Argument. sspm.c 813
V595 Der Zeiger 'aValues' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 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; } .... }
Diagnose
V595 findet normalerweise typische Nullzeiger-Dereferenzierungsfehler. Es wurde jedoch ein sehr interessanter Fall gefunden, der besondere Aufmerksamkeit verdient.
Technisch gesehen hat der Analysator Recht, dass der
aValues- Zeiger zuerst dereferenziert und dann überprüft wird, aber der Fehler ist anders. Dies ist ein Doppelzeiger, daher sollte der richtige Code folgendermaßen aussehen:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Ein weiterer sehr ähnlicher Ort:
- V595 Der Zeiger '_retval' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 357, 358. nsLDAPSyncQuery.cpp 357
V1044 Schleifenunterbrechungsbedingungen hängen nicht von der Anzahl der Iterationen ab. 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); } .... }
Dieser Fehler wurde mithilfe der neuen Diagnose festgestellt, die in der nächsten Version des Analysators verfügbar sein wird. Alle Variablen, die in der Bedingung zum Stoppen der
while-Schleife verwendet werden, werden nicht geändert, da die Variablen
ptr2 und
cSet im Funktionskörper
verwechselt werden.
netwerk
netwerk enthält C-Schnittstellen und Code für den Zugriff auf das Netzwerk auf niedriger Ebene (mithilfe von Sockets und Datei- und Speichercaches) sowie auf Zugriff auf höherer Ebene (mithilfe verschiedener Protokolle wie http, ftp, gopher, castanet). Dieser Code ist auch unter den Namen "netlib" und "Necko" bekannt.
V501 Links und rechts vom Operator '&&' befinden sich identische Unterausdrücke 'connectStarted'. nsSocketTransport2.cpp 1693
nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) {
Zuerst dachte ich, dass das Duplizieren der Variablen
connectStarted nur überflüssiger Code ist, bis ich die gesamte ausreichend lange Funktion durchgesehen und ein ähnliches Fragment gefunden habe. Anstelle einer einzelnen Variablen
connectStarted sollte hier höchstwahrscheinlich auch eine Variable
connectCalled verwendet werden .
V611 Der Speicher wurde mit dem Operator 'new T []' zugewiesen, aber mit dem Operator 'delete' freigegeben. Überprüfen Sie diesen Code. Es ist wahrscheinlich besser, 'delete [] mData;' zu verwenden. Überprüfen Sie die Zeilen: 233, 222. DataChannel.cpp 233
BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length];
Der
mData- Zeiger zeigt auf ein Array, nicht auf ein einzelnes Objekt. Sie haben einen Fehler im Klassendestruktor gemacht und vergessen, Klammern für den
Löschoperator hinzuzufügen.
V1044 Schleifenunterbrechungsbedingungen hängen nicht von der Anzahl der Iterationen ab. 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'; .... }
Der Wert von
pos wird in der Schleife um den gleichen Betrag überschrieben. Es scheint, dass die neue Diagnose einen weiteren Fehler gefunden hat.
gfx
gfx enthält C-Schnittstellen und Code für plattformunabhängiges Zeichnen und Imaging. Es kann verwendet werden, um Rechtecke, Linien, Bilder usw. zu zeichnen. Im Wesentlichen handelt es sich um eine Reihe von Schnittstellen für einen plattformunabhängigen Gerätekontext (Zeichnungskontext). Widgets oder bestimmte Zeichenroutinen werden nicht verarbeitet. Es werden nur die primitiven Operationen zum Zeichnen bereitgestellt.
V501 Links und rechts vom '||' befinden sich identische Unterausdrücke. Operator: mVRSystem || mVRCompositor || mVRSystem OpenVRSession.cpp 876
void OpenVRSession::Shutdown() { StopHapticTimer(); StopHapticThread(); if (mVRSystem || mVRCompositor || mVRSystem) { ::vr::VR_Shutdown(); mVRCompositor = nullptr; mVRChaperone = nullptr; mVRSystem = nullptr; } }
In der Bedingung ist die Variable
mVRSystem zweimal vorhanden. Offensichtlich sollte einer von ihnen durch
mVRChaperon ersetzt werden .
dom
dom enthält C-Schnittstellen und Code zum Implementieren und Verfolgen von DOM-Objekten (Document Object Model) in Javascript. Es bildet die C-Unterstruktur, die integrierte und benutzerdefinierte Objekte gemäß dem Javascript-Skript erstellt, zerstört und bearbeitet.
V570 Die Variable 'clonedDoc-> mPreloadReferrerInfo' wird sich selbst zugewiesen. Document.cpp 12049
already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
Der Analysator hat die Zuordnung einer Variablen zu sich selbst erkannt.
xpcom
xpcom enthält die C-Schnittstellen auf niedriger Ebene, C-Code, C-Code, ein bisschen Assembly-Code und Befehlszeilen-Tools zum Implementieren der grundlegenden Maschinerie von XPCOM-Komponenten (was für "Platform Component Component Object Model" steht). XPCOM ist der Mechanismus, mit dem Mozilla Schnittstellen exportieren und automatisch für JavaScript-Skripte, Microsoft COM und regulären Mozilla C-Code verfügbar machen kann.
V611 Der Speicher wurde mit der Funktion 'malloc / realloc' zugewiesen, aber mit dem Operator 'delete' freigegeben. Überprüfen Sie die Betriebslogik hinter der Variablen 'key'. Überprüfen Sie die Zeilen: 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; };
Nach dem Aufrufen der Funktion
strdup müssen
Sie Speicher mit der Funktion
free freigeben , nicht mit dem Operator
delete .
V716 Verdächtige Typkonvertierung bei der Initialisierung: '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; } .... }
Die
WinGI SHGetSpecialFolderPathW- Funktion gibt einen Wert vom Typ
BOOL zurück , nicht
HRESULT . Überprüfen Sie, ob das Ergebnis der Funktion auf das richtige umgeschrieben werden muss.
nsprpub
nsprpub enthält C-Code für die plattformübergreifende Laufzeitbibliothek "C". Die C-Laufzeitbibliothek enthält grundlegende nicht visuelle C-Funktionen zum Zuweisen und Freigeben von Speicher, Abrufen von Uhrzeit und Datum, Lesen und Schreiben von Dateien, Behandeln von Threads sowie Behandeln und Vergleichen von Zeichenfolgen auf allen Plattformen
V647 Der Wert vom Typ 'int' wird dem Zeiger vom Typ 'short' zugewiesen. Überprüfen Sie die Zuordnung: '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; }
Der Analysator hat die Zuordnung einer numerischen Konstante zum Zeiger
out_flags festgestellt . Höchstwahrscheinlich haben sie einfach vergessen, ihn zu dereferenzieren:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Fazit
Dies ist nicht das Ende. Seien Sie neue Code-Bewertungen. Der Thunderbird- und Firefox-Code enthält zwei Hauptbibliotheken: Network Security Services (NSS) und WebRTC (Web Real Time Communications). Es gab einige sehr interessante Fehler. In dieser Rezension werde ich eine nach der anderen zeigen.
NssV597 Der Compiler könnte den Funktionsaufruf 'memset' löschen, mit dem der 'newdeskey'-Puffer geleert wird. Die Funktion RtlSecureZeroMemory () sollte verwendet werden, um die privaten Daten zu löschen. 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 ist eine Bibliothek zum Entwickeln sicherer Client- und Serveranwendungen, und hier wird DES Key nicht bereinigt. Der Compiler entfernt den
Memset- Aufruf aus dem Code als Das
newdeskey- Array
wird in Code außerhalb dieses Speicherorts nicht mehr verwendet.
WebRTCV519 Der
Variablen 'state [state_length - x_length + i]' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 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];
Im zweiten Zyklus werden Daten in das falsche Array geschrieben, da der Autor den Code kopiert und vergessen hat, den Namen des Arrays von
state in
state_low zu
ändern .
Es gibt wahrscheinlich interessantere Fehler in diesen Projekten, die es wert sind, erwähnt zu werden. Und wir werden es in naher Zukunft tun.
Probieren Sie in der Zwischenzeit
PVS-Studio für Ihr Projekt aus.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Svyatoslav Razmyslov.
Dunkles Thema von Thunderbird als Grund, einen Code-Analysator auszuführen .