Because of the dark theme, Thunderbird had to run a code analyzer

The “adventure” with the Mozilla Thunderbird email client began with an automatic upgrade to version 68.0. Notable features of this version were this: more text is added to pop-up notifications and a dark theme by default. There was an error that I wanted to try to detect using static analysis. This was an occasion to once again verify the source code of the project using PVS-Studio. It turned out that by the time of analysis, the error had already been fixed. But since we drew attention to this project, we can write about other defects found in it.

Introduction

The dark theme of the new version of Thunderbird looks pretty pretty. I love dark themes. Already switched to them in instant messengers, Windows, macOS. Soon, the iPhone will upgrade to iOS 13, where a dark theme has appeared. For this, I even had to change my iPhone 5S to a newer model. In practice, it turned out that the dark theme requires more effort for developers to choose the colors of the interface. Not everyone copes with this the first time. So my standard tags in Thunderbird began to look like:

I use six tags (5 standard + 1 custom) to mark emails. It became impossible to watch half of them after the update, and I decided to change the color to brighter in the settings. But then I ran into a bug:

You cannot change the color of the tag !!! More precisely, it is possible, but the editor will not allow it to be saved, referring to an existing name (WTF ???).



Another manifestation of the bug will be the inaction of the OK button, if you try to change the name, since this name can not be saved. You cannot rename either.



Finally, you may notice that the dark theme did not touch the settings, which is also not very beautiful.



After a long struggle with the build system in Windows, it was still possible to assemble Thunderbird from the source. The latest version of the mail client turned out to be much better than the latest release. In it, a dark theme got to the settings, and this bug with the tag editor also disappeared. But so that the efforts to build the project would not be wasted, the PVS-Studio static code analyzer was launched.



Note Thunderbird source code somehow overlaps with the Firefox code base. Therefore, the analysis included errors from different components, which are worth a close look at the developers of different teams.



Note 2 While the article was being written, the Thunderbird 68.1 update came out with a fix for this bug:

comm

comm-central is a Mercurial repository of the Thunderbird, SeaMonkey, and Lightning extension code.



V501 There are identical sub-expressions '(! Strcmp (header, "Reply-To"))' to the left and to the right of the '||' 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; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The header line was compared with the constant HEADER_REPLY_TO twice. Perhaps in its place there should have been another constant.



V501 There are identical sub-expressions 'obj-> options-> headers! = MimeHeadersCitation' to the left and to the right of the '&&' 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) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Another strange comparison of a variable with a similar name is headers . As always, there are two possible explanations: an extra check or a typo.



