Our website uses cookies to enhance your browsing experience.
We already checked Mozilla Firefox with the PVS-Studio analyzer three years ago. It was pretty inconvenient and troublesome at the time. You see, there is no Visual Studio project file for Firefox – the build is done with the help of makefiles. That's why you can't just take and check the project. We had to integrate PVS-Studio into the build system, which appeared a difficult task. If I remember it rightly, it all resulted in successfully analyzing only a part of the project. But everything is different now that we have PVS-Studio Standalone. We can now monitoring all compiler launches and easily check the project.
I don't think Firefox needs introduction, but the format of the article implies giving some description of the project under analysis. Well, I'm too lazy, so here you are an excerpt from the Wikipedia article:
Mozilla Firefox is a free and open-source web browser developed for Windows, OS X, and Linux, with a mobile version for Android, by the Mozilla Foundation and its subsidiary, the Mozilla Corporation. Firefox uses the Gecko layout engine to render web pages, which implements current and anticipated web standards.
As of February 2014, Firefox has between 12% and 22% of worldwide usage, making it the third most popular web browser.
Features include tabbed browsing, spell checking, incremental find, live bookmarking, Smart Bookmarks, a download manager, private browsing, location-aware browsing ("geolocation") based on a Google service and an integrated search system that uses Google by default in most localizations. Functions can be added through extensions, created by third-party developers.
We already tried to analyze Firefox earlier and even succeeded to some extent. Based on the analysis results, we wrote the article "How to make fewer errors at the stage of code writing. Part N4". What was difficult about checking the project at the time is that we had to integrate the call of PVS-Studio's command-line version into makefiles. Doing this in a large, unfamiliar project is usually troublesome. That is the reason why we never tried to reanalyze Firefox after the first check. It all changed when PVS-Studio Standalone was created.
PVS-Studio Standalone can be used in 2 modes:
You don't need to integrate PVS-Studio's command-line version into makefiles anymore. Firefox can now be checked in a much simpler way – and we have used it. The algorithm includes the following steps:
For more details about how to use this mode, please follow this link.
The Firefox project is a very high-quality one. What's more, I find some evidence that developers make use of static code analysis tools in the development process – Coverity and Klocwork; at least, these tools are mentioned in some files.
Taking all that into account, it would be a large success indeed to find anything worthy in this project. So let's find out if there are any interesting diagnostic messages by PVS-Studio for the Firefox project.
Typo No. 1
NS_IMETHODIMP
nsNativeThemeWin::WidgetStateChanged(....)
{
....
if (aWidgetType == NS_THEME_WINDOW_TITLEBAR ||
aWidgetType == NS_THEME_WINDOW_TITLEBAR_MAXIMIZED ||
aWidgetType == NS_THEME_WINDOW_FRAME_LEFT ||
aWidgetType == NS_THEME_WINDOW_FRAME_RIGHT ||
aWidgetType == NS_THEME_WINDOW_FRAME_BOTTOM ||
aWidgetType == NS_THEME_WINDOW_BUTTON_CLOSE ||
aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE || <<<===
aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE || <<<===
aWidgetType == NS_THEME_WINDOW_BUTTON_RESTORE) {
*aShouldRepaint = true;
return NS_OK;
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'aWidgetType == 237' to the left and to the right of the '||' operator. nsnativethemewin.cpp 2475
The 'aWidgetType' variable is compared to the NS_THEME_WINDOW_BUTTON_MINIMIZE constant twice. This is a typo: the variable should be compared to the constant NS_THEME_WINDOW_BUTTON_MAXIMIZE for the second time.
Typo No. 2
bool nsHTMLCSSUtils::IsCSSEditableProperty(....)
{
....
if (aAttribute && aAttribute->EqualsLiteral("align") &&
(nsEditProperty::ul == tagName <<<<====
|| nsEditProperty::ol == tagName
|| nsEditProperty::dl == tagName
|| nsEditProperty::li == tagName
|| nsEditProperty::dd == tagName
|| nsEditProperty::dt == tagName
|| nsEditProperty::address == tagName
|| nsEditProperty::pre == tagName
|| nsEditProperty::ul == tagName)) { <<<<====
return true;
}
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'nsEditProperty::ul == tagName' to the left and to the right of the '||' operator. nshtmlcssutils.cpp 432
The 'tagName' variable is compared to nsEditProperty::ul twice. Perhaps one of the checks is redundant, or it should have been compared to something else instead.
Typo No. 3
void Reverb::process(....)
{
....
bool isCopySafe =
destinationChannelL &&
destinationChannelR &&
size_t(destinationBus->mDuration) >= framesToProcess &&
size_t(destinationBus->mDuration) >= framesToProcess;
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'size_t (destinationBus->mDuration) >= framesToProcess' to the left and to the right of the '&&' operator. reverb.cpp 192
The 'framesToProcess' variable is compared to 'size_t(destinationBus->mDuration)' twice.
Typo No. 4
float
PannerNode::ComputeDopplerShift()
{
....
double scaledSpeedOfSound = listener->DopplerFactor() /
listener->DopplerFactor();
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'listener->DopplerFactor()' to the left and to the right of the '/' operator. pannernode.cpp 529
That's a very suspicious expression and it should be examined.
Typo No. 5
bool DataChannelConnection::SendDeferredMessages()
{
....
if ((result = usrsctp_sendv(mSocket, data, ...., 0) < 0)) {
....
}PVS-Studio's diagnostic message: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. datachannel.cpp 1105
A parenthesis is written in a wrong place. Let's simplify the expression to make the mistake clearer:
if ((result = foo() < 0))This expression is calculated in the following way. The result returned by the function is compared to 0; then true or false is written into the 'result' variable. The mistake is about one of the closing parentheses written in a wrong place. The programmer actually meant the expression to look as follows:
if ((result = foo()) < 0)In this case, the result returned by the function is first written into the 'result' variable and only then is compared to 0.
Typo No. 6
void nsRegion::SimplifyOutwardByArea(uint32_t aThreshold)
{
....
topRects = destRect;
bottomRects = bottomRectsEnd;
destRect = topRects;
....
}PVS-Studio's diagnostic message: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 358, 360. nsregion.cpp 360
This code is suspicious; there must be some typo in it.
Incorrect check No. 1
enum nsBorderStyle {
eBorderStyle_none = 0,
....
};
....
NS_IMETHODIMP
nsWindow::SetNonClientMargins(nsIntMargin &margins)
{
if (!mIsTopWidgetWindow ||
mBorderStyle & eBorderStyle_none ||
mHideChrome)
return NS_ERROR_INVALID_ARG;
....
}PVS-Studio's diagnostic message: V616 The 'eBorderStyle_none' named constant with the value of 0 is used in the bitwise operation. nswindow.cpp 2278
The "mBorderStyle & eBorderStyle_none" expression makes no sense. Absence of styles (eBorderStyle_none) is coded by value 0. The condition code should most likely look as follows:
if (!mIsTopWidgetWindow ||
mBorderStyle != eBorderStyle_none ||
mHideChrome)Incorrect check No. 2
NS_IMETHODIMP nsWindowsRegKey::ReadStringValue(....)
{
....
DWORD type;
....
if (type != REG_SZ && type == REG_EXPAND_SZ &&
type == REG_MULTI_SZ)
return NS_ERROR_FAILURE;
....
}PVS-Studio's diagnostic message: V547 Expression is always false. Probably the '||' operator should be used here. nswindowsregkey.cpp 292
The 'type' variable cannot equal two different values at once. Let's simplify the code to see more clearly what the analyzer doesn't like in this code sample:
if (... && type == 2 && type == 7)This condition is always false.
The code should most likely look as follows:
if (type != REG_SZ && type != REG_EXPAND_SZ &&
type != REG_MULTI_SZ)Incorrect check No. 3
const SafepointIndex *
IonScript::getSafepointIndex(uint32_t disp) const
{
....
size_t minEntry = 0;
....
size_t guess = ....;
....
while (--guess >= minEntry) {
guessDisp = table[guess].displacement();
JS_ASSERT(guessDisp >= disp);
if (guessDisp == disp)
return &table[guess];
}
....
}PVS-Studio's diagnostic message: V547 Expression '-- guess >= minEntry' is always true. Unsigned type value is always >= 0. ion.cpp 1112
The loop will only terminate when the necessary item is found. If there is no such an item, the loop termination condition will never be true, and an array overrun will occur.
The reason is that the 'guess' variable is unsigned. It means that the (‑‑guess >= 0) condition is always true.
Inattention No. 1
void WinUtils::LogW(const wchar_t *fmt, ...)
{
....
char* utf8 = new char[len+1];
memset(utf8, 0, sizeof(utf8));
....
}PVS-Studio's diagnostic message: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. winutils.cpp 146
The 'sizeof(utf8)' expression returns the pointer size, not the size of the allocated memory buffer. The correct code should look like this:
memset(utf8, 0, sizeof(*utf8) * (len+1));Inattention No. 2
As usual, there are some code fragments to be found where pointers are first used and only then checked for being null. I will cite only one of these samples; Firefox' authors can use our analyzer themselves to find all the rest errors of this kind.
void
nsHttpTransaction::RestartVerifier::Set(
int64_t contentLength, nsHttpResponseHead *head)
{
if (mSetup)
return;
if (head->Status() != 200) <<<<====
return;
mContentLength = contentLength;
if (head) { <<<<====
....
}PVS-Studio's diagnostic message: V595 The 'head' pointer was utilized before it was verified against nullptr. Check lines: 1915, 1920. nshttptransaction.cpp 1915
The 'head' pointer is first dereferenced in the "head->Status()" expression and only then is checked for being null.
Inattention No. 3
NPError NPP_New(....)
{
....
InstanceData* instanceData = new InstanceData;
....
NPError err = pluginInstanceInit(instanceData);
if (err != NPERR_NO_ERROR) {
NPN_ReleaseObject(scriptableObject);
free(instanceData);
return err;
}
....
}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 1029
The 'new' operator is used to allocate memory while the 'free' function is called to free it. It results in undefined program behavior. It's not that crucial though because this code fragment is related to tests.
Inattention No. 4
Another code fragment found in tests. The 'device' variable might be left uninitialized:
static ID3D10Device1* getD3D10Device()
{
ID3D10Device1 *device;
....
if (createDXGIFactory1)
{
....
hr = createD3DDevice(...., &device);
....
}
return device;
}PVS-Studio's diagnostic message: V614 Potentially uninitialized pointer 'device' used. nptest_windows.cpp 164
The purpose of this article was not to describe every bug that PVS-Studio is able to detect. I'm sure I have missed something; and some bugs I didn't describe consciously. For example, the analyzer generated lots of V610 warnings related to shift operations that cause undefined behavior. But these warnings all look alike, so I don't find them interesting enough to be mentioned here.
The article is meant to show you the capabilities of static analysis and attract programmers' attention to our tool. Firefox' developers should carry out a more thorough analysis of their project as it will be much easier for them to figure out if certain issues are genuine bugs or not.
A note for Firefox' developers. The project is pretty large, so PVS-Studio generates quite a number of false positives. However, most of them are related to specific macros. You can easily reduce the number of false positives by several times by adding special comments into the code. See the documentation to figure out how to suppress warnings on certain macros (see the "Suppression of false alarms" section). If you are interested in purchasing a PVS-Studio license, we are also ready to take part in eliminating false positives in your project.
There were few suspicious code fragments in Firefox. The reason is that many bugs had been already caught through other testing methods and static analyzers. Static code analyzers are most useful when being used regularly as they allow you to detect mistakes as early as at the coding stage. To learn more on the subject, see the article "Leo Tolstoy and static code analysis".
I wish you good luck in programming and bugless code.
0
0
0
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:
0