Die Abenteuer mit dem Mozilla Thunderbird Mail-Client begannen mit dem automatischen Update auf Version 68.0. Mehr Text in Popup-Benachrichtigungen und ein dunkles Standardthema sind die bemerkenswerten Merkmale dieser Version. Gelegentlich fand ich einen Fehler, den ich sofort mit statischer Analyse erkennen wollte. Dies wurde zum Grund für eine weitere Überprüfung des Projektquellcodes mit PVS-Studio. So kam es, dass der Fehler zum Zeitpunkt der Analyse bereits behoben war. Da wir dem Projekt jedoch einige Aufmerksamkeit geschenkt haben, gibt es keinen Grund, nicht über andere festgestellte Mängel zu schreiben.
Einführung
Das dunkle Thema der neuen Thunderbird-Version sieht hübsch aus. Ich mag dunkle Themen. Ich habe bereits in Messenger, Windows, MacOS zu ihnen gewechselt. Bald wird das iPhone mit einem dunklen Thema auf iOS 13 aktualisiert. Aus diesem Grund musste ich sogar mein iPhone 5S gegen ein neueres Modell austauschen. In der Praxis stellte sich heraus, dass ein dunkles Thema für Entwickler mehr Aufwand erfordert, um die Farben der Benutzeroberfläche zu erfassen. Nicht jeder kann beim ersten Mal damit umgehen.
So sahen Standard-Tags in Thunderbird nach dem Update aus:
Normalerweise verwende ich 6 Tags (5 Standard +1 benutzerdefinierte), um E-Mails zu markieren. Die Hälfte von ihnen war nach dem Update nicht mehr zu sehen, daher habe ich beschlossen, die Farbe in den Einstellungen in eine hellere zu ändern. Zu diesem Zeitpunkt hatte ich einen Fehler:
Sie können eine Tag-Farbe nicht ändern !!! Sie können es zwar, aber der Editor lässt Sie nicht speichern und verweist auf einen bereits vorhandenen Namen (WTF ???).
Ein weiteres Symptom für diesen Fehler ist eine inaktive OK-Schaltfläche. Da ich im selben Namensschild keine Änderungen vornehmen konnte, habe ich versucht, den Namen zu ändern. Nun, es stellt sich heraus, dass Sie es auch nicht umbenennen können.
Schließlich haben Sie vielleicht bemerkt, dass das dunkle Thema für die Einstellungen nicht funktioniert hat, was auch nicht sehr schön ist.
Nach einem langen Kampf mit dem Build-System in Windows habe ich Thunderbird schließlich aus den Quelldateien erstellt. Die neueste Version des Mail-Clients erwies sich als viel besser als die neue Version. Darin erreichte auch das dunkle Thema die Einstellungen, und dieser Fehler mit dem Tags-Editor verschwand. Um sicherzustellen, dass das Erstellen des Projekts nicht nur Zeitverschwendung ist, hat sich der statische Code-Analysator
PVS-Studio an die Arbeit gemacht.
Hinweis Der Quellcode von Thunderbird überschneidet sich in gewisser Weise mit der Firefox-Codebasis. Daher enthält die Analyse Fehler von verschiedenen Komponenten, die von den Entwicklern dieser Teams genau betrachtet werden sollten.
Hinweis 2. Während ich den Artikel schrieb, wurde Thunderbird 68.1 veröffentlicht und dieser Fehler wurde 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
Header- Zeichenfolge wurde zweimal mit der Konstante
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 -
Header . Wie immer gibt es zwei mögliche Erklärungen: eine unnötige Überprüfung 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 Tasten Strg + C und Strg + V haben definitiv dazu beigetragen, das Schreiben dieser Kaskade von bedingten Ausdrücken zu beschleunigen. 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, dass er noch geschrieben wird. Man kann mit Sicherheit sagen, dass sich der Fehler nach der Verfeinerung des Codes zeigt. Ein Programmierer kann den kommentierten Code ändern, erhält jedoch niemals die Kontrolle darüber. Bitte seien Sie sehr vorsichtig und aufmerksam mit diesem Code.
V522 Es kann zu einer Dereferenzierung der Nullzeigerzeile kommen. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Mögliche Dereferenzierung des
Zeilennullzeigers in der folgenden Zeile:
morkCell* cell = row->CellAt(ev, pos);
Höchstwahrscheinlich wurde ein Zeiger beispielsweise durch die
GetRow- Methode usw. nicht initialisiert.
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. Seine verschiedenen Bits repräsentieren verschiedene Felder einer Fehlerbeschreibung. Sie müssen den Fehlercode mithilfe spezieller Konstanten aus Systemheaderdateien festlegen.
Ein paar Fragmente wie dieses:
- 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 der Strukturen zurückzusetzen, verwendeten die Autoren die
Memset- Funktion, übergaben jedoch die
Zeigergröße als Größe des Speicherplatzes.
Ähnliche verdächtige Fragmente:
- 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; } .... }
Die
V595- Diagnose erkennt normalerweise typische Fehler der Nullzeiger-Dereferenzierung. In diesem Fall haben wir ein äußerst interessantes Beispiel, das besondere Aufmerksamkeit verdient.
Technisch ist der Analysator korrekt, dass der
aValues- Zeiger zuerst dereferenziert und dann überprüft wird, aber der tatsächliche Fehler ist unterschiedlich. Es ist ein Doppelzeiger, daher sollte der richtige Code wie folgt aussehen:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Ein weiteres ähnliches Fragment:
- 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 wird mithilfe einer neuen Diagnose gefunden, die in der nächsten Version des Analysators verfügbar sein wird. Alle Variablen, die in der Bedingung der
while- Schleife verwendet werden, ändern sich nicht, da die Variablen
ptr2 und
cSet im Hauptteil der Funktion 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 redundanter Code ist. Aber dann habe ich die ganze ziemlich lange Funktion durchgesehen und ein ähnliches Fragment gefunden. Höchstwahrscheinlich muss die Variable
connectCalled hier anstelle der Variablen
connectStarted sein .
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. Im Klassendestruktor ist ein Fehler aufgetreten, da für den
Löschoperator Klammern fehlen.
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 der Variablen
pos wird in der Schleife für denselben Wert neu geschrieben. Anscheinend hat die neue Diagnose einen weiteren Fehler gefunden.
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; } }
Die Variable
mVRSystem wird in der Bedingung zweimal
angezeigt . Offensichtlich sollte eines seiner Vorkommen 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 der Variablen zu sich selbst festgestellt.
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 Aufruf der Funktion
strdup muss der Speicher mit der
freien Funktion freigegeben werden, nicht mit dem
Löschoperator .
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; } .... }
SHGetSpecialFolderPathW Die WinAPI-Funktion gibt den Wert des
BOOL- Typs zurück, nicht
HRESULT . Man muss die Überprüfung des Funktionsergebnisses auf das richtige umschreiben.
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 festgestellt, dass dem Zeiger
out_flags eine numerische Konstante
zugewiesen wurde . Höchstwahrscheinlich hat man gerade vergessen, es zu dereferenzieren:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Fazit
Es ist noch nicht das Ende. Lassen Sie neue Code-Bewertungen sein! Thunderbird- und Firefox-Code mit zwei großen Bibliotheken: Network Security Services (NSS) und WebRTC (Web Real Time Communications). Ich habe dort einige überzeugende Fehler gefunden. In dieser Rezension zeige ich eine aus jedem Projekt.
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. Während DES Key hier nicht gelöscht wird. Der Compiler löscht den
Memset- Aufruf aus dem Code, da das
newdeskey- Array nirgendwo im Code weiter verwendet wird.
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];
In der zweiten Schleife werden Daten in das falsche Array geschrieben, da der Autor den Code kopiert und vergessen hat, den Namen des
Status- Arrays für
state_low zu
ändern .
Wahrscheinlich gibt es in diesen Projekten immer noch interessante Fehler, über die berichtet werden sollte. Und wir werden es bald tun.
Probieren Sie in der Zwischenzeit
PVS-Studio für Ihr Projekt aus.