V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 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)) { // <= MAPI_TRACE1("%s {Error retrieving property}\n", pTag); } else if (pVal && (PROP_TYPE(pVal->ulPropTag) == PT_ERROR)) { // <= MAPI_TRACE1("%s {Error retrieving property}\n", pTag); } else { MAPI_TRACE1("%s invalid value, expecting long\n", pTag); } if (pVal) MAPIFreeBuffer(pVal); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The cascade of conditional expressions was clearly accelerated by pressing Ctrl + C and Ctrl + V. As a result, one of the branches is never executed.



V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 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) { // XXX we don't deal with aboutEach... //MOZ_LOG(gLog, LogLevel::Warning, // ("rdfxml: ignoring aboutEach at line %d", // aNode.GetSourceLineNumber())); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The first and last condition are the same. The code shows that the code is in the process of writing. It is safe to say that with a high probability the error will manifest itself after the code is finalized. The programmer can change the commented code, but he will never get control. Be careful and careful with this code.



V522 Dereferencing of the null pointer 'row' might take place. morkRowCellCursor.cpp 175

 NS_IMETHODIMP morkRowCellCursor::MakeCell( // get cell at current pos in the row nsIMdbEnv* mev, // context mdb_column* outColumn, // column for this particular cell mdb_pos* outPos, // position of cell in row sequence nsIMdbCell** acqCell) { nsresult outErr = NS_OK; nsIMdbCell* outCell = 0; mdb_pos pos = 0; mdb_column col = 0; morkRow* row = 0; morkEnv* ev = morkEnv::FromMdbEnv(mev); if (ev) { pos = mCursor_Pos; morkCell* cell = row->CellAt(ev, pos); if (cell) { col = cell->GetColumn(); outCell = row->AcquireCellHandle(ev, cell, col, pos); } outErr = ev->AsErr(); } if (acqCell) *acqCell = outCell; if (outPos) *outPos = pos; if (outColumn) *outColumn = col; return outErr; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The dereferencing of the row null pointer is possible on the following line:

 morkCell* cell = row->CellAt(ev, pos);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Most likely, pointer initialization was skipped before this line, for example, using the GetRow method, etc.



V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type. 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; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The HRESULT type is a complex data type. Different bits of a variable of this type represent different error description fields. The error code must be set using special constants from the system header files.



A couple more of these places:

• V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type. MapiApi.cpp 817
• V543 It is odd that value '-1' is assigned to the variable 'm_lastError' of HRESULT type. MapiApi.cpp 1749

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third 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, /* Max parts */ icalmime_local_action_map, /* Actions */ get_string, data, /* data for get_string*/ 0 /* First header */); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The parts variable is a pointer to an array of structures. To reset the values ​​of structures, they used the memset function, but they transferred the size of the pointer to it as the size of a piece of memory.



Other suspicious places:

• V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. icalmime.c 385
• V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. icalparameter.c 114
• V579 The snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. icaltimezone.c 1908
• V579 The snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. icaltimezone.c 1910
• V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. sspm.c 707
• V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. sspm.c 813

V595 The 'aValues' pointer was utilized before it was verified against nullptr. Check lines: 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 typically finds typical null pointer dereferencing errors. But there was found a very interesting case, worthy of special attention.



Technically, the analyzer is right that the aValues pointer is first dereferenced, then checked, but the error is different. This is a double pointer, so the correct code should look like this:

 *aValues = static_cast<nsILDAPBERValue **>( moz_xmalloc(numVals * sizeof(nsILDAPBERValue))); if (!*aValues) { ldap_value_free_len(values); return NS_ERROR_OUT_OF_MEMORY; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Another very similar place:

• V595 The '_retval' pointer was utilized before it was verified against nullptr. Check lines: 357, 358. nsLDAPSyncQuery.cpp 357

V1044 Loop break conditions do not depend on the number of iterations. 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); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

This error was found using a new diagnostic, which will be available in the next release of the analyzer. All the variables used in the condition for stopping the while loop are not modified, because the ptr2 and cSet variables are mixed up in the body of the function.

netwerk

netwerk contains C interfaces and code for low-level access to the network (using sockets and file and memory caches) as well as higher-level access (using various protocols such as http, ftp, gopher, castanet). This code is also known by the names, "netlib" and "Necko."



V501 There are identical sub-expressions 'connectStarted' to the left and to the right of the '&&' operator. nsSocketTransport2.cpp 1693

 nsresult nsSocketTransport::InitiateSocket() { .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectCalled) { // <= good, line 1630 SendPRBlockingTelemetry( connectStarted, Telemetry::PRCONNECT_BLOCKING_TIME_NORMAL, Telemetry::PRCONNECT_BLOCKING_TIME_SHUTDOWN, Telemetry::PRCONNECT_BLOCKING_TIME_CONNECTIVITY_CHANGE, Telemetry::PRCONNECT_BLOCKING_TIME_LINK_CHANGE, Telemetry::PRCONNECT_BLOCKING_TIME_OFFLINE); } .... if (gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase() && connectStarted && connectStarted) { // <= fail, line 1694 SendPRBlockingTelemetry( connectStarted, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_NORMAL, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_SHUTDOWN, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_CONNECTIVITY_CHANGE, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_LINK_CHANGE, Telemetry::PRCONNECT_FAIL_BLOCKING_TIME_OFFLINE); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

At first, I thought that duplicating the connectStarted variable is just superfluous code until I looked through the entire sufficiently long function and found a similar fragment. Most likely, instead of a single variable connectStarted here should also be a variable connectCalled .



V611 The memory was allocated using 'new T []' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mData;'. Check lines: 233, 222. DataChannel.cpp 233

 BufferedOutgoingMsg::BufferedOutgoingMsg(OutgoingMsg& msg) { size_t length = msg.GetLeft(); auto* tmp = new uint8_t[length]; // infallible malloc! memcpy(tmp, msg.GetData(), length); mLength = length; mData = tmp; mInfo = new sctp_sendv_spa; *mInfo = msg.GetInfo(); mPos = 0; } BufferedOutgoingMsg::~BufferedOutgoingMsg() { delete mInfo; delete mData; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The mData pointer points to an array, not a single object. They made a mistake in the class destructor, forgetting to add square brackets for the delete operator.



V1044 Loop break conditions do not depend on the number of iterations. 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'; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The value of pos is overwritten in the loop by the same amount. It seems that the new diagnostics found another mistake.

gfx

gfx contains C interfaces and code for platform independent drawing and imaging. It can be used to draw rectangles, lines, images, etc. Essentially, it is aa set of interfaces for a platform-independent device (drawing) context. It does not handle widgets or specific drawing routines; it just provides the primitive operations for drawing.



V501 There are identical sub-expressions to the left and to the right of the '||' 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 the condition, the variable mVRSystem is present two times. Obviously, one of them should be replaced with mVRChaperone .

dom

dom contains C interfaces and code for implementing and tracking DOM (Document Object Model) objects in Javascript. It forms the C substructure which creates, destroys and manipulates built-in and user-defined objects according to the Javascript script.



V570 The 'clonedDoc-> mPreloadReferrerInfo' variable is assigned to itself. Document.cpp 12049

 already_AddRefed<Document> Document::CreateStaticClone( nsIDocShell* aCloneContainer) { .... clonedDoc->mReferrerInfo = static_cast<dom::ReferrerInfo*>(mReferrerInfo.get())->Clone(); clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer detected the assignment of a variable to itself.

xpcom

xpcom contains the low-level C interfaces, C code, C code, a bit of assembly code and command line tools for implementing the basic machinery of XPCOM components (which stands for "Cross Platform Component Object Model"). XPCOM is the mechanism that allows Mozilla to export interfaces and have them automatically available to JavaScript scripts, to Microsoft COM and to regular Mozilla C code.



V611 The memory was allocated using 'malloc / realloc' function but was released using the 'delete' operator. Consider inspecting operation logics behind the 'key' variable. Check lines: 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; };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

After calling the strdup function, you need to free memory using the free function, not the delete operator.



V716 Suspicious type conversion in initialization: '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; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The WinGI SHGetSpecialFolderPathW function returns a value of type BOOL , not HRESULT . Checking the result of the function must be rewritten to the correct one.

nsprpub

nsprpub contains C code for the cross platform "C" Runtime Library. The “C” Runtime Library contains basic non-visual C functions to allocate and deallocate memory, get the time and date, read and write files, handle threads and handling and compare strings across all platforms



V647 The value of 'int' type is assigned to the pointer of 'short' type. Consider inspecting the assignment: '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; } /* SocketPoll */
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer detected the assignment of a numerical constant to the out_flags pointer. Most likely, they simply forgot to dereference him:

 if (fd->secret->alreadyConnected) { *out_flags = PR_POLL_WRITE; return PR_POLL_WRITE; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Conclusion

This is not the end. Be new code reviews. The Thunderbird and Firefox code includes two major libraries: Network Security Services (NSS) and WebRTC (Web Real Time Communications). There were some very interesting errors. In this review, I will show one at a time.



Nss



V597 The compiler could delete the 'memset' function call, which is used to flush 'newdeskey' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. 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 is a library for developing secure client and server applications, and here DES Key is not cleaned. The compiler will remove the memset call from the code, as the newdeskey array is no longer used in code beyond this location.



WebRTC



V519 The 'state [state_length - x_length + i]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 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 the second cycle, the data is written to the wrong array, because the author copied the code and forgot to change the name of the array from state to state_low .



There are probably more interesting bugs in these projects that are worth talking about. And we will do it in the near future. In the meantime, try PVS-Studio on your project.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Dark theme of Thunderbird as a reason to run a code analyzer .