Les aventures avec le client de messagerie Mozilla Thunderbird ont commencé avec la mise à jour automatique vers la version 68.0. Plus de texte dans les notifications contextuelles et le thème sombre par défaut sont les caractéristiques notables de cette version. Parfois, j'ai trouvé une erreur que j'avais immédiatement envie de détecter avec une analyse statique. C'est devenu la raison d'aller pour une autre vérification du code source du projet en utilisant PVS-Studio. Il se trouve qu'au moment de l'analyse, le bogue avait déjà été corrigé. Cependant, puisque nous avons prêté attention au projet, il n'y a aucune raison de ne pas écrire sur d'autres défauts trouvés.
Présentation
Le thème sombre de la nouvelle version de Thunderbird est joli. J'aime les thèmes sombres. Je les ai déjà passés dans des messagers, Windows, macOS. Bientôt, l'iPhone sera mis à jour vers iOS 13 avec un thème sombre. Pour cette raison, j'ai même dû changer mon iPhone 5S pour un modèle plus récent. Dans la pratique, il s'est avéré qu'un thème sombre nécessite plus d'efforts pour que les développeurs choisissent les couleurs de l'interface. Tout le monde ne peut pas le gérer la première fois.
Voici à quoi ressemblaient les balises standard dans Thunderbird après la mise à jour:
J'utilise normalement 6 balises (5 standard +1 personnalisées) pour baliser les e-mails. La moitié d'entre eux est devenu impossible à regarder après la mise à jour, j'ai donc décidé de changer la couleur des paramètres pour une plus lumineuse. À ce stade, je suis resté coincé avec un bug:
Vous ne pouvez pas changer la couleur d'une étiquette !!! Plus vrai, vous le pouvez, mais l'éditeur ne vous laissera pas l'enregistrer, se référant à un nom déjà existant (WTF ???).
Un autre symptôme de ce bogue est un bouton OK inactif. Comme je ne pouvais pas apporter de modifications dans la même étiquette de nom, j'ai essayé de changer son nom. Eh bien, il s'avère que vous ne pouvez pas le renommer non plus.
Enfin, vous avez peut-être remarqué que le thème sombre ne fonctionnait pas pour les paramètres, ce qui n'est pas très agréable non plus.
Après une longue lutte avec le système de construction de Windows, j'ai finalement construit Thunderbird à partir des fichiers source. La dernière version du client de messagerie s'est avérée bien meilleure que la nouvelle version. Dans ce document, le thème sombre est également entré dans les paramètres, et ce bug avec l'éditeur de balises a disparu. Néanmoins, pour s'assurer que la construction du projet ne soit pas seulement une perte de temps, l'analyseur de code statique
PVS-Studio s'est mis au travail.
Remarque Le code source de Thunderbird recoupe la base de code de Firefox par certains moyens. Par conséquent, l'analyse inclut des erreurs de différents composants, qui méritent d'être examinées de près par les développeurs de ces équipes.
Note 2. Pendant que j'écrivais l'article, Thunderbird 68.1 est sorti et ce bug a été corrigé:
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 chaîne d'en-
tête a été comparée deux fois avec la constante
HEADER_REPLY_TO . Peut-être qu'il aurait dû y avoir une autre constante à sa place.
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 -
en -
têtes . Comme toujours, il y a deux explications possibles: une vérification inutile 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)) {
Les touches Ctrl + C et Ctrl + V ont certainement contribué à accélérer l'écriture de cette cascade d'expressions conditionnelles. Par conséquent, l'une des branches ne sera 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 qu'il est toujours en cours d'écriture. On peut dire en toute sécurité que l'erreur apparaîtra une fois le code raffiné. Un programmeur peut changer le code commenté, mais n'en aura jamais le contrôle. Soyez très prudent et attentif 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(
Déréférencement possible du pointeur null de
ligne dans la ligne suivante:
morkCell* cell = row->CellAt(ev, pos);
Très probablement, un pointeur n'a pas été initialisé, par exemple, par 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. Ses différents bits représentent différents champs d'une description d'erreur. Vous devez définir le code d'erreur à l'aide de constantes spéciales des fichiers d'en-tête système.
Quelques fragments comme celui-ci:
- 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. Afin de réinitialiser les valeurs des structures, les auteurs ont utilisé la fonction
memset , mais ont transmis la taille du pointeur comme taille de l'espace mémoire.
Fragments suspects similaires:
- 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; } .... }
Le diagnostic
V595 détecte généralement les erreurs typiques de déréférencement de pointeur nul. Dans ce cas, nous avons un exemple extrêmement intéressant, qui mérite 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 réelle est différente. C'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 fragment 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 est détecté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 de la boucle while ne changent pas, car les variables
ptr2 et
cSet sont confondues 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) {
J'ai d'abord pensé que la duplication de la variable
connectStarted n'était qu'un code redondant. Mais j'ai ensuite parcouru toute la fonction assez longue et trouvé un fragment similaire. Très probablement, la variable
connectCalled doit être ici au lieu de la variable
connectStarted .
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. Une erreur a été commise dans le destructeur de classe en raison de crochets manquants 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 la variable
pos est réécrite dans la boucle pour la même valeur. Il semble que le nouveau diagnostic ait 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; } }
La variable
mVRSystem apparaît deux fois dans la condition
. De toute évidence, l'une de ses occurrences devrait être remplacée 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 trouvé l'affectation de la 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 , il faut libérer la mémoire en utilisant la fonction
libre , pas l'opérateur de
suppression .
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; } .... }
SHGetSpecialFolderPathW La fonction WinAPI renvoie la valeur du type
BOOL , pas
HRESULT . Il faut réécrire la vérification du résultat de la fonction sur 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'attribution d'une constante numérique au pointeur
out_flags . Très probablement, on a juste 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 encore la fin. Que les nouvelles révisions de code soient! Code Thunderbird et Firefox comprenant deux grandes bibliothèques: Network Security Services (NSS) et WebRTC (Web Real Time Communications). J'y ai trouvé des erreurs convaincantes. Dans cette revue, je vais en montrer un de chaque projet.
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. Alors que DES Key n'est pas effacé ici. Le compilateur supprimera l'appel
memset du code, car le tableau
newdeskey n'est plus utilisé nulle part dans le code.
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 la deuxième boucle, les données sont écrites dans le mauvais tableau, car l'auteur a copié le code et a oublié de modifier le nom du tableau d'
état pour
state_low .
Probablement, il y a encore des bogues intéressants dans ces projets, dont il faut parler. Et nous le ferons bientôt. En attendant, essayez
PVS-Studio sur votre projet.