Easily and easily check Firefox with PVS-Studio Standalone

Three years ago we tested Mozilla Firefox using the PVS-Studio analyzer. Then it was inconvenient and difficult. For Firefox, there is no project file for Visual Studio. Build by using makefiles. Therefore, just take and check the project can not be. It was necessary to integrate PVS-Studio into the build system, which turned out to be a difficult task. As a result, as I recall, only part of the project was analyzed. But everything changed when PVS-Studio Standalone appeared. Now you can track all runs of compilers and easily check the project.

Mozilla firefox

I think there’s no need to talk about Firefox. However, the format of the article requires you to write a little about the proven project. I will be lazy and use the description from Wikipedia:

Mozilla Firefox is a free browser based on the Gecko engine developed and distributed by the Mozilla Corporation. The third most popular browser in the world and the first among free software - in August 2013, its market share was 19.26%. In the browser there is an interface with many tabs, spell checking, search as you type, “live bookmarks”, download manager, field for accessing search engines. New features can be added using extensions.
')
We already tried to test Firefox. In part, we even succeeded. According to the results of the check, an article was written “ How to reduce the probability of an error at the stage of writing the code. Note N4 ”. The difficulty was that you need to insert the command-line version of PVS-Studio into the make-files. It can be difficult to do this in a big unfamiliar project. That’s why we didn’t make any further attempts to double-check Firefox. The situation has changed with the advent of PVS-Studio Standalone.

PVS-Studio Standalone

PVS-Studio Standalone can be used in 3 modes:

1. Convenient work with the report file (* .plog), which contains information about errors found on a computer where Visual Studio is not installed.
2. Check in any way preprocessed * .i files.
3. Tracking the compiler launch and collecting all the necessary information to further check the files. We are now interested in this particular mode of operation.

Now it is not necessary to integrate the command-line version of PVS-Studio into make-files. You can check Firefox much easier. We did just that. Sequencing:

1. Run the PVS-Studio Standalone program;
2. We execute the “Compiler Monitoring” command;
3. Compile the project Firefox;
4. Stop the tracking procedure (“Stop Monitoring”);
5. File scanning starts;
6. We study the warnings issued by the code analyzer.

More details on how to use this mode can be found here .

Mozilla Firefox check results

Firefox project is very high quality. In addition, as I understand it, in its development, static code analysis tools are already used: Coverity and Klocwork. At least, I have found references to these analyzers in some files.

Therefore, to find at least something is already an achievement. Let's see which warnings of the PVS-Studio analyzer seemed interesting to me.

Typo n1

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 warning: V501 There are identical 'aWidgetType == 237' there | operator. nsnativethemewin.cpp 2475

The variable 'aWidgetType' is compared twice with the constant NS_THEME_WINDOW_BUTTON_MINIMIZE. This is a missprint. The second time you need to compare the variable with the constant NS_THEME_WINDOW_BUTTON_MAXIMIZE.

Typo N2

 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 warning: V501 There are identical sub-expressions 'nsEditProperty :: ul == tagName' operator. nshtmlcssutils.cpp 432

The variable 'tagName' is compared twice with nsEditProperty :: ul. Perhaps one check is superfluous. Or forgot to compare with something else.

Typo n3

 void Reverb::process(....) { .... bool isCopySafe = destinationChannelL && destinationChannelR && size_t(destinationBus->mDuration) >= framesToProcess && size_t(destinationBus->mDuration) >= framesToProcess; .... } 

PVS-Studio warning: V501 There are identical sub-expressions 'size_t (destinationBus-> mDuration)> = framesToProcess' the operator. reverb.cpp 192

The variable 'framesToProcess' is compared twice with 'size_t (destinationBus-> mDuration)'.

Typo n4

 float PannerNode::ComputeDopplerShift() { .... double scaledSpeedOfSound = listener->DopplerFactor() / listener->DopplerFactor(); .... } 

PVS-Studio warning: V501 There are identical sub-expressions 'listener-> DopplerFactor ()' to the left and right. pannernode.cpp 529

A very suspicious expression worth checking out.

