Static analysis of Mozilla Thunderbird code using PVS-Studio

In this article I want to talk about the verification of the Mozilla Thunderbird project with the PVS-Studio static analyzer. Using Thunderbird, I sometimes encountered hangs and strange behavior of the program. Perhaps we can find at least some of the reasons for this in the source code. I invite you to see what mistakes can hide in such a popular project.

Email client Mozilla Thunderbird

Mozilla Thunderbird is a free, cross-platform freeware program for working with e-mail and newsgroups developed by the Mozilla Foundation. The main advantages of using Thunderbird are the simplicity and flexibility of the program. The user can customize the interface by changing, adding or removing buttons. In addition to this, the installation of extensions and new themes is supported. The program can use digital signatures, message encryption and certificate verification.

About the PVS-Studio analyzer

PVS-Studio is a static code analyzer for C and C ++ programs. PVS-Studio is provided as a plug-in to the Visual Studio development system, but can also be used via the Standalone utility. This utility has a monitoring function that tracks the compiler calls and transfers the necessary files to the analyzer. Thus, PVS-Studio does not depend on the project build system.

The tool is easy to use, so instead of many words, it is better to download and try the demo version on your own code.
')

Build and validate Thunderbird

Mozilla has its own build system. Instructions describing the basic steps for building a project can be found here . The assembly itself is made as convenient as possible for the user. Mozilla provides a binary installer of all utilities needed for installation under windows, for example, 7zip, msys, mercurial, etc.

The test was carried out using the compiler's call monitoring system mentioned above, which is provided by the Standalone utility included in the PVS-Studio package.

Analyzer Warnings

Thunderbird is a large project and uses many third-party libraries. Most of the warnings fell on their code. For the article, I tried to weed out these warnings and leave only those that were issued to the source code of the mail program.

In addition, to describe the bugs in the projects of Mozilla there is a page with a description of keywords . Among them you can find such as coverity, klocwork, valgrind and clang-anazyler. It appears that these code analysis tools are already used in Mozilla. So it will be interesting to look at what these analyzers did not notice.

Suspicious conditions

PVS-Studio warning : V501 There are identical sub-expressions "aStatus == NS_ERROR_OFFLINE" operator. nsdocshell.cpp 7606

nsresult nsDocShell::EndPageLoad(nsresult aStatus, ....) { if(....) { .... } else if (aStatus == NS_ERROR_NET_TIMEOUT || .... aStatus == NS_ERROR_OFFLINE || aStatus == NS_ERROR_MALWARE_URI || aStatus == NS_ERROR_PHISHING_URI || aStatus == NS_ERROR_UNWANTED_URI || aStatus == NS_ERROR_UNSAFE_CONTENT_TYPE || aStatus == NS_ERROR_REMOTE_XUL || aStatus == NS_ERROR_OFFLINE || ....) } 

This code contains an extra check of "NS_ERROR_OFFLINE". The list of values ​​for which you need to check the variable aStatus is large, so you can easily make mistakes and randomly duplicate the check. The second option may be that the programmer, after copying, inserted the same line in order not to rewrite the same part, and forgot to change the name of the constant “NS_ERROR_OFFLINE”. In this case, the code lacks one necessary check.

PVS-Studio warning : V590 Consider inspecting the 'type! = (1) && type == (2)' expression. The expression is misprint. nswindowsregkey.cpp 313

 #define REG_SZ ( 1 ) #define REG_EXPAND_SZ ( 2 ) #define REG_MULTI_SZ ( 7 ) NS_IMETHODIMP nsWindowsRegKey::ReadStringValue(const nsAString& aName, nsAString& aResult) { .... if (type != REG_SZ && type == REG_EXPAND_SZ && type == REG_MULTI_SZ) { return NS_ERROR_FAILURE; } .... } 

The condition “type == REG_EXPAND_SZ && type == REG_MULTI_SZ” is always false, since one variable cannot have two values ​​at the same time. As a result, the function will never return the error status NS_ERROR_FAILURE.

