Third check of the Chromium project code using the PVS-Studio analyzer
Chromium browser is developing very quickly. For example, when in 2011 we checked this project for the first time (solution), it consisted of 473 projects. Now, it already consists of 1169 projects. We wondered if the Google developers were able to maintain the highest quality of the code, at this speed of Chromium development. Yes, they could.
Chromium is an open source web browser developed by Google. Chromium is based on the Google Chrome browser. On the page " Get the Code " you can learn how to download the source code of this project.
Previously, we have already checked the project Chromium, about which there are two articles: the first check (05/23/2011), the second check (10/13/2011). And all the time they found mistakes. 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 C / C ++ source code is 260 megabytes . In addition, you can add another 450 megabytes of used external libraries.
')
If we take our first test of the Chromium project in 2011, then 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.
In the PVS-Studio analyzer, you can search for functions with high cyclomatic complexity . As a rule, such functions are candidates for refactoring. Having checked 1160 projects, I naturally wondered which of them could be called the record holder in the nomination “the most difficult function”.
The most maximal cyclomatic complexity, equal to 2782, belongs to the function ValidateChunkAMD64 () in the Chromium project. But she had to be disqualified from the competition. The function is in the file validator_x86_64.c, which is autogenerated. It's a pity. And that would be an epic record holder. I didn’t come across such a cyclomatic complexity.
Thus, the first three places get the following functions:
If someone does not know what cyclomatic complexity 1000 is, then let him not know. Mental health will be better :). In general, a lot of it.
What can I say about the quality of the Chromium project code? The quality is still great. Yes, as in any big project, you can always find mistakes. But if you divide their number by the amount of code, their density will be negligible. This is a very good code with a very small number of errors. I hand over a medal for a clean code. The previous medal went to the project Casablanca (C ++ REST SDK) from Microsoft.
Figure 1. Medal to the creators of Chromium.
For the company together with Chromium, the third-party libraries included in it were checked. But to describe the errors found in them is not interesting. Moreover, I looked through the report very superficially. No, I'm not a bad person at all. I would look at you if you tried to fully examine the verification report of 1169 projects. What I noticed during a quick scan, I put examples of errors in the database . In this article I want to touch on only those errors that I managed to notice in the code of Chromium itself (its plug-ins and the like).
Since the Chromium project is so good, why should 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 die from this. Despite the fact that the project is being 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 time (about 5 hours) due to parallel testing (AMD FX-8320 / 3.50 GHz / eight-core processor, 16.0 GB RAM).
I propose to consider some examples of the code on which I looked when viewing the report. I am sure that with detailed study, it will be possible to find a lot more interesting.
V501 There are identical - the operator: data_ [M02] - data_ [M02] matrix3_f.cc 128
We need to check that the 3x3 matrix is ​​symmetric.
Figure 2. 3x3 matrix.
To do this, compare the following elements:
Most likely, the code was written using the Copy-Paste technology . As a result, cell M02 is compared to itself. Here is such a fun class matrix.
Another simple typo:
V501 There are identical sub-expressions' field.form_control_type == "text" operator. autocomplete_history_manager.cc 35
Two times there is a 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 skipped text framed with double quotes. But in fact, this code does nothing. The condition is immediately false. For clarity, I will write pseudocode to emphasize the essence of the error:
Most likely, here you forgot to move the pointer one character and the code should look like this:
V530 The return value of the 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 used incorrectly. In fact, remove_if () does not remove anything. It shifts elements to the beginning and returns an iterator to the garbage. You need to remove garbage yourself by calling the erase () function on containers. See also the Wikipedia article " Erase-remove idiom ".
The correct code is:
SOCKET in the Linux world, this is an integer SIGNAL data type.
SOCKET in the world of Windows, is an integer with no character type.
In Visual C ++ header files, the SOCKET type is declared as follows:
However, they constantly forget about it and write the following code:
V547 Expression 'socket_ <0' is always false. Unsigned type value is never <0. tcp_server_socket_win.cc 48
An unsigned variable is always greater than or equal to zero. This means that checking the 'socket_ <0' does not make sense. If the program fails to open the socket, this situation will not be processed correctly.
V564 The '&' operator is applied to bool type value. You've probably forgotten to include the operator. label_button.cc 131
As it seems to me, the code should work 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:
For the code to work correctly, you should use the operation '~':
V623 Consider inspecting the '?:' Operator. A temporary object of the ScopedComPtr type was created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
This code is unlikely to 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 C ++.
At first glance, everything is simple. Depending on the condition, we select the 'src_surface' pointer or one of the elements of the 'scaler_scratch_surfaces_' array. The array consists of objects of the type base :: win :: ScopedComPtr <IDirect3DSurface9>, which can be automatically led to a pointer to IDirect3DSurface9.
The devil is in the details.
The ternary operator '?:' Cannot return a different type depending on the condition. Let me explain with a simple example.
Operator?: Returns type '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 the type 'double'!
The trouble arises if you write something like this:
As a result of executing this code, the variable 's' will point to the data inside the temporary object of the CString type. The problem is that this object will be immediately destroyed.
We now return to the source code Chromium.
Here the following will occur if the condition 'i == 0' is fulfilled:
I do not know the logic of the program and the class ScopedComPtr and I find it difficult to say whether negative consequences can arise or not. Most likely, in the designer the counter of the number of references will be increased, and in the destructor it will be reduced. And all will be well.
If this is not the case, then inadvertently you can get a non-valid pointer or break the reference count.
In a word, even if there is no mistake, I will be glad if readers learned something new. The ternary operator is more dangerous than it seems.
Here is another such suspicious place:
V623 Consider inspecting the '?:' Operator. A temporary object of the 'GenericScopedHandle' type is subsequently created and subsequently destroyed. Check third operand. handle_policy.cc 81
V581 The conditional expressions of the 'if' are 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 "\ tFILE_WRITE_EA \ n" sink will be added twice. Very suspicious code.
A similar strange picture can be seen here:
V581 The conditional expressions of the 'if' are located alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
The “pf1.username_value <pf2.username_value” check is repeated twice. Possible, one line is just extra. Or perhaps they forgot to check something else, and a completely different condition must be written.
V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
There is something wrong with this cycle. The loop performs only one iteration. At the end of the loop is the unconditional return statement. Possible reasons:
There were other strange cycles 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, the function will return an uninitialized variable.
Another code snippet:
V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth argument. 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. It is possible that curly brackets are missing. trace_event_unittest.cc 57
Considering such code, it is not clear whether braces are forgotten here or not. Even if the code is correct, it should be corrected so that it does not introduce other programmers into a thoughtful state.
Here are a couple of places with VERY suspicious code alignment:
In many programs, the old inherited code lives, written back in the days when the 'new' operator did not throw an exception. Previously, in the event of insufficient memory, he returned a null pointer.
Chromium in this regard is no exception, and it includes such checks. The trouble is not that a meaningless check is performed. It is dangerous that with a null pointer, before any actions or functions had to be performed, certain values ​​should be returned. Now, due to the generation of the exception, the logic of the work has changed. The code that was supposed to get control in case of a memory allocation error is now inactive.
Consider an example:
V668 It has been allocated to the use of the operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 against '' '' '' allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
The first check “if (! Dict) return NULL;” most likely will not bring harm. But with the second check the situation is worse. If the creation of an object using “new base :: ListValue ()” fails to allocate memory, then an 'std :: bad_alloc' exception will be thrown. This is the end of the GetDictValueStats () function.
As a result, this code:
never destroy an object whose address is stored in the 'dict' variable.
The correct solution here will be to refactor the code and use smart pointers.
Consider another code snippet:
V668 has been defined as using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
In case of a memory allocation error, the Destroy () function will not be called.
Further writing about it is not interesting. I will simply provide a list of other potentially dangerous places I have noticed in the code:
Unit tests are a great technology for improving the quality of programs. However, the tests themselves often contain errors, as a result of which they do not perform their functions. Writing tests for tests is of course a bust. Can help static code analysis. I considered this idea in more detail in the article “ How static analysis complements TDD ”.
I will give some examples of errors I encountered in tests for Chromium:
V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)'. test_audio_config.cc 56
Only one test will run in the loop. The error is that "sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts)" equals one. The correct expression is: “sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts [0])”.
Another erroneous test:
V549 The first argument of the memcmp function is equal to the second argument. entry_unittest.cc 393
The “memcmp ()” function compares the buffer to itself. As a result, the test does not perform the required verification. Apparently, there should be:
But a test that may unexpectedly break other tests:
V557 Array overrun is possible. Shader_bench.cc 152
Perhaps earlier the array 'painters' consisted of three elements. Now there are only two. And the value of the constant 'kNumPainters' remains equal to 3.
Some other places in the tests that I think deserve attention:
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get ()' pointer was used before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was used before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was used before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was used before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was used before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
In all programs, many defects are found in the code intended for handling errors and responding 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, the programs begin to behave much more bizarre than planned.
Example:
V576 Incorrect format. Consider checking the fourth argument of the fprintf function. 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 a billiberd at the end. Moreover, this code can lead to access violation .
For recording, it uses the fprintf () function. 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 it is unknown how the program will behave further.
Once again, I looked through the list of messages superficially. I brought in this article only what caught my attention. Moreover, I noticed more than I wrote in the article. Describing everything, I will get an article too long. And she is already too big.
I dropped a lot of code fragments that I think will not be very interesting to the reader. I will give a couple of examples for clarity.
V595 'extension pointer extension nu pointer Check lines: 277, 280. managed_user_service.cc 277
In the beginning, the 'extension' pointer is dereferenced in the expression "extension-> id ()". Then this pointer is checked for equality to zero.
Often, this 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 could be mistaken and give a completely working code for the wrong one.
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, a negative value shift results in undefined behavior . However, many compilers are stable and exactly as the programmer expects. As a result, this code works long and successfully, although it is not required. I don’t want to fight this now, and I’ll miss such messages. For those who want to understand the question in more detail, I recommend the article " Without knowing the ford, do not climb into the water - part three ."
I am often asked a question:
In the articles you are 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 about the PVS-Studio analyzer?
And I always do not know what to immediately answer this question. I have two opposite answers: the first - a lot, the second - a little. 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.
The PVS-Studio analyzer issued 3582 first-level warnings (GA rule set). This is a lot. And most of these messages are false. If you go to the "forehead" and start immediately to view the entire list, then it will get very tired very quickly. And the impression will be terrible. Some solid homogeneous false positives. Nothing interesting comes across. Bad tool.
, . , PVS-Studio , . , . .
. Chromium. , - 'DVLOG'. - . PVS-Studio , . , , . , DVLOG, . , 2300 «V501 There are identical sub-expressions.....».
, , , :
//-V:DVLOG:501
, 3582 , 2300 . 65% . .
. , . , . " ". , , .
. — . — . , .
, . , - . , , :
Chromium
Chromium is an open source web browser developed by Google. Chromium is based on the Google Chrome browser. On the page " Get the Code " you can learn how to download the source code of this project.
Some general information
Previously, we have already checked the project Chromium, about which there are two articles: the first check (05/23/2011), the second check (10/13/2011). And all the time they found mistakes. 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 C / C ++ source code is 260 megabytes . In addition, you can add another 450 megabytes of used external libraries.
')
If we take our first test of the Chromium project in 2011, then 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 the sake of interest, we consider the cyclomatic complexity
In the PVS-Studio analyzer, you can search for functions with high cyclomatic complexity . As a rule, such functions are candidates for refactoring. Having checked 1160 projects, I naturally wondered which of them could be called the record holder in the nomination “the most difficult function”.
The most maximal cyclomatic complexity, equal to 2782, belongs to the function ValidateChunkAMD64 () in the Chromium project. But she had to be disqualified from the competition. The function is in the file validator_x86_64.c, which is autogenerated. It's a pity. And that would be an epic record holder. I didn’t come across such a cyclomatic complexity.
Thus, the first three places get the following functions:
- WebKit library. The HTMLTokenizer :: nextToken () function in the htmltokenizer.cpp file. Cyclomatic complexity 1106 .
- Mesa library. The _mesa_glsl_lex () function in the glsl_lexer.cc file. Cyclomatic complexity 1088 .
- Library usrsctplib (some unknown sportsman). The sctp_setopt () function in the htmltokenizer.cpp file. Cyclomatic complexity 1026 .
If someone does not know what cyclomatic complexity 1000 is, then let him not know. Mental health will be better :). In general, a lot of it.
Quality code
What can I say about the quality of the Chromium project code? The quality is still great. Yes, as in any big project, you can always find mistakes. But if you divide their number by the amount of code, their density will be negligible. This is a very good code with a very small number of errors. I hand over a medal for a clean code. The previous medal went to the project Casablanca (C ++ REST SDK) from Microsoft.
Figure 1. Medal to the creators of Chromium.
For the company together with Chromium, the third-party libraries included in it were checked. But to describe the errors found in them is not interesting. Moreover, I looked through the report very superficially. No, I'm not a bad person at all. I would look at you if you tried to fully examine the verification report of 1169 projects. What I noticed during a quick scan, I put examples of errors in the database . In this article I want to touch on only those errors that I managed to notice in the code of Chromium itself (its plug-ins and the like).
Since the Chromium project is so good, why should 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 die from this. Despite the fact that the project is being 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 time (about 5 hours) due to parallel testing (AMD FX-8320 / 3.50 GHz / eight-core processor, 16.0 GB RAM).
Some of the errors found
I propose to consider some examples of the code on which I looked when viewing the report. I am sure that with detailed study, it will be possible to find a lot more interesting.
Noted N1 - typos
Vector3dF Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const { // The matrix must be symmetric. const float 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 - the operator: data_ [M02] - data_ [M02] matrix3_f.cc 128
We need to check that the 3x3 matrix is ​​symmetric.
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 the Copy-Paste technology . As a result, cell M02 is compared to itself. Here is such a fun class matrix.
Another simple typo:
bool IsTextField(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" operator. autocomplete_history_manager.cc 35
Two times there is a comparison with the string "text". This is suspicious. Perhaps one line is just superfluous. Or maybe there is no other comparison needed here.
N2 seen - opposite conditions
static void ParseRequestCookieLine( const std::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 skipped text framed with double quotes. But in fact, this code does nothing. The condition is immediately false. For clarity, I will write pseudocode to emphasize the essence of the error:
if ( A == 'X' ) { while ( .... && A != 'X' ) ....; Most likely, here you forgot to move the pointer one character and the code should look like this:
if (*i == '"') { ++i; while (i != header_value.end() && *i != '"') ++i; Noted N3 - unsuccessful deletion of elements
void ShortcutsProvider::DeleteMatchesWithURLs( const std::set<GURL>& urls) { std::remove_if(matches_.begin(), matches_.end(), RemoveMatchPredicate(urls)); listener_->OnProviderUpdate(true); } V530 The return value of the 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 used incorrectly. In fact, remove_if () does not remove anything. It shifts elements to the beginning and returns an iterator to the garbage. You need to remove garbage yourself by calling the erase () function on containers. See also the Wikipedia article " Erase-remove idiom ".
The correct code is:
matches_.erase(std::remove_if(.....), matches_.end()); N4 - Eternal confusion with SOCKET
SOCKET in the Linux world, this is an integer SIGNAL data type.
SOCKET in the world of Windows, is an integer with no character type.
In Visual C ++ header files, the SOCKET type is declared as follows:
typedef UINT_PTR SOCKET; However, they constantly forget about it and write the following code:
class NET_EXPORT_PRIVATE TCPServerSocketWin { .... 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 variable is always greater than or equal to zero. This means that checking the 'socket_ <0' does not make sense. If the program fails to open the socket, this situation will not be processed correctly.
Noted 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 the operator. label_button.cc 131
As it seems to me, the code should work like this:
- If the variable 'is_default' is true, then you should always set the bit responsible for the BOLD style.
- If the variable 'is_default' is false, then you always need to reset 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; For the code to work correctly, you should use the operation '~':
style = is_default ? style | gfx::Font::BOLD : style & ~gfx::Font::BOLD; Noted 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 was created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
This code is unlikely to 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 C ++.
At first glance, everything is simple. Depending on the condition, we select the 'src_surface' pointer or one of the elements of the 'scaler_scratch_surfaces_' array. The array consists of objects of the type base :: win :: ScopedComPtr <IDirect3DSurface9>, which can be automatically led to a pointer to IDirect3DSurface9.
The devil is in the details.
The ternary operator '?:' Cannot return a different type depending on the condition. Let me explain with a simple example.
int A = 1; auto X = v ? A : 2.0; Operator?: Returns type '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 the type 'double'!
The trouble arises if you write something like this:
CString s1(L"1"); wchar_t s2[] = L"2"; bool a = false; const wchar_t *s = a ? s1 : s2; As a result of executing this code, the variable 's' will point to the data inside the temporary object of the CString type. The problem is that this object will be immediately destroyed.
We now return to the source code Chromium.
IDirect3DSurface9* read_buffer = (i == 0) ? src_surface : scaler_scratch_surfaces_[read_buffer_index]; Here the following will occur if the condition 'i == 0' is fulfilled:
- from the 'src_surface' pointer, a temporary object of the type base :: win :: ScopedComPtr <IDirect3DSurface9> will be constructed;
- the temporary object will be implicitly cast to a pointer of type IDirect3DSurface9 and placed in the read_buffer variable;
- temporary object will be destroyed.
I do not know the logic of the program and the class ScopedComPtr and I find it difficult to say whether negative consequences can arise or not. Most likely, in the designer the counter of the number of references will be increased, and in the destructor it will be reduced. And all will be well.
If this is not the case, then inadvertently you can get a non-valid pointer or break the reference count.
In a word, even if there is no mistake, I will be glad if readers learned something new. The ternary operator is more dangerous than it seems.
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 subsequently created and subsequently destroyed. Check third operand. handle_policy.cc 81
Noted 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' are 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 "\ tFILE_WRITE_EA \ n" sink will be added twice. Very suspicious code.
A similar strange picture can be seen here:
static bool PasswordFormComparator(const PasswordForm& pf1, const PasswordForm& pf2) { if (pf1.submit_element < pf2.submit_element) return true; if (pf1.username_element < pf2.username_element) return true; if (pf1.username_value < pf2.username_value) return true; if (pf1.username_value < pf2.username_value) return true; if (pf1.password_element < pf2.password_element) return true; if (pf1.password_value < pf2.password_value) return true; return false; } V581 The conditional expressions of the 'if' are located alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
The “pf1.username_value <pf2.username_value” check is repeated twice. Possible, one line is just extra. Or perhaps they forgot to check something else, and a completely different condition must be written.
Noted N8 - “disposable” cycles
ResourceProvider::ResourceId PictureLayerImpl::ContentsResourceId() const { .... for (PictureLayerTilingSet::CoverageIterator iter(....); iter; ++iter) { if (!*iter) return 0; const ManagedTileState::TileVersion& tile_version = ....; if (....) return 0; if (iter.geometry_rect() != content_rect) return 0; return tile_version.get_resource_id(); } return 0; } V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
There is something wrong with this cycle. The loop performs only one iteration. At the end of the loop is the unconditional return statement. Possible reasons:
- So conceived. But it is very doubtful. Why then create a loop, create an iterator, etc.?
- Instead of one of 'return', 'continue' should be written. But also doubtful.
- Most likely, before the last 'return' some condition is missing.
There were other strange cycles 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( const std::string& uuid) const { for (ServiceRecordList::const_iterator iter = service_record_list_.begin(); iter != service_record_list_.end(); ++iter) { return *iter; } return NULL; } V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224
Noted 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, the function will return an uninitialized variable.
Another code snippet:
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 argument. save_package.cc 1326
The variable 'skip_dir_check' may remain uninitialized.
Noted N10 - alignment of the code does not match the logic of its work
void OnTraceNotification(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. It is possible that curly brackets are missing. trace_event_unittest.cc 57
Considering such code, it is not clear whether braces are forgotten here or not. Even if the code is correct, it should be corrected so that it does not introduce 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
Noted N11 - check pointer after new
In many programs, the old inherited code lives, written back in the days when the 'new' operator did not throw an exception. Previously, in the event of insufficient memory, he returned a null pointer.
Chromium in this regard is no exception, and it includes such checks. The trouble is not that a meaningless check is performed. It is dangerous that with a null pointer, before any actions or functions had to be performed, certain values ​​should be returned. Now, due to the generation of the exception, the logic of the work has changed. The code that was supposed to get control in case of 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) return NULL; dict->SetDouble("timestamp", report.timestamp); base::ListValue* values = new base::ListValue(); if (!values) { delete dict; return NULL; } .... } V668 It has been allocated to the use of the operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 against '' '' '' allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
The first check “if (! Dict) return NULL;” most likely will not bring harm. But with the second check the situation is worse. If the creation of an object using “new base :: ListValue ()” fails to allocate memory, then an 'std :: bad_alloc' exception will be thrown. This is the end of the GetDictValueStats () function.
As a result, this code:
if (!values) { delete dict; return NULL; } never destroy an object whose address is stored in the 'dict' variable.
The correct solution here will be to refactor the code and use smart pointers.
Consider another code snippet:
bool Target::Init() { { .... ctx_ = new uint8_t[abi_->GetContextSize()]; if (NULL == ctx_) { Destroy(); return false; } .... } V668 has been defined as using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
In case of a memory allocation error, the Destroy () function will not be called.
Further writing about it is not interesting. I will simply provide a list of other potentially dangerous places I have 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
Noted N12 - tests that do not test well
Unit tests are a great technology for improving the quality of programs. However, the tests themselves often contain errors, as a result of which they do not perform their functions. Writing tests for tests is of course a bust. Can help static code analysis. I considered this idea in more detail in the article “ How static analysis complements TDD ”.
I will give some examples of errors I encountered in tests for Chromium:
std::string TestAudioConfig::TestValidConfigs() { .... static const uint32_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)'. test_audio_config.cc 56
Only one test will run in the loop. The error is that "sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts)" equals one. The correct expression is: “sizeof (kRequestFrameCounts) / sizeof (kRequestFrameCounts [0])”.
Another erroneous 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 the memcmp function is equal to the second argument. entry_unittest.cc 393
The “memcmp ()” function compares the buffer to itself. As a result, the test does not perform the required verification. Apparently, there should be:
EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000)); But a test that may unexpectedly break other tests:
static const int kNumPainters = 3; static const struct { const char* name; GPUPainter* painter; } painters[] = { { "CPU CSC + GPU Render", new CPUColorPainter() }, { "GPU CSC/Render", new GPUColorWithLuminancePainter() }, }; int main(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. Shader_bench.cc 152
Perhaps earlier the array 'painters' consisted of three elements. Now there are only two. And the value of the constant 'kNumPainters' remains equal to 3.
Some other places in the tests that I think deserve attention:
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get ()' pointer was used before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was used before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was used before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was used before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was used before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
Noted N13 - Function with variable number of arguments
In all programs, many defects are found in the code intended for handling errors and responding 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, the programs begin to behave much more bizarre than planned.
Example:
DWORD GetLastError(VOID); void TryOpenFile(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 argument of the fprintf function. 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 a billiberd at the end. Moreover, this code can lead to access violation .
For recording, it uses the fprintf () function. 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 it is unknown how the program will behave further.
Unnoticed
Once again, I looked through the list of messages superficially. I brought in this article only what caught my attention. Moreover, I noticed more than I wrote in the article. Describing everything, I will get an article too long. And she is already too big.
I dropped a lot of code fragments 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())) return true; if (extension) { bool was_installed_by_default = extension->was_installed_by_default(); ..... } } V595 'extension pointer extension nu pointer Check lines: 277, 280. managed_user_service.cc 277
In the beginning, the 'extension' pointer is dereferenced in the expression "extension-> id ()". Then this pointer is checked for equality to zero.
Often, this 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 could be mistaken and give a completely working code for the wrong one.
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, a negative value shift results in undefined behavior . However, many compilers are stable and exactly as the programmer expects. As a result, this code works long and successfully, although it is not required. I don’t want to fight this now, and I’ll miss such messages. For those who want to understand the question in more detail, I recommend the article " Without knowing the ford, do not climb into the water - part three ."
About false positives
I am often asked a question:
In the articles you are 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 about the PVS-Studio analyzer?
And I always do not know what to immediately answer this question. I have two opposite answers: the first - a lot, the second - a little. 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.
The PVS-Studio analyzer issued 3582 first-level warnings (GA rule set). This is a lot. And most of these messages are false. If you go to the "forehead" and start immediately to view the entire list, then it will get very tired very quickly. And the impression will be terrible. Some solid homogeneous false positives. Nothing interesting comes across. Bad tool.
, . , PVS-Studio , . , . .
. Chromium. , - 'DVLOG'. - . PVS-Studio , . , , . , DVLOG, . , 2300 «V501 There are identical sub-expressions.....».
, , , :
//-V:DVLOG:501
, 3582 , 2300 . 65% . .
. , . , . " ". , , .
. — . — . , .
, . , - . , , :
- « ?», " , ".
- , . twitter, - .
- : @Code_Analysis . ++.
Source: https://habr.com/ru/post/189892/
All Articles