L'aventure avec le client de messagerie Mozilla Thunderbird a commencé avec une mise à niveau automatique vers la version 68.0. Les caractéristiques notables de cette version étaient les suivantes: plus de texte est ajouté aux notifications contextuelles et un thème sombre par défaut. Il y avait une erreur que je voulais essayer de détecter à l'aide de l'analyse statique. Ce fut l'occasion de vérifier à nouveau le code source du projet à l'aide de PVS-Studio. Il s'est avéré qu'au moment de l'analyse, l'erreur avait déjà été corrigée. Mais puisque nous avons attiré l'attention sur ce projet, nous pouvons écrire sur d'autres défauts qui s'y trouvent.
Présentation
Le thème sombre de la nouvelle version de Thunderbird est plutôt joli. J'adore les thèmes sombres. Déjà passé à eux dans les messageries instantanées, Windows, macOS. Bientôt, l'iPhone passera à iOS 13, où un thème sombre est apparu. Pour cela, j'ai même dû changer mon iPhone 5S en un modèle plus récent. En pratique, il s'est avéré que le thème sombre nécessite plus d'efforts pour que les développeurs choisissent les couleurs de l'interface. Tout le monde n'y fait pas face la première fois.
Donc, mes balises standard dans Thunderbird ont commencé à ressembler à:
J'utilise six balises (5 standard + 1 personnalisée) pour marquer les e-mails. Il est devenu impossible de regarder la moitié d'entre eux après la mise à jour, et j'ai décidé de changer la couleur en plus clair dans les paramètres. Mais ici, je suis tombé sur un bug:
Vous ne pouvez pas changer la couleur du tag !!! Plus précisément, c'est possible, mais l'éditeur ne permettra pas de l'enregistrer, en se référant à un nom existant (WTF ???).
Une autre manifestation du bogue sera l'inaction du bouton OK, si vous essayez de changer le nom, car vous ne pouvez pas enregistrer ce nom. Vous ne pouvez pas renommer non plus.
Enfin, vous remarquerez peut-être que le thème sombre n'a pas touché les paramètres, ce qui n'est pas non plus très beau.
Après une longue lutte avec le système de construction de Windows, il était toujours possible d'assembler Thunderbird à partir de la source. La dernière version du client de messagerie s'est avérée bien meilleure que la dernière version. Dans ce document, un thème sombre est entré dans les paramètres, et ce bug avec l'éditeur de balises a également disparu. Mais pour que le travail d'assemblage du projet ne soit pas perdu, l'analyseur de code statique
PVS-Studio a été lancé.
Remarque Le code source de Thunderbird chevauche en quelque sorte la base de code de Firefox. Par conséquent, l'analyse a inclus des erreurs de différents composants, qui méritent un examen attentif des développeurs de différentes équipes.
Remarque 2 Pendant la rédaction de l'article, la mise à jour Thunderbird 68.1 est sortie avec un correctif pour ce bogue:
comm
comm-central est un référentiel Mercurial du code d'extension Thunderbird, SeaMonkey et Lightning.
V501 Il existe des sous-expressions identiques '(! Strcmp (en-tête, "Reply-To"))' à gauche et à droite de '||' opérateur. 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; .... }
La ligne d'en-
tête a été comparée deux fois avec la constante
HEADER_REPLY_TO . Peut-être qu'à sa place, il aurait dû y avoir une autre constante.
V501 Il existe des sous-expressions identiques «obj-> options-> headers! = MimeHeadersCitation» à gauche et à droite de l'opérateur «&&». 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) { .... } .... }
Une autre comparaison étrange d'une variable avec un nom similaire est celle des en-
têtes . Comme toujours, il y a deux explications possibles: une vérification supplémentaire ou une faute de frappe.
V517 L'utilisation du
modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifier les lignes: 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)) {
La cascade d'expressions conditionnelles a été clairement accélérée en appuyant sur Ctrl + C et Ctrl + V. Par conséquent, l'une des branches n'est jamais exécutée.
V517 L'utilisation du
modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifiez les lignes: 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) {
La première et la dernière condition sont les mêmes. Le code montre que le code est en cours d'écriture. Il est sûr de dire qu'avec une probabilité élevée, l'erreur se manifestera une fois le code finalisé. Un programmeur peut changer le code commenté, mais il n'en aura jamais le contrôle. Soyez prudent avec ce code.
V522 Le déréférencement de la «ligne» du pointeur nul peut avoir lieu. morkRowCellCursor.cpp 175
NS_IMETHODIMP morkRowCellCursor::MakeCell(
Le déréférencement du pointeur null de
ligne est possible sur la ligne suivante:
morkCell* cell = row->CellAt(ev, pos);
Très probablement, l'initialisation du pointeur a été ignorée avant cette ligne, par exemple, en utilisant la méthode
GetRow , etc.
V543 Il est étrange que la valeur '-1' soit affectée à la variable 'm_lastError' de type 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; } .... }
Le type
HRESULT est un type de données complexe. Différents bits d'une variable de ce type représentent différents champs de description d'erreur. Le code d'erreur doit être défini à l'aide de constantes spéciales des fichiers d'en-tête système.
Quelques autres de ces endroits:
- V543 Il est étrange que la valeur '-1' soit affectée à la variable 'm_lastError' de type HRESULT. MapiApi.cpp 817
- V543 Il est étrange que la valeur '-1' soit affectée à la variable 'm_lastError' de type HRESULT. MapiApi.cpp 1749
V579 La fonction memset reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le troisième 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 ); .... }
La variable
parts est un pointeur sur un tableau de structures. Pour réinitialiser les valeurs des structures, ils ont utilisé la fonction
memset , mais ils lui ont transféré la taille du pointeur comme la taille d'un morceau de mémoire.
Autres endroits suspects:
- V579 La fonction memset reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le troisième argument. icalmime.c 385
- V579 La fonction memset reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le troisième argument. icalparameter.c 114
- V579 La fonction snprintf reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le deuxième argument. icaltimezone.c 1908
- V579 La fonction snprintf reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le deuxième argument. icaltimezone.c 1910
- V579 La fonction strncmp reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le troisième argument. sspm.c 707
- V579 La fonction strncmp reçoit le pointeur et sa taille comme arguments. C'est peut-être une erreur. Inspectez le troisième argument. sspm.c 813
V595 Le pointeur 'aValues' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 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; } .... }
Diagnostics
V595 trouve généralement des erreurs de déréférencement de pointeur nul typiques. Mais on a trouvé un cas très intéressant, digne d'une attention particulière.
Techniquement, l'analyseur a raison de dire que le pointeur
aValues est d' abord déréférencé, puis vérifié, mais l'erreur est différente. Ceci est un double pointeur, donc le code correct devrait ressembler à ceci:
*aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
Un autre endroit très similaire:
- V595 Le pointeur '_retval' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 357, 358. nsLDAPSyncQuery.cpp 357
V1044 Les conditions d'interruption de boucle ne dépendent pas du nombre d'itérations. 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); } .... }
Cette erreur a été trouvée à l'aide d'un nouveau diagnostic, qui sera disponible dans la prochaine version de l'analyseur. Toutes les variables utilisées dans la condition d'arrêt de la
boucle while ne sont pas modifiées, car les variables
ptr2 et
cSet sont mélangées dans le corps de la fonction.
netwerk
netwerk contient des interfaces C et du code pour un accès de bas niveau au réseau (en utilisant des sockets et des caches de fichiers et de mémoire) ainsi qu'un accès de plus haut niveau (en utilisant divers protocoles tels que http, ftp, gopher, castanet). Ce code est également connu sous les noms de "netlib" et "Necko".
V501 Il existe des sous-expressions identiques «connectStarted» à gauche et à droite de l'opérateur «&&». nsSocketTransport2.cpp 1693
nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) {
Au début, je pensais que la duplication de la variable
connectStarted était juste du code superflu jusqu'à ce que je regarde toute la fonction suffisamment longue et que je trouve un fragment similaire. Très probablement, au lieu d'une seule variable
connectStarted ici devrait également être une variable
connectCalled .
V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il vaut probablement mieux utiliser 'delete [] mData;'. Vérifiez les lignes: 233, 222. DataChannel.cpp 233
BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length];
Le pointeur
mData pointe sur un tableau, pas sur un seul objet. Ils ont fait une erreur dans le destructeur de classe, oubliant d'ajouter des crochets pour l'opérateur de
suppression .
V1044 Les conditions d'interruption de boucle ne dépendent pas du nombre d'itérations. 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'; .... }
La valeur de
pos est écrasée dans la boucle du même montant. Il semble que les nouveaux diagnostics aient trouvé une autre erreur.
gfx
gfx contient des interfaces C et du code pour le dessin et l'imagerie indépendants de la plateforme. Il peut être utilisé pour dessiner des rectangles, des lignes, des images, etc. Il s'agit essentiellement d'un ensemble d'interfaces pour un contexte de périphérique (dessin) indépendant de la plate-forme. Il ne gère pas les widgets ou les routines de dessin spécifiques; il fournit simplement les opérations primitives de dessin.
V501 Il existe des sous-expressions identiques à gauche et à droite de '||' opérateur: mVRSystem || mVRCompositor || mVRSystem OpenVRSession.cpp 876
void OpenVRSession::Shutdown() { StopHapticTimer(); StopHapticThread(); if (mVRSystem || mVRCompositor || mVRSystem) { ::vr::VR_Shutdown(); mVRCompositor = nullptr; mVRChaperone = nullptr; mVRSystem = nullptr; } }
Dans la condition, la variable
mVRSystem est présente deux fois. De toute évidence, l'un d'eux devrait être remplacé par
mVRChaperone .
dom
dom contient des interfaces C et du code pour implémenter et suivre les objets DOM (Document Object Model) en Javascript. Il forme la sous-structure C qui crée, détruit et manipule des objets intégrés et définis par l'utilisateur selon le script Javascript.
V570 La variable 'clonedDoc-> mPreloadReferrerInfo' est assignée à elle-même. Document.cpp 12049
already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
L'analyseur a détecté l'affectation d'une variable à lui-même.
xpcom
xpcom contient les interfaces C de bas niveau, le code C, le code C, un peu de code d'assemblage et des outils de ligne de commande pour implémenter la machinerie de base des composants XPCOM (qui signifie "Cross Platform Component Object Model"). XPCOM est le mécanisme qui permet à Mozilla d'exporter des interfaces et de les mettre automatiquement à la disposition des scripts JavaScript, de Microsoft COM et du code Mozilla C normal.
V611 La mémoire a été allouée à l'aide de la fonction 'malloc / realloc' mais a été libérée à l'aide de l'opérateur 'delete'. Envisagez d'inspecter les logiques d'opération derrière la variable «clé». Vérifiez les lignes: 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; };
Après avoir appelé la fonction
strdup ,
vous devez libérer de la mémoire à l'aide de la fonction
free , et non de l'opérateur
delete .
V716 Conversion de type suspect lors de l'initialisation: '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; } .... }
La fonction
WinGI SHGetSpecialFolderPathW renvoie une valeur de type
BOOL , pas
HRESULT . La vérification du résultat de la fonction doit être réécrite dans la bonne.
nsprpub
nsprpub contient le code C pour la bibliothèque d'exécution multiplateforme "C". La bibliothèque d'exécution «C» contient des fonctions C non visuelles de base pour allouer et désallouer la mémoire, obtenir l'heure et la date, lire et écrire des fichiers, gérer les threads et gérer et comparer les chaînes sur toutes les plateformes
V647 La valeur de type "int" est affectée au pointeur de type "court". Pensez à inspecter l'affectation: '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; }
L'analyseur a détecté l'affectation d'une constante numérique au pointeur
out_flags . Très probablement, ils ont simplement oublié de le déréférencer:
if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
Conclusion
Ce n'est pas la fin. Soyez de nouvelles revues de code. Le code Thunderbird et Firefox comprend deux bibliothèques principales: les services de sécurité réseau (NSS) et WebRTC (Web Real Time Communications). Il y a eu des erreurs très intéressantes. Dans cette revue, je vais en montrer un à la fois.
NssV597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'newdeskey'. La fonction RtlSecureZeroMemory () doit être utilisée pour effacer les données privées. 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 est une bibliothèque pour développer des applications client et serveur sécurisées, et ici la clé DES n'est pas nettoyée. Le compilateur supprimera l'appel
memset du code, comme le tableau
newdeskey n'est plus utilisé dans le code au-delà de cet emplacement.
WebRTCV519 La
variable 'state [state_length - x_length + i]' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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];
Dans le deuxième cycle, les données sont écrites dans le mauvais tableau, car l'auteur a copié le code et a oublié de changer le nom du tableau de
state en
state_low .
Il y a probablement des bogues plus intéressants dans ces projets qui méritent d'être mentionnés. Et nous le ferons dans un proche avenir. En attendant, essayez
PVS-Studio sur votre projet.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Svyatoslav Razmyslov.
Thème sombre de Thunderbird comme raison d'exécuter un analyseur de code .