Introduction
The dark theme of the new Thunderbird version looks pretty. I like dark themes. I've already switched to them in messengers, Windows, macOS. Soon iPhone will be updated to iOS 13 with a dark theme. For this reason I even had to change my iPhone 5S for a newer model. In practice, it turned out that a dark theme requires more effort for developers to pick up the colors of the interface. Not everyone can handle it the first time.This is how standard tags looked like in Thunderbird after updating:
I normally use 6 tags (5 standard +1 custom) to mark up emails. Half of them became impossible to look at after the update, so I decided to change the color in the settings to a brighter one. At this point I got stuck with a bug:
You can't change a tag color!!! More truly, you can, but the editor won't let you save it, referring to an already existing name (WTF???).
Another symptom of this bug is an inactive OK button. Since I couldn't make changes in the same name tag, I tried to change its name. Well, it turns out you can't rename it either.
Finally, you might have noticed that the dark theme didn't work for the settings, which is also not very nice.
After a long struggle with the build system in Windows I eventually built Thunderbird from the source files. The latest version of the mail client turned out to be much better than the fresh release. In it, the dark theme got to the settings as well, and this bug with tags editor disappeared. Nevertheless, to ensure that project's building isn't just a waste of time, the PVS-Studio static code analyzer got to its work.
Note. Thunderbird's source code intersects with the Firefox code base by some means. Therefore, the analysis includes errors from different components, which are worth a close look by developers of these teams.
Note 2. While I was writing the article, Thunderbird 68.1 was released and this bug was fixed:
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 string was compared with the HEADER_REPLY_TO constant twice. Maybe there should have been another constant in its place.
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 — headers. As always, there are two possible explanations: an unnecessary 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);
}Ctrl+C and Ctrl+V keys definitely helped to speed up the writing of this cascade of conditional expressions. As a result, one of the branches will never be 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 it is still in the process of writing. It may safely be said that the error will show itself after code is refined. A programmer can change the commented code, but will never get control of it. Please, be very careful and attentive 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;
}Possible dereference of the row null pointer in the following line:
morkCell* cell = row->CellAt(ev, pos);Most likely, a pointer wasn't initialized, for example, by 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. Its different bits represent different fields of an error description. You need to set the error code using special constants from system header files.
A couple of fragments like this:
- 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. In order to reset the structures' values, authors used the memset function, but passed the pointer size as the size of the memory space.
Similar suspicious fragments:
- 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;
}
....
}The V595 diagnostic usually detects typical errors of null pointer dereference. In this case we have an extremely interesting example, worthy of special attention.
Technically the analyzer is correct that the aValues pointer is first dereferenced and then checked, but the actual error is different. It's a double pointer, so correct code should look as follows:
*aValues = static_cast<nsILDAPBERValue **>(
moz_xmalloc(numVals * sizeof(nsILDAPBERValue)));
if (!*aValues) {
ldap_value_free_len(values);
return NS_ERROR_OUT_OF_MEMORY;
}Another similar fragment:
- 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 is found using a new diagnostic that will be available in the next analyzer release. All variables used in the while loop's condition don't change, as variables ptr2 and cSet are confused 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);
}
....
}First I thought that duplicating the connectStarted variable is just redundant code. But then I looked through the whole quite long function and found a similar fragment. Most likely, the connectCalled variable has to be here instead of the connectStarted variable.
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. An error was made in the class destructor due to missing 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 the pos variable gets rewritten in the loop for the same value. Looks like the new diagnostic has found another error.
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 a a 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;
}
}The mVRSystem variable appears in the condition twice. Obviously, one of its occurrences 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 found the assigning of the 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, one has to release the 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;
}
....
}SHGetSpecialFolderPathW WinAPI function returns the value of the BOOL type, not HRESULT. One has to rewrite the check of the function result 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 has detected assigning a numerical constant to the out_flags pointer. Most likely, one has just forgotten to dereference it:
if (fd->secret->alreadyConnected) {
*out_flags = PR_POLL_WRITE;
return PR_POLL_WRITE;
}Conclusion
It's not the end yet. Let new code reviews be! Thunderbird and Firefox code comprise two large libraries: Network Security Services (NSS) and WebRTC (Web Real Time Communications). I found there some compelling errors. In this review I'll show one from each project.
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. Whereas DES Key isn't being cleared here. The compiler will delete the memset call from the code, as the newdeskey array isn't used anywhere in code further.
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 loop, data is written in the wrong array, because the author copied the code and forgot to change the state array name for state_low.
Probably, there are still interesting bugs in these projects, which should be told about. And we'll do it soon. In the meantime, try PVS-Studio on your project.