Typo n5

 bool DataChannelConnection::SendDeferredMessages() { .... if ((result = usrsctp_sendv(mSocket, data, ...., 0) < 0)) { .... } 

PVS-Studio warning: V593 Consider reviewing the expression A = B <C 'kind. The expression is calculated as the following: 'A = (B <C)'. datachannel.cpp 1105

Not there is a bracket. Simplify the expression to make the error more noticeable:

 if ((result = foo() < 0)) 

This expression is calculated as follows. The result returned by the function is compared with 0. Then true or false is written to the 'result' variable. The error is that one of the brackets is not closing there. In fact, the programmer wanted to write an expression:

 if ((result = foo()) < 0) 

Now the value returned by the function is written to the 'result' variable. And only then this value is compared with zero.

Typo N6

 void nsRegion::SimplifyOutwardByArea(uint32_t aThreshold) { .... topRects = destRect; bottomRects = bottomRectsEnd; destRect = topRects; .... } 

PVS-Studio warning: V587 An odd sequence of assignments of this kind: A = B; B = A ;. Check lines: 358, 360. nsregion.cpp 360

The code is suspicious. There must be some typo here.

Incorrect check N1

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

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

The expression "mBorderStyle & eBorderStyle_none" does not make sense. The lack of styles (eBorderStyle_none) is encoded with a value of 0. For all appearances, the condition code should be written as:

 if (!mIsTopWidgetWindow || mBorderStyle != eBorderStyle_none || mHideChrome) 

Incorrect check N2

 NS_IMETHODIMP nsWindowsRegKey::ReadStringValue(....) { .... DWORD type; .... if (type != REG_SZ && type == REG_EXPAND_SZ && type == REG_MULTI_SZ) return NS_ERROR_FAILURE; .... } 

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

The variable 'type' cannot be equal to two different values ​​at the same time. Simplify the code to make it easier to understand what the analyzer does not like:

 if (... && type == 2 && type == 7) 

This condition is always false.

Most likely, the code should be like this:

 if (type != REG_SZ && type != REG_EXPAND_SZ && type != REG_MULTI_SZ) 

Incorrect check N3

 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 warning: V547 Expression '- guess> = minEntry' is always true. Unsigned type value is always> = 0. ion.cpp 1112

The cycle will stop only when the necessary element is found. If there is no such element, the condition for stopping the loop will never be fulfilled, and the array will go beyond the bounds of the array.

The reason is that the variable 'guess' is of unsigned type. This means that the condition (--guess> = 0) is always true.

Inattention n1

 void WinUtils::LogW(const wchar_t *fmt, ...) { .... char* utf8 = new char[len+1]; memset(utf8, 0, sizeof(utf8)); .... } 

PVS-Studio warning: V579. It is possibly a mistake. Inspect the third argument. winutils.cpp 146

The expression 'sizeof (utf8)' returns the size of the pointer, not the size of the allocated memory buffer. The correct code is:

 memset(utf8, 0, sizeof(*utf8) * (len+1)); 

N2 inattention

As always, there is a code where the pointer is used at the beginning, and only then it is checked for equality to zero. I will cite in the article only one such case. The remaining errors the authors of Firefox can find by running the analyzer.

 void nsHttpTransaction::RestartVerifier::Set( int64_t contentLength, nsHttpResponseHead *head) { if (mSetup) return; if (head->Status() != 200) <<<<==== return; mContentLength = contentLength; if (head) { <<<<==== .... } 

PVS-Studio warning: V595 The 'head' pointer was used before it was verified against nullptr. Check lines: 1915, 1920. nshttptransaction.cpp 1915

At the beginning, the 'head' pointer is dereferenced in the expression "head-> Status ()". And then it is checked for equality to zero.

Inattention n3

 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 warning: V611 Consider inspecting operation logics behind the 'instanceData' variable. nptest.cpp 1029

Memory is allocated using the 'new' operator, and is released by calling the 'free' function. The result is an undefined program behavior. However, this is not terrible, since the code refers to the tests.

Inattention n4

Another code snippet related to tests. The variable 'device' may remain uninitialized:

 static ID3D10Device1* getD3D10Device() { ID3D10Device1 *device; .... if (createDXGIFactory1) { .... hr = createD3DDevice(...., &device); .... } return device; } 

PVS-Studio Warning: V614 Potentially uninitialized pointer 'device' used. nptest_windows.cpp 164

More thorough check

The purpose of the article was not to describe all the errors that PVS-Studio can reveal. Surely, I missed something. About something that did not write deliberately. For example, the analyzer issued many V610 warnings related to shift operations, which lead to undefined behavior. These warnings are of the same type, and it is not interesting to write about them.

The purpose of the article is to show the possibilities of static analysis, and to draw people's attention to our tool. A more thorough analysis of Firefox can be carried out by the developers themselves. It will be much easier for them to understand whether something is a mistake or not.

A note for Firefox developers. The project is quite large, so the PVS-Studio analyzer generates a lot of false positives. However, most of them refer to specific macros. You can very quickly reduce the number of false warnings several times by placing the corresponding comments in the code. How to suppress warnings related to certain macros is described in the documentation (see section “ Suppressing false warnings ”). If the Firefox developers are interested in purchasing a license for PVS-Studio, we, for our part, are also ready to take part in reducing false positives.

Conclusion

There were few suspicious code areas. The reason is that errors have already been found using other test methods and other static analyzers. Static code analyzers are most useful when used regularly, as they allow to detect an error even at the stage of writing code. This is discussed in more detail in the article " Leo Tolstoy and Static Code Analysis ".

I wish you all success in programming and fewer errors.

Additional links

1. PVS-Studio analyzer. Find a lot of stupid mistakes in the process of writing code. Save team time. Don't you make silly mistakes? Haha
2. We invite you to subscribe to our twitter: @Code_Analysis . We regularly publish links to interesting articles on programming and on checking new projects.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Firefox Easily Analyzed by PVS-Studio Standalone .