PVS-Studio warning : V616 The 'eBorderStyle_none' is in the bitwise operation. nswindow.cpp 2318

 enum nsBorderStyle { eBorderStyle_none = 0, .... } NS_IMETHODIMP nsWindow::SetNonClientMargins(....) { if (!mIsTopWidgetWindow || mBorderStyle & eBorderStyle_none) return NS_ERROR_INVALID_ARG; .... } 

When checking the condition, a constant is used with a value of 0, which participates in the bitwise AND operation with a variable. The result of the operation, of course, will also be zero. Thus, the condition does not depend on the “mBorderStyle” variable.

A similar warning:

• V616 The 'nsIDocShell :: BUSY_FLAGS_NONE' is used in the bitwise operation. presentationcallbacks.cpp 105

PVS-Studio warning : V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. nsnativethemewin.cpp 924 :

 nsresult nsNativeThemeWin::GetThemePartAndState(nsIFrame* aFrame, uint8_t aWidgetType, int32_t& aPart, int32_t& aState) { .... { .... if (!aFrame) { aState = TS_NORMAL; } else { if (GetCheckedOrSelected(aFrame, !isCheckbox)) { inputState = CHECKED; } if (isCheckbox && GetIndeterminate(aFrame)) { inputState = INDETERMINATE; } .... } .... } 

It is possible that before the last “if” the word else is omitted. The code for the current view implies that both if conditions can be met, and then the value of “CHECKED” in the variable “inputState” will be changed to “INDETERMINATE”. If either one condition or another were satisfied in this code, it would be more logical to use an if - else, as in the external structure.

Another similar design is located here:

• V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. debugger.cpp 4794

PVS-Studio warning : V713 The pointer mHTMLEditor was used for the same logical expression. nshtmleditrules.cpp 6593 :

 nsHTMLEditor* mHTMLEditor; nsresult nsHTMLEditRules::SplitParagraph(...) { if (mHTMLEditor->IsTextNode(child) || !mHTMLEditor || mHTMLEditor->IsContainer(child)) .... } 

The SplitParagraph function in its check contains an erroneous order of arguments. If the mHTMLEditor pointer is zero in this code, then it will be de-referenced before detecting this, which will lead to undefined behavior. To fix the code, you need to swap "! MHTMLEditor" and "mHTMLEditor-> IsTextNode (child)".

Two similar errors are located here:

• V713 The mHTMLEditor was used in the same logical expression. nshtmleditrules.cpp 7392
• V713 The mHTMLEditor was used in the same logical expression. nshtmleditrules.cpp 7413

PVS-Studio warning : V522 Dereferencing of the null pointer 'aStyleValues' might take place. sdnaccessible.cpp 252 :

 STDMETHODIMP sdnAccessible::get_computedStyle( BSTR __RPC_FAR* aStyleProperties, BSTR __RPC_FAR* aStyleValues, unsigned short __RPC_FAR* aNumStyleProperties) { if (!aStyleProperties || aStyleValues || !aNumStyleProperties) return E_INVALIDARG; .... aStyleValues[realIndex] = ::SysAllocString(value.get()); .... } 

As they say, mind you prankster.



The analyzer detected null pointer dereferencing. When checking the programmer forgot to put "!" before "aStyleValues". Further code gets control only when this pointer is equal to zero, and leads to its dereference.

PVS-Studio warning : V547 Expression is always false. Probably the '||' operator should be used here. nsmsgdbview.cpp 3014 :

 class NS_NO_VTABLE nsMsgViewCommandType { enum { .... junk = 27, unjunk = 28, .... }; }; nsresult nsMsgDBView:: ApplyCommandToIndices(nsMsgViewCommandTypeValue command, ....) { .... if ((command == nsMsgViewCommandType::junk) && (command == nsMsgViewCommandType::unjunk)) .... } 

The code corresponding to the if block will never be executed, since the command variable cannot have two values ​​at the same time. It seems more logical to use the operator "OR" - "||".

Pointer issues

PVS-Studio warning : V579 . It is possibly a mistake. Inspect the second argument. nsdisplaylist.h 929 :

 struct AnimatedGeometryRootLookup { .... PLDHashNumber Hash() const { return mozilla::HashBytes(this, sizeof(this)); } .... } 

The analyzer found it suspicious that a pointer is passed to the HashBytes function as the first argument, and a pointer size as the second. If you search the source files for the function name, you can find the following comment in the hashfunctions.h file:

 /* Utilities for hashing. */ /* * This file exports functions for hashing data down * to a 32-bit value, including: .... * - HashBytes Hash a byte array of known length. .... */ 

The comment suggests that the second argument should be the size of the object located at the pointer. Most likely the correct code should look like this:

 return mozilla::HashBytes(this, sizeof(*this)); 

Let's move on to the next warning.

PVS-Studio warning : V611 Consider inspecting operation logics behind the 'instanceData' variable. nptest.cpp 971 :

 NPError NPP_New(....) { .... InstanceData* instanceData = new InstanceData; .... free(instanceData); .... } 

The error lies in the fact that the memory is allocated using the operator "new", and is released using the "free". The “free” function does not call the destructor of the object located on this pointer. This means that if the object contained more pointers with allocated memory, it will not be freed and a leak will occur.

And in general, this can not be done. Such code leads to undefined program behavior.

PVS-Studio Warning: V614 Potentially uninitialized pointer 'hOldFont' used. progressui_win.cpp 168 :

 static void InitDialog(....) { .... HFONT hInfoFont, hOldFont; hInfoFont = (HFONT)SendMessage(hWndInfo, WM_GETFONT, 0, 0); if (hInfoFont) hOldFont = (HFONT)SelectObject(hDCInfo, hInfoFont); .... if (hOldFont) SelectObject(hDCInfo, hOldFont); .... } 

If the “SendMessage” function returns zero, the result of the next check will be false, which means the hOldFont variable will not be initialized. The variable will have a random value that may not be zero. If it is not 0, then this random value will be passed to the SelectObject function.

Another similar situation may arise here:

• V614 Potentially uninitialized pointer 'queryD3DKMTStatistics' used. gfxwindowsplatform.cpp 206

Copy-paste errors

PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1060, 1062. nsstylestruct.cpp 1060 :

 nsStyleClipPath::nsStyleClipPath(const nsStyleClipPath& aSource) { if (aSource.mType == NS_STYLE_CLIP_PATH_URL) { SetURL(aSource.mURL); } else if (aSource.mType == NS_STYLE_CLIP_PATH_SHAPE) { SetBasicShape(aSource.mBasicShape, aSource.mSizingBox); } else if (aSource.mType == NS_STYLE_CLIP_PATH_SHAPE) { SetSizingBox(aSource.mSizingBox); } } 

The “if - else if” block contains a duplicate equality test, called by the copy-paste method. This means that the last part of the code corresponding to the second check on “NS_STYLE_CLIP_PATH_SHAPE” will never be executed.



PVS-Studio warning : V523 The 'then' statement is equivalent to the 'else' statement.
mozspelli18nmanager.cpp 34 :

 NS_IMETHODIMP mozSpellI18NManager::GetUtil(mozISpellI18NUtil **_retval, ....) { .... nsAutoString lang; .... if(lang.EqualsLiteral("en")) { *_retval = new mozEnglishWordUtils; } else { *_retval = new mozEnglishWordUtils; } NS_IF_ADDREF(*_retval); return NS_OK; } 

The analyzer paid attention to the fact that the same actions correspond to the if and else blocks. This may be an error when copying, a superfluous condition or simply unfinished code. Anyway, in this form, the condition does not make sense.

Some more similar errors:

• V523 The 'then' statement is equivalent to the 'else' statement. jemalloc.c 6504
• V523 The 'then' statement is equivalent to the 'else' statement. nsnativethemewin.cpp 1007
• V523 The 'then' statement is equivalent to the 'else' statement. msgmapihook.cpp 677

Indefinite behavior

PVS-Studio warning : V595 The 'aParent' pointer was used before it was verified against nullptr. Check lines: 511, 518. nsgenericdomdatanode.cpp 511 :

 #define NS_ADDREF(_ptr) \ (_ptr)->AddRef() nsresult nsGenericDOMDataNode::BindToTree(nsIContent* aParent, ....) { .... ShadowRoot* parentContainingShadow = aParent->GetContainingShadow(); .... if (aParent) { if (!GetParent()) { NS_ADDREF(aParent); } mParent = aParent; } .... } 

Checking the “aParent” pointer indicates that it may be null. This means that the first time it is dereferenced, which occurs before the inspection, we risk getting undefined behavior.

The V595 warning is one of the most common among checked projects, and Thunderbird is no exception. In total, the analyzer issued 95 warnings related directly to the Thunderbird code.

PVS-Studio warning : V610 Undefined behavior. Check the shift operator '<<'. The left operand '~ 0L' is negative. nsprotocolproxyservice.cpp 336 :

 static void proxy_MaskIPv6Addr(PRIPv6Addr &addr, uint16_t mask_len) { .... addr.pr_s6_addr32[3] = PR_htonl( PR_ntohl(addr.pr_s6_addr32[3]) & (~0L << (128 - mask_len))); .... } 

When one of the left-shift parameters is a negative number, the behavior is undefined. Here is what is said about this in the standard:

The shift operators << and >> group left-to-right. shift-expression << additive-expression, shift-expression >> additive-expression.

This is an integral part of this article. 1. The promoted left operand. If you are the right operand. 2. ... If you want to use it, it’s possible to make it easier to use it. Otherwise, if E1 is a signed type and non-negative value, and E1 * 2 ^ E2 is representable, then that is the resulting value; otherwise, the behavior is undefined . ...

3 more cases of indefinite behavior:

• V610 Undefined behavior. Check the shift operator '<<'. The left operand '~ 0L' is negative. nsprotocolproxyservice.cpp 341
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '~ 0L' is negative. nsprotocolproxyservice.cpp 347
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '~ 0L' is negative. nsprotocolproxyservice.cpp 354

Warnings with features

PVS-Studio warning : V597 The compiler could delete the memset function call, which is used to flush the ctx object. The RtlSecureZeroMemory () function should be used to erase the private data. gmploader.cpp 166 :

 bool GMPLoaderImpl::Load(....) { SHA256Context ctx; .... // Overwrite all data involved in calculation as it could //potentially identify the user, so there's no chance a GMP //can read it and use it for identity tracking. memset(&ctx, 0, sizeof(ctx)); .... } 

Here the analyzer noticed that the call to the 'memset' function can be deleted. Since the variable 'ctx' is not used further, the compiler has every right to remove the “memset” call when optimizing. In Windows, you can use the RtlSecureZeroMemory feature.

PVS-Studio warning : V530 The return value of the 'getenv' is required to be utilized.
nswindowswmain.cpp 134

 int wmain(int argc, WCHAR **argv) { .... // Force creation of the multibyte _environ variable. getenv("PATH"); int result = main(argc, argvConverted, _environ); .... } 

Here we are dealing with a call to the function “getenv”, the result of which is not used and is not even written to a variable. Here’s how this feature is described on cplusplus.com.

This is where the environment variable is defined. The function returns a null pointer.

Using "getenv" in this form is meaningless and only confusing when reading the code.

UPD : habrahabr.ru/company/pvs-studio/blog/267663/#comment_8589575

Other warnings

PVS-Studio warning : V609 Divide by zero. Denominator range [0..8]. ionbuilder.cpp 10922 :

 static inline size_t UnboxedTypeSize(JSValueType type) { switch (type) { .... default: return 0; } } MInstruction*IonBuilder::loadUnboxedProperty(size_t offset, JSValueType unboxedType, ....) { size_t index = offset / UnboxedTypeSize(unboxedType); .... } 

Since the “UnboxedTypeSize” function can return zero, potentially we are dealing with division by zero. If a new type is transferred to the “UnboxedTypeSize” function, it will return the default zero value, which will lead to an exception. Better to be safe and add a check before dividing.

Another potential division by zero:

• V609 Divide by zero. Denominator range [0..8]. ionbuilder.cpp 11844

PVS-Studio warning : V621 Consider inspecting the 'for' operator. It is possible that you will not be executed at all. nsmsgdbfolder.cpp 4501 :

 NS_IMETHODIMP nsMsgDBFolder::GetDisplayRecipients(bool *displayRecipients) { .... // There's one FCC folder for sent mail, and one for sent news nsIMsgFolder *fccFolders[2]; int numFccFolders = 0; for (int i = 0; i < numFccFolders; i++) { .... } .... } 

The analyzer found a suspicious place in which the loop does not go through a single iteration. The reason for this is the variable "numFccFolders", the value of which is zero. Perhaps this assignment is done for some purpose, but it is also possible that this is a typo. The comment and pointer declaration above suggest that the variable should be 2.

PVS-Studio warning : V678 An object is used as an argument to its own method. Consider checking this function. nsgenerichtmlelement.h 411 :

 class nsGenericHTMLElement : public nsGenericHTMLElementBase, public nsIDOMHTMLElement { .... NS_IMETHOD GetItemId(nsAString& aId) final override { nsString id; GetItemId(id); aId.Assign(aId); return NS_OK; } .... } 

In itself, using the "aId" object as an argument in its own method is not an error. But this code is suspicious because the function uses a variable with a similar name “id”. This suggests that there is a typo and the argument of the function "aId.Assign" should be the variable "id".

PVS-Studio warning : V670 The uninitialized class member 'mWorkerConnection' is used to initialize the 'mWorkerStatements' member. Remember that members are initialized in the order of declarations inside a class. domstoragedbthread.cpp 50

 DOMStorageDBThread::DOMStorageDBThread() : mWorkerStatements(mWorkerConnection) , .... {} class DOMStorageDBThread final : public DOMStorageDBBridge { private: .... StatementCache mWorkerStatements; //<=line 304 .... nsCOMPtr<mozIStorageConnection> mWorkerConnection; //<=line 309 .... } 

When using the initialization list, one thing should be remembered: the variables are initialized in the order in which they are declared in the class, and the order in the initialization list does not matter. In this code, the "mWorkerStatements" variable is initialized by the "mWorkerConnection" object of another class. But at the time of initialization, a constructor was not yet called for this object, since it was declared later in the class than the variable “mWorkerStatements”. To fix this, it is enough to swap the declaration of these two objects in the class.

In this class there is another similar bug:

• V670 The uninitialized class member 'mReaderConnection' is used to initialize the 'mReaderStatements' member. Remember that members are initialized in the order of declarations inside a class. domstoragedbthread.cpp 51

Conclusion

Summing up, I would like to note that PVS-Studio found many suspicious places in the Mozilla Thunderbird project. Most of them belong to third-party libraries. Nevertheless, in the Thunderbird found some interesting errors.

To write a big project without errors is beyond the power of even the most experienced and attentive programmers. For such purposes, there are static code analyzers. Using them will help you save time searching for old mistakes and prevent new ones. I suggest trying PVS-Studio on your project: http://www.viva64.com/ru/pvs-studio-download/ .



If you want to share this article with an English-speaking audience, then please use the link to the translation: Igor Shtukarev. Static Analysis of Mozilla Thunderbird's Code by PVS-Studio .