Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
In this article, we will talk about the analysis of the Mozilla Thunderbird project by the PVS-Studio static analyzer. Being a Thunderbird user, I would occasionally run into hangs and strange behavior of the program. Hopefully our analysis will help to reveal at least some of the reasons behind it in the source code. So welcome to follow me to see what errors can be found in this popular project.
Mozilla Thunderbird is a free, open-source, cross-platform email, news, and chat client developed by the Mozilla Foundation. Its simplicity and flexibility are believed to be Thunderbird's major advantages. Users can customize the interface on their own by changing, adding, or deleting buttons. Also, the program supports the installation of new add-ons and themes, and allows using digital signatures, message encryption, and certificate validation.
PVS-Studio is a static code analyzer for C and C++ programs. It comes as a plugin for the Visual Studio IDE but can also be used as a Standalone version. This utility employs the monitoring feature, which tracks compiler calls and passes all the necessary files to the analyzer. It allows PVS-Studio to work independently from the project's build system.
The tool is easy to use, so instead of talking about it I'd rather recommend that you download and try the demo version on your own code.
Mozilla has its own build system. The documentation on the basic steps for building the project can be found here. The building process itself is ensured to be as comfortable for the user as possible. Mozilla provides a binary installer for all the utilities necessary for running the program under Windows, for example 7zip, msys, mercurial, and so on.
The analysis was done with the help of the compiler call monitoring system of the Standalone utility coming with the PVS-Studio pack, as mentioned above.
Thunderbird is a large project, using lots of third-party libraries. It is the code of these libraries that most of the generated warnings refer to. For this article, I tried to sieve out these warnings and focus on those triggered by the source code of the client itself.
Besides, Mozilla has a page with a list of keywords to describe bugs found in their projects. Among those words, you can see such words as coverity, klocwork, valgrind, and clang-analyzer. Looks like Mozilla already use these code analyzers, so it would be interesting to look at the bugs these tools missed.
PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'aStatus == NS_ERROR_OFFLINE' to the left and to the right of the '||' 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 excessive check "NS_ERROR_OFFLINE". The list of values the 'aStatus' variable must be checked for is pretty lengthy, so it's no wonder the programmer made a mistake and duplicated the check. Another explanation is that the programmer was pasting one and the same copied line to avoid having to rewrite the repeating part and forgot to change the name of the "NS_ERROR_OFFLINE" constant. If this is the case, then there is a missing check in this code.
PVS-Studio's diagnostic message: V590 Consider inspecting the 'type != (1) && type == (2)' expression. The expression is excessive or contains a 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 "type == REG_EXPAND_SZ && type == REG_MULTI_SZ" condition is always false as one variable can't have two values at a time. As a result, the function will never return the status of the NS_ERROR_FAILURE error.
PVS-Studio's diagnostic message: V616 The 'eBorderStyle_none' named constant with the value of 0 is used 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;
....
}The condition is checked with the help of a constant with the value 0, acting as an operand in the bitwise "AND" operation with a variable as the second operand. The result of this operation is, naturally, also zero. That is, the condition doesn't depend on the "mBorderStyle" variable.
Another similar warning:
PVS-Studio's diagnostic message: 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;
}
....
} ....
}The keyword else is probably missing before the last "if". The code in its current form implies that both if conditions can be true, in which case the "CHECKED" value of the "inputState" variable will be changed to "INDETERMINATE". If only one of the two conditions was meant to be true, it would be more logical to use "if - else", like in the external construct.
Another similar construct can be found in the following fragment:
PVS-Studio's diagnostic message: V713 The pointer mHTMLEditor was utilized in the logical expression before it was verified against nullptr in the same logical expression. nshtmleditrules.cpp 6593
nsHTMLEditor* mHTMLEditor;
nsresult
nsHTMLEditRules::SplitParagraph(...)
{
if (mHTMLEditor->IsTextNode(child) ||
!mHTMLEditor ||
mHTMLEditor->IsContainer(child))
....
}Incorrect order of arguments in the check inside the "SplitParagraph" function. If the mHTMLEditor pointer turns out to be null, it will have been already dereferenced before the fact is discovered, which will cause undefined behavior. To fix the code, we need to swap "!mHTMLEditor" and "mHTMLEditor->IsTextNode(child)".
Two more errors of this type can be found in the following fragments:
PVS-Studio's diagnostic message: 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());
....
}Find the rogue.
The analyzer has detected a null pointer dereferencing issue. When implementing the check, the programmer forgot to add "!" before "aStyleValues". The subsequent code gets control only when this pointer equals zero, and dereferences it.
PVS-Studio's diagnostic message: 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 in the if block will never execute because the command variable can't have two values at a time. It would be more logical to use the "OR" - "||" operation here.
PVS-Studio's diagnostic message: V579 The HashBytes function receives the pointer and its size as arguments. 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 strange that a pointer is passed into the "HashBytes" function as its first argument, while the pointer size as the second one. If you look for the function name in the source files, you'll 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 tells us that the second argument should be represented by the size of the object pointed to by the pointer. The correct code, therefore, should look like this, I guess:
return mozilla::HashBytes(this, sizeof(*this));
Going on to the next warning.
PVS-Studio's diagnostic message: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'instanceData' variable. nptest.cpp 971
NPError NPP_New(....)
{
....
InstanceData* instanceData = new InstanceData;
....
free(instanceData);
....
}The error here is about memory being allocated through the "new" operator and freed through the "free" function. This function doesn't call the destructor of the object pointed to by the pointer. It means that if the object contained other pointers with allocated memory, it won't be freed and a leak will occur.
Well, it's no good doing things like that, anyway: they result in undefined behavior.
PVS-Studio's diagnostic message: 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 next check will evaluate to false, which means the hOldFont variable won't be initialized. The variable will take a random value, which won't be necessarily zero. And if it's not 0, this random value will be passed into the SelectObject function.
Here's another similar issue:
PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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 duplicated equality check, this error being caused by careless usage of the copy-paste method. It means that the last part of the code, corresponding to the second check for "NS_STYLE_CLIP_PATH_SHAPE", will never be executed.
PVS-Studio's diagnostic message: 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 noticed that the if and else branches are identical. This may be a copy-paste error, an excessive condition, or simply incomplete code. Whatever it is, the condition is meaningless.
A few more errors of this kind:
PVS-Studio's diagnostic message: V595 The 'aParent' pointer was utilized 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;
}
....
}The check of the "aParent" pointer suggests that it can be null. It means that the first time it's dereferenced, which happens before the check, we risk getting undefined behavior.
The V595 warning is one of the most frequent across all the projects we scan, and Thunderbird is no exception. In total, the analyzer output 95 warnings of this type for the code of Thunderbird itself.
PVS-Studio's diagnostic message: 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 operands of the left-shift operation is a negative value, the behavior is undefined. This is what the standard has to say about it:
The shift operators << and >> group left-to-right. shift-expression << additive-expression, shift-expression >> additive-expression
The operands shall be of integral or unscoped enumeration type and integral promotions are performed. 1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand. 2. ... If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. ...
3 more cases of undefined behavior:
PVS-Studio's diagnostic message: V597 The compiler could delete the 'memset' function call, which is used to flush '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));
....
}In this code, the analyzer noticed that the call of the 'memset' function might be removed. Since the 'ctx' variable is not used afterwards, the compiler has a full right to remove the call of "memset" for the optimization's sake. Under Windows, you can use the "RtlSecureZeroMemory" function to avoid this.
PVS-Studio's diagnostic message: V530 The return value of function '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);
....
}In this sample, we are dealing with a call of the "getenv" function whose result is not used and nor even written into a variable. This is how this function is described on the cplusplus.com site.
Retrieves a C-string containing the value of the environment variable whose name is specified as argument. If the requested variable is not part of the environment list, the function returns a null pointer.
Using "getenv" in its current form is pointless and will only confuse whoever may happen to read the code.
PVS-Studio's diagnostic message: 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 may return zero, we have a potential division by zero here. If a new type is passed into the "UnboxedTypeSize" function, it will return the default zero value, which will result in throwing an exception. We'd better play it safe and add a check before the division.
Another potential division by zero:
PVS-Studio's diagnostic message: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't 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 fragment where a loop doesn't run through even a single iteration. The reason is the "numFccFolders" variable, storing a zero. Perhaps this assignment was written purposefully, but it might as well be a typo. The comment and the pointer declaration a bit earlier suggest that the variable must have the value 2.
PVS-Studio's diagnostic message: V678 An object is used as an argument to its own method. Consider checking the first actual argument of the 'Assign' 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;
}
....
}Using the "aId" object as an argument in its own method is not an error in itself, but this code does look suspicious because of the variable with a similar name, "id", used in the function. It prompts an idea that we are dealing with a typo and it is the "id" variable that should have been the argument of the "aId.Assign" function.
PVS-Studio's diagnostic message: V670 The uninitialized class member 'mWorkerConnection' is used to initialize the 'mWorkerStatements' member. Remember that members are initialized in the order of their 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 working with initialization lists, keep in mind one tricky detail: variables are initialized in the same order they were declared in the class, while the order in the initialization list doesn't matter. In the code sample above, the "mWorkerStatements" variable is initialized to the "mWorkerConnection" object of another class. But the destructor for this object hasn't been called yet by the moment of variable initialization, for it is declared in the class later than the "mWorkerStatements" variable. To fix that, we just need to swap the declarations of these two objects in the class.
This class has one more error of the same kind:
Summing it up, I'd like to notice that PVS-Studio has found plenty of suspicious fragments in the Mozilla Thunderbird project. Most of them refer to third-party libraries; however, the client itself has a number of interesting bugs too.
Writing a large-scale project without a single mistake is beyond the power of even the most experienced and careful programmers. This is why static code analyzers exist: they can help you save time on searching for old bugs, and avoid new ones. Welcome to try PVS-Studio on your project: http://www.viva64.com/en/pvs-studio/download/.
0
0
0
0