Third Chromium project code verification using PVS-Studio analyzer
The Chromium browser is developing very fast. For example, when in 2011 we first tested this project (solution), it consisted of 473 projects. Now, it already consists of 1169 projects. We were wondering if Google developers were able to maintain the highest quality of the code, at such a speed of Chromium development. Yes, they could.
Chromium is an open source web browser developed by Google. Based on Chromium, the Google Chrome browser is created. On the Get the Code page, you can learn how to download the source code for this project.
We used to check the Chromium project, about which there are two articles: the first check (05/23/2011), the second check (13/10/2011). And all the time they found errors. This is a subtle hint about the benefits of code analyzers.
Now (the source code of the project was downloaded in July 2013) Chromium consists of 1169 projects . The total amount of source code in C / C ++ is 260 megabytes . In addition to this, you can add another 450 megabytes of used external libraries.
If we take our first test of the Chromium project in 2011, we can see that the volume of external libraries as a whole has not changed. But the code of the project itself has grown significantly from 155 megabytes to 260 megabytes.
The PVS-Studio analyzer has the ability to search for functions with great cyclomatic complexity . Typically, such functions are candidates for refactoring. Having checked 1,160 projects, I naturally became interested in which of them can be called the champion in the nomination “the most complex function”.
The highest cyclomatic complexity, equal to 2782, belongs to the ValidateChunkAMD64 () function in the Chromium project. But she had to be disqualified from the competition. The function is located in the validator_x86_64.c file, which is auto-generated. It's a pity. And that would be an epic record holder. I have never come across such cyclomatic complexity.
Thus, the first three places receive the following functions:
If someone does not know what cyclomatic complexity is 1000, then let him not. Mental health will be better :). In general, a lot of this.
What can I say about the quality of the Chromium project code? The quality is still excellent. Yes, as in any large project, you can always find errors. But if you divide their number by the amount of code, their density will be negligible. This is very good code with very few errors. I give a medal for a clean code. The previous medal went to the Casablanca project (C ++ REST SDK) from Microsoft.
Figure 1. Medal to the creators of Chromium.
For the company, along with Chromium, third-party libraries included in it were checked. But to describe the errors found in them is not interesting. Moreover, I looked at the report very superficially. No, I'm not a bad person at all. I would look at you if you tried to fully study the report on the verification of 1169 projects. What I noticed during a quick look, I put in the database examples of errors . In this article I want to touch only on those errors that I managed to notice in the code of Chromium itself (its plugins and the like).
Since the Chromium project is so good, so why will I give examples of errors found? Everything is very simple. I want to demonstrate the power of the PVS-Studio analyzer. If he managed to find errors in Chromium, then the tool deserves your attention.
The analyzer managed to chew tens of thousands of files, with a total volume of 710 megabytes, and did not bend from this. Despite the fact that the project is developed by highly qualified developers and checked by various tools, PVS-Studio still managed to identify defects. This is a great achievement! And the last - he did it in a reasonable amount of time (about 5 hours) due to parallel verification (AMD FX-8320 / 3.50 GHz / eight-core processor, 16.0 GB RAM).
I propose to consider some code examples that my eyes settled on when viewing the report. I am sure that with a detailed study, it will be possible to find much more interesting things.
V501 There are identical sub-expressions to the left and to the right of the '-' operator: data_ [M02] - data_ [M02] matrix3_f.cc 128
We need to check that a 3x3 matrix is symmetrical.
Figure 2. 3x3 matrix.
To do this, compare the following elements:
Most likely, the code was written using Copy-Paste technology . As a result, cell M02 is compared to itself. Here is such a fun matrix class.
Another simple typo:
V501 There are identical sub-expressions 'field.form_control_type == "text"' to the left and to the right of the '||' operator. autocomplete_history_manager.cc 35
Two times the comparison with the string "text". This is suspicious. Perhaps one line is just superfluous. Or maybe there is no other comparison needed here.
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 500, 501. web_request_api_helpers.cc 500
It seems to me that this code should have passed text framed by double quotes. But in fact, this code does nothing. The condition is immediately false. For clarity, I will write a pseudocode to emphasize the essence of the error:
Most likely, they forgot to move the pointer to one character here and the code should look like this:
V530 The return value of function 'remove_if' is required to be utilized. shortcuts_provider.cc 136
To remove items from the container, use the std :: remove_if () function. But it is used incorrectly. In fact, remove_if () does not remove anything. It shifts the elements to the beginning and returns the iterator to the trash. You need to delete the garbage yourself by calling the erase () function on the containers. See also the Wikipedia article " Erase-remove idiom ".
The correct code is:
SOCKET in the Linux world, it is an integer SIGNATURE DATA type.
SOCKET in the Windows world, it is an integer UNSIGNABLE data type.
In Visual C ++ header files, the SOCKET type is declared like this:
However, they constantly forget about it and write code of the following form:
V547 Expression 'socket_ <0' is always false. Unsigned type value is never <0. tcp_server_socket_win.cc 48
An unsigned type variable is always greater than or equal to zero. This means that checking for 'socket_ <0' does not make sense. If during operation of the program the socket cannot be opened, this situation will not be processed correctly.
V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. label_button.cc 131
It seems to me that the code should have worked like this:
However, the expression “style &! Gfx :: Font :: BOLD” does not work as the programmer expects. The result of the operation! Gfx :: Font :: BOLD will be false. Or in other words, 0. The code written above is equivalent to this:
In order for the code to work correctly, the operation '~' should be used:
V623 Consider inspecting the '?:' Operator. A temporary object of the 'ScopedComPtr' type is being created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
It is unlikely that this code will lead to an error, but it deserves to be told about it. It seems to me that some programmers will learn about a new interesting trap in the C ++ language.
At first glance, everything is simple. Depending on the condition, we select the pointer 'src_surface' or one of the elements of the array 'scaler_scratch_surfaces_'. The array consists of objects of type base :: win :: ScopedComPtr <IDirect3DSurface9>, which can be automatically cast to a pointer to IDirect3DSurface9.
The devil is in the details.
The ternary operator '?:' Cannot return a different type depending on the condition. I will explain with a simple example.
The?: Operator returns a type of 'double'. As a result, the variable 'X' will also be of type double. But it is not important. It is important that the variable 'A' will be implicitly expanded to type 'double'!
The trouble arises if you write something like this:
As a result of the execution of this code, the variable 's' will indicate the data inside the temporary object of type CString. The problem is that this object will be immediately destroyed.
Now back to the source code of Chromium.
Here the following will happen if the condition 'i == 0' is fulfilled:
I do not know the logic of the program and the ScopedComPtr class and find it difficult to say whether negative consequences can occur or not. Most likely, in the constructor the counter of the number of links will be increased, and in the destructor reduced. And all will be well.
If this is not the case, then inadvertently you can get an invalid pointer or break the reference count.
In a word, even if there is no mistake, I will be glad if readers have learned something new. The ternary operator is far more dangerous than it sounds.
Here is another such suspicious place:
V623 Consider inspecting the '?:' Operator. A temporary object of the 'GenericScopedHandle' type is being created and subsequently destroyed. Check third operand. handle_policy.cc 81
V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 176, 178. handle_enumerator_win.cc 178
If the FILE_WRITE_EA flag is set, then the drain "\ tFILE_WRITE_EA \ n" will be added twice. Very suspicious code.
A similar strange picture can be observed here:
V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
The check “pf1.username_value <pf2.username_value” is repeated twice. Perhaps one line is just superfluous. Or perhaps they forgot to check something else, and a completely different condition should be written.
V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
There is something wrong with this loop. The loop performs only one iteration. At the end of the loop is an unconditional return statement. Possible reasons:
There are other strange loops that run only once:
V612 An unconditional 'break' within a loop. action_info.cc 76
V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224
V614 Potentially uninitialized variable 'result' used. ie_event_sink.cc 240
If the 'browser' pointer is zero, then the function will return an uninitialized variable.
Another piece of code:
V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth actual argument of the 'Bind' function. save_package.cc 1326 The
variable 'skip_dir_check' may remain uninitialized.
V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. trace_event_unittest.cc 57
Considering such a code, it is not clear whether curly brackets are forgotten here or not. Even if the code is correct, it should be corrected so that it does not lead other programmers into a thoughtful state.
Here are a couple of places with VERY suspicious code alignment:
Many programs have old legacy code written back in the days when the 'new' operator did not throw an exception. Previously, in case of insufficient memory, it returned a null pointer.
Chromium is no exception in this regard, and it contains such checks. The trouble is not that a meaningless check is performed. It is dangerous that with a null pointer some actions had to be performed before, or functions should return certain values. Now, due to the generation of an exception, the logic of work has changed. Code that should have been given control with a memory allocation error is now inactive.
Consider an example:
V668 There is no sense in testing the 'dict' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 There is no sense in testing the 'values' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
The first check “if (! dict) return NULL;” is likely to do no harm. But the second check is worse. If when creating an object using “new base :: ListValue ()” it is not possible to allocate memory, an exception 'std :: bad_alloc' will be thrown. This completes the operation of the GetDictValueStats () function.
As a result
will never destroy an object whose address is stored in the 'dict' variable.
The right solution here would be to refactor the code and use smart pointers.
Consider another piece of code:
V668 There is no sense in testing the 'ctx_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
In the event of a memory allocation error, the Destroy () function will not be called.
Further writing about this is not interesting. I will simply give a list of other potentially dangerous places I noticed in the code:
Unit tests are a wonderful technology for improving the quality of programs. However, the tests themselves often contain errors, as a result of which they do not fulfill their functions. Writing tests for tests is of course overkill. Static code analysis can help. I considered this idea in more detail in the article “ How Static Analysis Supplements TDD ”.
Here are some examples of errors that I encountered in tests for Chromium:
V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)' to the left and to the right of the '/' operator. test_audio_config.cc 56
Only one test will be executed in a loop. The error is that “sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts)” is one. The correct expression is "sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts [0])."
Another error test:
V549 The first argument of 'memcmp' function is equal to the second argument. entry_unittest.cc 393
The memcmp () function compares the buffer with itself. As a result, the test does not perform the required test. Apparently, here should be:
And here is a test that might suddenly break other tests:
V557 Array overrun is possible. The value of 'i' index could reach 2. shader_bench.cc 152
Perhaps earlier the array of 'painters' consisted of three elements. Now there are only two of them. And the value of the constant 'kNumPainters' remained equal to 3.
Some other places in the tests that I think deserve attention:
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer was utilized before it was verified against nullptr. Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get ()' pointer was utilized before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was utilized before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
In all programs, many defects are found in code designed to handle errors and respond to incorrect input data. This is because such places are difficult to test. And, as a rule, they are not tested. As a result, when an error occurs, programs begin to behave much more bizarre than planned.
Example:
V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of wchar_t type symbols is expected. fs.cc 17
If the variable 'size' is zero, the program will try to write a text message to the file. But this message is likely to contain billiards at the end. Moreover, this code can lead to access violation .
It uses the fprintf () function for writing. This function does not control the types of its arguments. She expects the last argument to be a pointer to a string. But in fact, the actual argument is a number (error code). This number will be converted to an address and how the program will behave further is unknown.
Once again, I was looking through the list of messages superficially. I have cited in this article only what caught my attention. Moreover, I noticed more than I wrote in the article. Describing everything, I get too long an article. And she is already too big.
I threw away so many pieces of code that I think will not be very interesting to the reader. I will give a couple of examples for clarity.
V595 The 'extension' pointer was utilized before it was verified against nullptr. Check lines: 277, 280. managed_user_service.cc 277
At the beginning, the 'extension' pointer is dereferenced in the expression "extension-> id ()". Then this pointer is checked for equality to zero.
Often, such code does not contain errors. It happens that the pointer simply cannot be zero and the check is redundant. Therefore, it makes no sense to list such places, since I can make a mistake and give out a completely working code as erroneous.
Here is another diagnostic example that I chose not to notice:
V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. webm_cluster_parser.cc 217
Formally, shifting a negative value results in undefined behavior . However, many compilers behave stably and exactly as the programmer expects. As a result, this code works successfully for a long time, although it is not required. I don’t feel like fighting this now, and I will miss such messages. For those who want to understand the issue in more detail, I recommend the article " Without knowing the ford, do not get into the water - part three ."
I am often asked the question:
In the articles you very cleverly give examples of errors found. But you do not say what is the total number of messages issued. Often, static analyzers produce a lot of false positives and among them it is almost impossible to find real errors. What is the situation with the PVS-Studio analyzer?
And I always don’t know what to immediately answer this question. I have two opposite answers: the first is a lot, the second is not enough. It all depends on how to approach the consideration of the issued message list. Now, using the example of Chromium, I will try to explain the essence of such duality.
PVS-Studio analyzer issues 3582Level 1 alerts (GA General Purpose Rules). This is a lot. And in most of these messages are false. If you go “head-on” and start immediately browsing the entire list, then it will get tired very quickly. And the impression will be terrible. Some solid false positives of the same type. Nothing interesting comes across. Bad tool.
The mistake of such a user is that even the minimum tool setup has not been completed. Yes, we try to make the PVS-Studio tool so that it works immediately after installation. We try so that nothing needs to be configured. A person should simply check his project and study the list of warnings issued.
However, this does not always work out. This did not work with Chromium. For example, a huge number of false messages occurred due to the macro 'DVLOG'. This macro is logging something. It was written cunningly and PVS-Studio mistakenly considered that there might be a mistake in it. Since the macro is very actively used, a lot of false messages turned out. We can say that how many times the DVLOG macro is used, so many false warnings will get into the report. Namely, about 2300 false messages “V501 There are identical sub-expressions .....” were issued about the macro .
You can suppress these warnings by writing in the header file next to the macro declaration, here is such a comment:
// - V: DVLOG: 501
Look, with such a simple action we will subtract 2300 false messages from 3582 messages.We immediately eliminated 65% of the messages . And now we do not need to view them in vain.
Without much effort, you can make several more of these refinements and settings. As a result, most of the false positives will disappear. In some cases, you should restart the analysis after tuning; in some, no. All this is described in more detail in the documentation section " Suppression of false warnings ". Specifically, in the case of errors in macros, you will have to restart the analysis.
I hope it’s clear now. Why the first answer - there are a lot of false positives. And why the second answer - there are few false positives. It all depends on whether a person is ready to spend at least a little time studying the product and ways to save himself from unnecessary messages.
I take this opportunity to say hello to my parents. Oh no, it's something wrong. Taking this opportunity, I want to convey greetings to programmers and recall that:
Chromium
Chromium is an open source web browser developed by Google. Based on Chromium, the Google Chrome browser is created. On the Get the Code page, you can learn how to download the source code for this project.
Some general information
We used to check the Chromium project, about which there are two articles: the first check (05/23/2011), the second check (13/10/2011). And all the time they found errors. This is a subtle hint about the benefits of code analyzers.
Now (the source code of the project was downloaded in July 2013) Chromium consists of 1169 projects . The total amount of source code in C / C ++ is 260 megabytes . In addition to this, you can add another 450 megabytes of used external libraries.
If we take our first test of the Chromium project in 2011, we can see that the volume of external libraries as a whole has not changed. But the code of the project itself has grown significantly from 155 megabytes to 260 megabytes.
For fun, let's calculate the cyclomatic complexity
The PVS-Studio analyzer has the ability to search for functions with great cyclomatic complexity . Typically, such functions are candidates for refactoring. Having checked 1,160 projects, I naturally became interested in which of them can be called the champion in the nomination “the most complex function”.
The highest cyclomatic complexity, equal to 2782, belongs to the ValidateChunkAMD64 () function in the Chromium project. But she had to be disqualified from the competition. The function is located in the validator_x86_64.c file, which is auto-generated. It's a pity. And that would be an epic record holder. I have never come across such cyclomatic complexity.
Thus, the first three places receive the following functions:
- WebKit library . The HTMLTokenizer :: nextToken () function in the htmltokenizer.cpp file. Cyclomatic difficulty 1106 .
- Mesa Library . The _mesa_glsl_lex () function in the glsl_lexer.cc file. Cyclomatic difficulty 1088 .
- Usrsctplib library (some unknown athlete). The sctp_setopt () function in the htmltokenizer.cpp file. Cyclomatic difficulty 1026 .
If someone does not know what cyclomatic complexity is 1000, then let him not. Mental health will be better :). In general, a lot of this.
Code quality
What can I say about the quality of the Chromium project code? The quality is still excellent. Yes, as in any large project, you can always find errors. But if you divide their number by the amount of code, their density will be negligible. This is very good code with very few errors. I give a medal for a clean code. The previous medal went to the Casablanca project (C ++ REST SDK) from Microsoft.
Figure 1. Medal to the creators of Chromium.
For the company, along with Chromium, third-party libraries included in it were checked. But to describe the errors found in them is not interesting. Moreover, I looked at the report very superficially. No, I'm not a bad person at all. I would look at you if you tried to fully study the report on the verification of 1169 projects. What I noticed during a quick look, I put in the database examples of errors . In this article I want to touch only on those errors that I managed to notice in the code of Chromium itself (its plugins and the like).
Since the Chromium project is so good, so why will I give examples of errors found? Everything is very simple. I want to demonstrate the power of the PVS-Studio analyzer. If he managed to find errors in Chromium, then the tool deserves your attention.
The analyzer managed to chew tens of thousands of files, with a total volume of 710 megabytes, and did not bend from this. Despite the fact that the project is developed by highly qualified developers and checked by various tools, PVS-Studio still managed to identify defects. This is a great achievement! And the last - he did it in a reasonable amount of time (about 5 hours) due to parallel verification (AMD FX-8320 / 3.50 GHz / eight-core processor, 16.0 GB RAM).
Some of the bugs found
I propose to consider some code examples that my eyes settled on when viewing the report. I am sure that with a detailed study, it will be possible to find much more interesting things.
Seen N1 - typos
Vector3dF
Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const
{
// The matrix must be symmetric.constfloat epsilon = std::numeric_limits<float>::epsilon();
if (std::abs(data_[M01] - data_[M10]) > epsilon ||
std::abs(data_[M02] - data_[M02]) > epsilon ||
std::abs(data_[M12] - data_[M21]) > epsilon) {
NOTREACHED();
return Vector3dF();
}
....
}V501 There are identical sub-expressions to the left and to the right of the '-' operator: data_ [M02] - data_ [M02] matrix3_f.cc 128
We need to check that a 3x3 matrix is symmetrical.
Figure 2. 3x3 matrix.
To do this, compare the following elements:
- M01 and M10
- M02 and M20
- M12 and M21
Most likely, the code was written using Copy-Paste technology . As a result, cell M02 is compared to itself. Here is such a fun matrix class.
Another simple typo:
boolIsTextField(const FormFieldData& field){
return
field.form_control_type == "text" ||
field.form_control_type == "search" ||
field.form_control_type == "tel" ||
field.form_control_type == "url" ||
field.form_control_type == "email" ||
field.form_control_type == "text";
}V501 There are identical sub-expressions 'field.form_control_type == "text"' to the left and to the right of the '||' operator. autocomplete_history_manager.cc 35
Two times the comparison with the string "text". This is suspicious. Perhaps one line is just superfluous. Or maybe there is no other comparison needed here.
Seen N2 - opposite conditions
staticvoidParseRequestCookieLine(
conststd::string& header_value,
ParsedRequestCookies* parsed_cookies){
std::string::const_iterator i = header_value.begin();
....
if (*i == '"') {
while (i != header_value.end() && *i != '"') ++i;
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 500, 501. web_request_api_helpers.cc 500
It seems to me that this code should have passed text framed by double quotes. But in fact, this code does nothing. The condition is immediately false. For clarity, I will write a pseudocode to emphasize the essence of the error:
if ( A == 'X' ) {
while ( .... && A != 'X' ) ....;Most likely, they forgot to move the pointer to one character here and the code should look like this:
if (*i == '"') {
++i;
while (i != header_value.end() && *i != '"') ++i;Seen N3 - failed delete elements
void ShortcutsProvider::DeleteMatchesWithURLs(
conststd::set<GURL>& urls)
{
std::remove_if(matches_.begin(),
matches_.end(),
RemoveMatchPredicate(urls));
listener_->OnProviderUpdate(true);
}V530 The return value of function 'remove_if' is required to be utilized. shortcuts_provider.cc 136
To remove items from the container, use the std :: remove_if () function. But it is used incorrectly. In fact, remove_if () does not remove anything. It shifts the elements to the beginning and returns the iterator to the trash. You need to delete the garbage yourself by calling the erase () function on the containers. See also the Wikipedia article " Erase-remove idiom ".
The correct code is:
matches_.erase(std::remove_if(.....), matches_.end());Seen N4 - Eternal Confusion with SOCKET
SOCKET in the Linux world, it is an integer SIGNATURE DATA type.
SOCKET in the Windows world, it is an integer UNSIGNABLE data type.
In Visual C ++ header files, the SOCKET type is declared like this:
typedef UINT_PTR SOCKET;However, they constantly forget about it and write code of the following form:
classNET_EXPORT_PRIVATETCPServerSocketWin {
....
SOCKET socket_;
....
};
int TCPServerSocketWin::Listen(....) {
....
socket_ = socket(address.GetSockAddrFamily(),
SOCK_STREAM, IPPROTO_TCP);
if (socket_ < 0) {
PLOG(ERROR) << "socket() returned an error";
return MapSystemError(WSAGetLastError());
}
....
}V547 Expression 'socket_ <0' is always false. Unsigned type value is never <0. tcp_server_socket_win.cc 48
An unsigned type variable is always greater than or equal to zero. This means that checking for 'socket_ <0' does not make sense. If during operation of the program the socket cannot be opened, this situation will not be processed correctly.
Seen N5 - confusion with operations ~ and!
enum FontStyle {
NORMAL = 0,
BOLD = 1,
ITALIC = 2,
UNDERLINE = 4,
};
void LabelButton::SetIsDefault(bool is_default) {
....
style = is_default ? style | gfx::Font::BOLD :
style & !gfx::Font::BOLD;
....
}V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. label_button.cc 131
It seems to me that the code should have worked like this:
- If the variable 'is_default' is true, then you always need to set the bit that is responsible for the BOLD style.
- If the variable 'is_default' is false, then you always need to clear the bit responsible for the BOLD style.
However, the expression “style &! Gfx :: Font :: BOLD” does not work as the programmer expects. The result of the operation! Gfx :: Font :: BOLD will be false. Or in other words, 0. The code written above is equivalent to this:
style = is_default ? style | gfx::Font::BOLD : 0;In order for the code to work correctly, the operation '~' should be used:
style = is_default ? style | gfx::Font::BOLD :
style & ~gfx::Font::BOLD;Seen N6 - suspicious creation of temporary objects
base::win::ScopedComPtr<IDirect3DSurface9> scaler_scratch_surfaces_[2];
bool AcceleratedSurfaceTransformer::ResizeBilinear(
IDirect3DSurface9* src_surface, ....)
{
....
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];
....
}V623 Consider inspecting the '?:' Operator. A temporary object of the 'ScopedComPtr' type is being created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
It is unlikely that this code will lead to an error, but it deserves to be told about it. It seems to me that some programmers will learn about a new interesting trap in the C ++ language.
At first glance, everything is simple. Depending on the condition, we select the pointer 'src_surface' or one of the elements of the array 'scaler_scratch_surfaces_'. The array consists of objects of type base :: win :: ScopedComPtr <IDirect3DSurface9>, which can be automatically cast to a pointer to IDirect3DSurface9.
The devil is in the details.
The ternary operator '?:' Cannot return a different type depending on the condition. I will explain with a simple example.
int A = 1;
auto X = v ? A : 2.0;The?: Operator returns a type of 'double'. As a result, the variable 'X' will also be of type double. But it is not important. It is important that the variable 'A' will be implicitly expanded to type 'double'!
The trouble arises if you write something like this:
CString s1(L"1");
wchar_t s2[] = L"2";
bool a = false;
constwchar_t *s = a ? s1 : s2;As a result of the execution of this code, the variable 's' will indicate the data inside the temporary object of type CString. The problem is that this object will be immediately destroyed.
Now back to the source code of Chromium.
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];Here the following will happen if the condition 'i == 0' is fulfilled:
- a temporary object of type base :: win :: ScopedComPtr <IDirect3DSurface9> will be constructed from the pointer 'src_surface';
- the temporary object will be implicitly cast to an IDirect3DSurface9 pointer and placed into the read_buffer variable;
- temporary object will be destroyed.
I do not know the logic of the program and the ScopedComPtr class and find it difficult to say whether negative consequences can occur or not. Most likely, in the constructor the counter of the number of links will be increased, and in the destructor reduced. And all will be well.
If this is not the case, then inadvertently you can get an invalid pointer or break the reference count.
In a word, even if there is no mistake, I will be glad if readers have learned something new. The ternary operator is far more dangerous than it sounds.
Here is another such suspicious place:
typedef
GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;
DWORD HandlePolicy::DuplicateHandleProxyAction(....)
{
....
base::win::ScopedHandle remote_target_process;
....
HANDLE target_process =
remote_target_process.IsValid() ?
remote_target_process : ::GetCurrentProcess();
....
}V623 Consider inspecting the '?:' Operator. A temporary object of the 'GenericScopedHandle' type is being created and subsequently destroyed. Check third operand. handle_policy.cc 81
Seen N7 - Duplicate Checks
string16 GetAccessString(HandleType handle_type,
ACCESS_MASK access){
....
if (access & FILE_WRITE_ATTRIBUTES)
output.append(ASCIIToUTF16("\tFILE_WRITE_ATTRIBUTES\n"));
if (access & FILE_WRITE_DATA)
output.append(ASCIIToUTF16("\tFILE_WRITE_DATA\n"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
....
}V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 176, 178. handle_enumerator_win.cc 178
If the FILE_WRITE_EA flag is set, then the drain "\ tFILE_WRITE_EA \ n" will be added twice. Very suspicious code.
A similar strange picture can be observed here:
staticboolPasswordFormComparator(const PasswordForm& pf1,
const PasswordForm& pf2){
if (pf1.submit_element < pf2.submit_element)
returntrue;
if (pf1.username_element < pf2.username_element)
returntrue;
if (pf1.username_value < pf2.username_value)
returntrue;
if (pf1.username_value < pf2.username_value)
returntrue;
if (pf1.password_element < pf2.password_element)
returntrue;
if (pf1.password_value < pf2.password_value)
returntrue;
returnfalse;
}V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
The check “pf1.username_value <pf2.username_value” is repeated twice. Perhaps one line is just superfluous. Or perhaps they forgot to check something else, and a completely different condition should be written.
Seen N8 - “one-time” cycles
ResourceProvider::ResourceId
PictureLayerImpl::ContentsResourceId() const
{
....
for (PictureLayerTilingSet::CoverageIterator iter(....);
iter;
++iter)
{
if (!*iter)
return0;
const ManagedTileState::TileVersion& tile_version = ....;
if (....)
return0;
if (iter.geometry_rect() != content_rect)
return0;
return tile_version.get_resource_id();
}
return0;
}V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
There is something wrong with this loop. The loop performs only one iteration. At the end of the loop is an unconditional return statement. Possible reasons:
- So it is conceived. But it is very doubtful. Why then create a loop, create an iterator, etc.
- Instead of one of the 'return', 'continue' should be written. But also doubtful.
- Most likely, before the last 'return' some condition is missing.
There are other strange loops that run only once:
scoped_ptr<ActionInfo> ActionInfo::Load(....)
{
....
for (base::ListValue::const_iterator iter = icons->begin();
iter != icons->end(); ++iter)
{
std::string path;
if (....);
return scoped_ptr<ActionInfo>();
}
result->default_icon.Add(....);
break;
}
....
}V612 An unconditional 'break' within a loop. action_info.cc 76
const BluetoothServiceRecord* BluetoothDeviceWin::GetServiceRecord(
conststd::string& uuid) const
{
for (ServiceRecordList::const_iterator iter =
service_record_list_.begin();
iter != service_record_list_.end();
++iter)
{
return *iter;
}
returnNULL;
}V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224
Seen N9 - uninitialized variables
HRESULT IEEventSink::Attach(IWebBrowser2* browser) {
DCHECK(browser);
HRESULT result;
if (browser) {
web_browser2_ = browser;
FindIEProcessId();
result = DispEventAdvise(web_browser2_, &DIID_DWebBrowserEvents2);
}
return result;
}V614 Potentially uninitialized variable 'result' used. ie_event_sink.cc 240
If the 'browser' pointer is zero, then the function will return an uninitialized variable.
Another piece of code:
void SavePackage::GetSaveInfo() {
....
bool skip_dir_check;
....
if (....) {
....->GetSaveDir(...., &skip_dir_check);
}
....
BrowserThread::PostTask(BrowserThread::FILE,
FROM_HERE,
base::Bind(..., skip_dir_check, ...));
}V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth actual argument of the 'Bind' function. save_package.cc 1326 The
variable 'skip_dir_check' may remain uninitialized.
Noticed N10 - code alignment does not match the logic of its operation
voidOnTraceNotification(int notification){
if (notification & TraceLog::EVENT_WATCH_NOTIFICATION)
++event_watch_notification_;
notifications_received_ |= notification;
}V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. trace_event_unittest.cc 57
Considering such a code, it is not clear whether curly brackets are forgotten here or not. Even if the code is correct, it should be corrected so that it does not lead other programmers into a thoughtful state.
Here are a couple of places with VERY suspicious code alignment:
- nss_memio.c 152
- nss_memio.c 184
Noticed N11 - pointer check after new
Many programs have old legacy code written back in the days when the 'new' operator did not throw an exception. Previously, in case of insufficient memory, it returned a null pointer.
Chromium is no exception in this regard, and it contains such checks. The trouble is not that a meaningless check is performed. It is dangerous that with a null pointer some actions had to be performed before, or functions should return certain values. Now, due to the generation of an exception, the logic of work has changed. Code that should have been given control with a memory allocation error is now inactive.
Consider an example:
static base::DictionaryValue* GetDictValueStats(
const webrtc::StatsReport& report){
....
DictionaryValue* dict = new base::DictionaryValue();
if (!dict)
returnNULL;
dict->SetDouble("timestamp", report.timestamp);
base::ListValue* values = new base::ListValue();
if (!values) {
delete dict;
returnNULL;
}
....
}V668 There is no sense in testing the 'dict' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 There is no sense in testing the 'values' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
The first check “if (! dict) return NULL;” is likely to do no harm. But the second check is worse. If when creating an object using “new base :: ListValue ()” it is not possible to allocate memory, an exception 'std :: bad_alloc' will be thrown. This completes the operation of the GetDictValueStats () function.
As a result
if (!values) {
delete dict;
returnNULL;
}will never destroy an object whose address is stored in the 'dict' variable.
The right solution here would be to refactor the code and use smart pointers.
Consider another piece of code:
bool Target::Init() {
{
....
ctx_ = newuint8_t[abi_->GetContextSize()];
if (NULL == ctx_) {
Destroy();
returnfalse;
}
....
}V668 There is no sense in testing the 'ctx_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
In the event of a memory allocation error, the Destroy () function will not be called.
Further writing about this is not interesting. I will simply give a list of other potentially dangerous places I noticed in the code:
- 'data' pointer. target.cc 109
- 'page_data' pointer. mock_printer.cc 229
- 'module' pointer. pepper_entrypoints.cc 39
- 'c_protocols' pointer. websocket.cc 44
- 'type_enum' pointer. pin_base_win.cc 96
- 'pin_enum' pointer. filter_base_win.cc 75
- 'port_data'. port_monitor.cc 388
- 'xcv_data' pointer. port_monitor.cc 552
- 'monitor_data'. port_monitor.cc 625
- 'sender_' pointer. crash_service.cc 221
- 'cache' pointer. crash_cache.cc 269
- 'current_browser' pointer. print_preview_dialog_controller.cc 403
- 'udp_socket' pointer. network_stats.cc 212
- 'popup_' pointer. try_chrome_dialog_view.cc 90
Seen N12 - tests that test poorly
Unit tests are a wonderful technology for improving the quality of programs. However, the tests themselves often contain errors, as a result of which they do not fulfill their functions. Writing tests for tests is of course overkill. Static code analysis can help. I considered this idea in more detail in the article “ How Static Analysis Supplements TDD ”.
Here are some examples of errors that I encountered in tests for Chromium:
std::string TestAudioConfig::TestValidConfigs() {
....
staticconstuint32_t kRequestFrameCounts[] = {
PP_AUDIOMINSAMPLEFRAMECOUNT,
PP_AUDIOMAXSAMPLEFRAMECOUNT,
1024,
2048,
4096
};
....
for (size_t j = 0;
j < sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts);
j++) {
....
}V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)' to the left and to the right of the '/' operator. test_audio_config.cc 56
Only one test will be executed in a loop. The error is that “sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts)” is one. The correct expression is "sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts [0])."
Another error test:
void DiskCacheEntryTest::ExternalSyncIOBackground(....) {
....
scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize2));
....
EXPECT_EQ(0, memcmp(buffer2->data(), buffer2->data(), 10000));
....
}V549 The first argument of 'memcmp' function is equal to the second argument. entry_unittest.cc 393
The memcmp () function compares the buffer with itself. As a result, the test does not perform the required test. Apparently, here should be:
EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000));And here is a test that might suddenly break other tests:
staticconstint kNumPainters = 3;
staticconststruct {constchar* name;
GPUPainter* painter;
} painters[] = {
{ "CPU CSC + GPU Render", new CPUColorPainter() },
{ "GPU CSC/Render", new GPUColorWithLuminancePainter() },
};
intmain(int argc, char** argv){
....
// Run GPU painter tests.for (int i = 0; i < kNumPainters; i++) {
scoped_ptr<GPUPainter> painter(painters[i].painter);
....
}V557 Array overrun is possible. The value of 'i' index could reach 2. shader_bench.cc 152
Perhaps earlier the array of 'painters' consisted of three elements. Now there are only two of them. And the value of the constant 'kNumPainters' remained equal to 3.
Some other places in the tests that I think deserve attention:
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer was utilized before it was verified against nullptr. Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get ()' pointer was utilized before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was utilized before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
Seen N13 - Function with Variable Number of Arguments
In all programs, many defects are found in code designed to handle errors and respond to incorrect input data. This is because such places are difficult to test. And, as a rule, they are not tested. As a result, when an error occurs, programs begin to behave much more bizarre than planned.
Example:
DWORD GetLastError(VOID);
voidTryOpenFile(wchar_t *path, FILE *output){
wchar_t path_expanded[MAX_PATH] = {0};
DWORD size = ::ExpandEnvironmentStrings(
path, path_expanded, MAX_PATH - 1);
if (!size) {
fprintf(output,
"[ERROR] Cannot expand \"%S\". Error %S.\r\n",
path, ::GetLastError());
}
....
}V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of wchar_t type symbols is expected. fs.cc 17
If the variable 'size' is zero, the program will try to write a text message to the file. But this message is likely to contain billiards at the end. Moreover, this code can lead to access violation .
It uses the fprintf () function for writing. This function does not control the types of its arguments. She expects the last argument to be a pointer to a string. But in fact, the actual argument is a number (error code). This number will be converted to an address and how the program will behave further is unknown.
Unnoticed
Once again, I was looking through the list of messages superficially. I have cited in this article only what caught my attention. Moreover, I noticed more than I wrote in the article. Describing everything, I get too long an article. And she is already too big.
I threw away so many pieces of code that I think will not be very interesting to the reader. I will give a couple of examples for clarity.
bool ManagedUserService::UserMayLoad(
const extensions::Extension* extension,
string16* error) const
{
if (extension_service &&
extension_service->GetInstalledExtension(extension->id()))
returntrue;
if (extension) {
bool was_installed_by_default =
extension->was_installed_by_default();
.....
}
}V595 The 'extension' pointer was utilized before it was verified against nullptr. Check lines: 277, 280. managed_user_service.cc 277
At the beginning, the 'extension' pointer is dereferenced in the expression "extension-> id ()". Then this pointer is checked for equality to zero.
Often, such code does not contain errors. It happens that the pointer simply cannot be zero and the check is redundant. Therefore, it makes no sense to list such places, since I can make a mistake and give out a completely working code as erroneous.
Here is another diagnostic example that I chose not to notice:
bool WebMClusterParser::ParseBlock(....)
{
int timecode = buf[1] << 8 | buf[2];
....
if (timecode & 0x8000)
timecode |= (-1 << 16);
....
}V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. webm_cluster_parser.cc 217
Formally, shifting a negative value results in undefined behavior . However, many compilers behave stably and exactly as the programmer expects. As a result, this code works successfully for a long time, although it is not required. I don’t feel like fighting this now, and I will miss such messages. For those who want to understand the issue in more detail, I recommend the article " Without knowing the ford, do not get into the water - part three ."
About false positives
I am often asked the question:
In the articles you very cleverly give examples of errors found. But you do not say what is the total number of messages issued. Often, static analyzers produce a lot of false positives and among them it is almost impossible to find real errors. What is the situation with the PVS-Studio analyzer?
And I always don’t know what to immediately answer this question. I have two opposite answers: the first is a lot, the second is not enough. It all depends on how to approach the consideration of the issued message list. Now, using the example of Chromium, I will try to explain the essence of such duality.
PVS-Studio analyzer issues 3582Level 1 alerts (GA General Purpose Rules). This is a lot. And in most of these messages are false. If you go “head-on” and start immediately browsing the entire list, then it will get tired very quickly. And the impression will be terrible. Some solid false positives of the same type. Nothing interesting comes across. Bad tool.
The mistake of such a user is that even the minimum tool setup has not been completed. Yes, we try to make the PVS-Studio tool so that it works immediately after installation. We try so that nothing needs to be configured. A person should simply check his project and study the list of warnings issued.
However, this does not always work out. This did not work with Chromium. For example, a huge number of false messages occurred due to the macro 'DVLOG'. This macro is logging something. It was written cunningly and PVS-Studio mistakenly considered that there might be a mistake in it. Since the macro is very actively used, a lot of false messages turned out. We can say that how many times the DVLOG macro is used, so many false warnings will get into the report. Namely, about 2300 false messages “V501 There are identical sub-expressions .....” were issued about the macro .
You can suppress these warnings by writing in the header file next to the macro declaration, here is such a comment:
// - V: DVLOG: 501
Look, with such a simple action we will subtract 2300 false messages from 3582 messages.We immediately eliminated 65% of the messages . And now we do not need to view them in vain.
Without much effort, you can make several more of these refinements and settings. As a result, most of the false positives will disappear. In some cases, you should restart the analysis after tuning; in some, no. All this is described in more detail in the documentation section " Suppression of false warnings ". Specifically, in the case of errors in macros, you will have to restart the analysis.
I hope it’s clear now. Why the first answer - there are a lot of false positives. And why the second answer - there are few false positives. It all depends on whether a person is ready to spend at least a little time studying the product and ways to save himself from unnecessary messages.
Parting words to readers
I take this opportunity to say hello to my parents. Oh no, it's something wrong. Taking this opportunity, I want to convey greetings to programmers and recall that:
- The answer to the question “Did you report any bugs found in the project to the developers?” Can be found in the note “ Answers to questions that are often asked after reading our articles .”
- It is best to contact us and ask questions using the feedback form on our website. Please do not use twitter for this, comments on articles somewhere on third-party sites, and so on.
- I invite you to join our twitter: @Code_Analysis . I constantly collect and publish interesting links on programming topics and the C ++ language.