# Heading for a Record: Chromium, the 5th Check

We checked Chromium more than once before, and those who follow our blog could reasonably ask, "Why another check? Weren't there enough of them?" Sure, Chromium's source code is particularly clean, which was shown by each of the previous checks, but new errors inevitably continue to appear\. Repeated checks prove that the more often you use static analysis, the better\. A good practice is to use the analyzer every day\. An even better practice is to analyze the new code right after you finish writing it \(automatic analysis of recently modified code\)\.

![0442_Chromium_5th_check_on_Linux/image1.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image1.png)

## A bit of history

We have checked Chromium four times already:

* [first check](https://pvs-studio.com/en/blog/posts/cpp/a0074/) \(23\.05\.2011\)
* [second check](https://pvs-studio.com/en/blog/posts/cpp/0113/) \(13\.10\.2011\)
* [third check](https://pvs-studio.com/en/blog/posts/cpp/0205/) \(12\.08\.2013\)
* [fourth check](https://pvs-studio.com/en/blog/posts/cpp/0225/) \(02\.12\.2013\)

All the previous checks were done with the Windows\-version of PVS\-Studio\. Now it supports Linux as well, and it is this version that we used this time\.

The Chromium solution has grown over the years: at the time of the third check, the number of projects reached the 1169 mark\. When I was writing this article, there were 4420 projects\. The source code has grown in size quite a bit, too, and is now 370 Mbytes \(260 Mbytes in 2013\)\. 

The previous four checks found Chromium's source code to be of extremely high quality, given its size\. Has it become worse during these two and a half years? No, it hasn't\. It's still up to the mark; but since it is so big and is still in development, there are still plenty of errors for us to catch there\.

## Analysis specifics

Let's talk about the details of analyzing Chromium using PVS\-Studio\. We are going to do it under Linux this time\. Once you have downloaded the source files using depot\_tools and prepared them for analysis \(see the details [here](https://www.chromium.org/developers/how-tos/get-the-code), before the 'Building' section\), build the solution:

```cpp
pvs-studio-analyzer trace -- ninja -C out/Default chrome
```

After that, run the following command \(in one line\):

```cpp
pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic 
-o /path/to/save/chromium.log -j<N>
```

where the "\-j" option initializes analysis in multithreaded mode\. The recommended number of threads is the number of physical CPU cores plus one \(for example, "\-j5" for a four\-core CPU\)\.

When the check is finished, PVS\-Studio will output an analysis log\. Use PlogConverter utility, which comes with the PVS\-Studio package, to convert that log into one of the three formats that can be conveniently viewed in other applications: xml, errorfile, tasklist\. We will be using the tasklist format in this article\. Here, we are interested only in the General Analysis warnings of every severity level \(High, Medium, Low\)\. This is what the conversion command should look like \(in one line\):

```cpp
plog-converter -t tasklist -o /path/to/save/chromium.tasks
-a GA:1,2,3 /path/to/saved/chromium.log
```

More information on PlogConverter's parameters can be found [here](https://pvs-studio.com/en/docs/manual/0036/#ID0EUXBK)\. To open the "chromium\.tasks" tasklist in QtCreator \(you need to install it in advance\), run the following command:

```cpp
qtcreator path/to/saved/chromium.tasks
```

We strongly recommend examining the warnings of the High and Medium levels first: they are very likely to deal with real defects and errors\. Low\-level warnings might point to potential bugs, but they are also more likely to produce false positives, so we don't usually discuss them in our articles\.

This is how the log is displayed in QtCreator:

![0442_Chromium_5th_check_on_Linux/image3.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image3.png)

Figure 1 \- Viewing analysis results in QtCreator \(click to enlarge\)

## Analysis statistics

PVS\-Studio issued a total of 2312 warnings\. The chart below shows the distribution of the warnings across the severity levels:

![0442_Chromium_5th_check_on_Linux/image4.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image4.png)

Figure 2 \- Warning distribution across severity levels

Let me briefly comment on this chart: the analyzer issued 171 High\-level, 290 Medium\-level, and 1851 Low\-level warnings\.

Despite the seemingly large amount of warnings, it's actually small for such a huge project\. The total number of SLOC, without the linked libraries, is 6468751\. If we consider the warnings of High and Medium levels only, I'd say there are just 220 genuine errors among them\. Well, that's the statistics, and the real error density is 0,034 per 1000 LOC\. This figure, however, takes into account only those errors that PVS\-Studio found, or, to be more exact, that caught my eye when looking through the log\.

Other projects usually have higher error density, so the Chromium developers did well\! Even so, don't get lax: there are still errors, and they are far from harmless\.

The most interesting ones are discussed below\.

## New errors

### Copy\-Paste

![0442_Chromium_5th_check_on_Linux/image5.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image5.png)

**PVS\-Studio warning:** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'request\_body\_send\_buf\_ \=\= nullptr' to the left and to the right of the '&&' operator\. http\_stream\_parser\.cc 1222

```cpp
bool HttpStreamParser::SendRequestBuffersEmpty() 
{
  return request_headers_ == nullptr 
      && request_body_send_buf_ == nullptr 
      && request_body_send_buf_ == nullptr;  // <=
}
```

This is classic\. The _request\_body\_send\_buf\__ pointer is compared with _nullptr_ twice\. It must be a typo, so there is some other class member that should be compared with _nullptr_\.

**PVS\-Studio warning:** [V766](https://pvs-studio.com/en/docs/warnings/v766/) An item with the same key '"colorSectionBorder"' has already been added\. ntp\_resource\_cache\.cc 581

```cpp
void NTPResourceCache::CreateNewTabCSS() 
{
  ....
  substitutions["colorSectionBorder"] =             // <=
      SkColorToRGBAString(color_section_border); 
  ....
  substitutions["colorSectionBorder"] =             // <=
      SkColorToRGBComponents(color_section_border); 
  ....
}
```

The analyzer detected a strange double initialization of the object associated with the _"colorSectionBorder"_ key\. The _substitutions_ variable is an associative array here\. When being initialized, the _color\_section\_border_ variable of type _SkColor_ \(defined as _uint32\_t_\) is cast to a string representation of RGBA \(as suggested by the _SkColorToRGBAString _method's name\) and mapped to the _"colorSectionBorder" _key\. After that, _color\_section\_border_ is cast to another string format \(method _SkColorToRGBComponents_\) and mapped to the same key\. It means that the previous value associated with the key _"colorSectionBorder"_ will be lost\. If this is what the programmer intended, then one of the assignments should be removed\. Otherwise, the color components should be mapped to different keys\. 

**Note\.** By the way, this is the first error found by the [V766](https://pvs-studio.com/en/docs/warnings/v766/) diagnostic in a real\-life project\. This is a specific type of bugs, but Chromium is so big that even exotic errors like that can be found there\.

### Incorrect pointer handling

![0442_Chromium_5th_check_on_Linux/image6.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image6.png)

Now a small warm\-up for your brains\. Look at the code below and try to find the bug by yourself\.

```cpp
// Returns the item associated with the component |id| or nullptr
// in case of errors.
CrxUpdateItem* FindUpdateItemById(const std::string& id) const;

void ActionWait::Run(UpdateContext* update_context,
                     Callback callback)
{
  ....
  while (!update_context->queue.empty()) 
  {
      auto* item = 
        FindUpdateItemById(update_context->queue.front());
      if (!item)
      {
        item->error_category = 
          static_cast<int>(ErrorCategory::kServiceError); 
        item->error_code =
          static_cast<int>(ServiceError::ERROR_WAIT);
        ChangeItemState(item, CrxUpdateItem::State::kNoUpdate);
      } else {
        NOTREACHED();
      }
      update_context->queue.pop();
  }
  ....
}
```

**PVS\-Studio warning:** [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'item' might take place\. action\_wait\.cc 41

The authors of this code made a conscious decision to shoot themselves in the foot\. The code iterates over the _queue_ queue consisting of identifiers presented as strings\. An identifier is taken out of the queue, and then the _FindUpdateItemById_ method is called to return a pointer to the object of type _CrxUpdateItem_ associated with that identifier\. If _FindUpdateItemById_ fails, it will return _nullptr_, which will then be dereferenced in the _if_ statement's _then_ branch\.

This is the fixed code:

```cpp
....
while (!update_context->queue.empty()) 
{
  auto* item = 
    FindUpdateItemById(update_context->queue.front());
  if (item != nullptr)
  { 
    ....
  }
  ....
}
....
```

**PVS\-Studio warning:** [V620](https://pvs-studio.com/en/docs/warnings/v620/) It's unusual that the expression of sizeof\(T\)\*N kind is being summed with the pointer to T type\. string\_conversion\.cc 62

```cpp
int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) 
{
  const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in);
  const UTF8 *source_end_ptr = source_ptr + sizeof(char);
  uint16_t *target_ptr = out;
  uint16_t *target_end_ptr = target_ptr + 2 * sizeof(uint16_t); // <=
  out[0] = out[1] = 0;
  ....
}
```

The analyzer detected a code fragment with strange address arithmetic\. As suggested by its name, the function converts characters from the UTF\-8 format to UTF\-16\. The current standard, Unicode 6\.x, implies widening a UTF\-8 character to four bytes, which is the reason why a UTF\-8 character is decoded as two UTF\-16 characters \(UTF\-16 characters are hardcoded with two bytes\)\. Decoding is done using four pointers: two pointing to the beginning, and two others pointing to the end of the arrays _in _and _out_\. The pointers to the end of the arrays act like STL iterators: they point to the location after the last array element\. While the _source\_end\_ptr_ pointer is evaluated correctly, things get complicated for _target\_end\_ptr_\. It was meant to point to the location after the second element of the _out_ array \(i\.e\. move by four bytes in relation to the _out_ pointer\), but what it will be actually pointing to is the address after the fourth element \(i\.e\. _out_ will be shifted by eight bytes\)\.

This is the planned logic:

![0442_Chromium_5th_check_on_Linux/image7.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image7.png)

And this is what actually happens:

![0442_Chromium_5th_check_on_Linux/image8.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image8.png)

The fixed code:

```cpp
int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) 
{
  const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in);
  const UTF8 *source_end_ptr = source_ptr + 1;
  uint16_t *target_ptr = out;
  uint16_t *target_end_ptr = target_ptr + 2;
  out[0] = out[1] = 0;
  ....
}
```

The analyzer also reported one more potential defect of this type:

* _V620 It's unusual that the expression of sizeof\(T\)\*N kind is being summed with the pointer to T type\. string\_conversion\.cc 106_

### Miscellaneous

![0442_Chromium_5th_check_on_Linux/image9.png](https://import.viva64.com/docx/blog/0442_Chromium_5th_check_on_Linux/image9.png)

Another warm\-up\. Can you find the bug in the code below?

```cpp
CheckReturnValue& operator=(const CheckReturnValue& other)
{
  if (this != &other)
  {
    DCHECK(checked_);
    value_ = other.value_;
    checked_ = other.checked_;
    other.checked_ = true;
  }
}
```

**PVS\-Studio warning:** [V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. memory\_allocator\.h 39

We are dealing with [undefined behavior](https://pvs-studio.com/en/blog/terms/0066/) here\. The C\+\+ standard says that any non\-void method must return a value\. What about our example? In the assignment statement, the current object is tested for being equal to itself \(the objects are compared by using their pointers\) and the fields are copied \(if the pointers are different\)\. However, the method does not return the reference to itself \(_return \*this_\)\.

Two more non\-void methods that don't return:

* _V591 Non\-void function should return a value\.  sandbox\_bpf\.cc 115_
* _V591 Non\-void function should return a value\. events\_x\.cc 73_



**PVS\-Studio warning:** [V583](https://pvs-studio.com/en/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: 1\. configurator\_impl\.cc 133

```cpp
int ConfiguratorImpl::StepDelay() const 
{
  return fast_update_ ? 1 : 1;
}
```

This code always returns 1 as the delay time\. Perhaps it's just incomplete code to be developed later, but the current implementation of the ternary operator doesn't do any good\.

**PVS\-Studio warning:** [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the 'rv \=\= OK \|\| rv \!\= ERR\_ADDRESS\_IN\_USE' expression\. The expression is excessive or contains a misprint\. udp\_socket\_posix\.cc 735

```cpp
int UDPSocketPosix::RandomBind(const IPAddress& address) 
{
  DCHECK(bind_type_ == DatagramSocket::RANDOM_BIND 
      && !rand_int_cb_.is_null());

  for (int i = 0; i < kBindRetries; ++i) {
    int rv = DoBind(IPEndPoint(address,
                               rand_int_cb_
                               .Run(kPortStart, kPortEnd)));
    if (rv == OK || rv != ERR_ADDRESS_IN_USE) // <=
      return rv;
  }
  return DoBind(IPEndPoint(address, 0));
}
```

The analyzer warns us about a potential redundant comparison\. The code above maps an IP to a random port\. Successful mapping terminates the loop \(which counts the number of mapping attempts\)\. Removing one of the comparisons won't affect the code's logic \(in the current version, the loop stops if the mapping has succeeded or if no error about the port being mapped to another IP was issued\)\.

**PVS\-Studio warning:** [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\.

```cpp
bool ResourcePrefetcher::ShouldContinueReadingRequest(
  net::URLRequest* request,
  int bytes_read
) 
{
  if (bytes_read == 0) {  // When bytes_read == 0, no more data.
    if (request->was_cached())
      FinishRequest(request); // <=
    else
      FinishRequest(request); // <=
    return false;
  }

  return true;
}
```

The analyzer detected identical statements in the _then_ and _else_ branches of the _if_ statement\. What are the possible implications? The current logic suggests that an uncached URL\-request \(_net::URLRequest_ _\*request_\) will be finished in the same way as a cached one\. If this is exactly what the programmer meant, then the _else_ statement can be safely removed:

```cpp
....
if (bytes_read == 0) {  // When bytes_read == 0, no more data.
  FinishRequest(request); // <=
  return false;
}
....
```

Otherwise, a wrong method will be called, which could result in spending numerous sleepless nights and drinking tons of coffee trying to debug the code\.

**PVS\-Studio warning**: [V609](https://pvs-studio.com/en/docs/warnings/v609/) Divide by zero\. Denominator range \[0\.\.4096\]\. addr\.h 159

```cpp
static int BlockSizeForFileType(FileType file_type)
{
  switch (file_type)
  {
    ....
    default:
      return 0; // <=
  }
}
static int RequiredBlocks(int size, FileType file_type)
{
  int block_size = BlockSizeForFileType(file_type);
  return (size + block_size - 1) / block_size; // <=
}
```

What about this code? It may produce an elusive bug\. The _RequiredBlocks_ method performs division by the value of the _block\_size _variable \(evaluated by the _BlockSizeForFileType _method\)\. The _switch _statement in the _BlockSizeForFileType_ method compares the value of the _FileType _enumeration passed to the method with some values and returns one of them, but there is also the default value, 0\. Suppose the programmer decided to add a new value to the _FileType_ enumeration but forgot to add the corresponding _case _label to the _switch _statement's body\. This mistake would lead to undefined behavior: the C\+\+ standard does not imply raising a software exception when division\-by\-zero occurs\. Instead, a hardware exception will be raised, which can't be caught by using the standard _try_/_catch_ block \(instead, signal handlers are used; more information can be found [here](http://www.cplusplus.com/reference/csignal/) and [here](http://www.yolinux.com/TUTORIALS/C++Signals.html)\)\.

**PVS\-Studio warning**: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The '\* list' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 136, 138\. util\.cc 138 

```cpp
bool GetListName(ListType list_id, std::string* list) 
{
  switch (list_id) {
    ....
    case IPBLACKLIST:
      *list = kIPBlacklist;
      break;
    case UNWANTEDURL:
      *list = kUnwantedUrlList;
      break;
    case MODULEWHITELIST:
      *list = kModuleWhitelist; // <=
    case RESOURCEBLACKLIST:
      *list = kResourceBlacklist;
      break;
    default:
      return false;
  }
  ....
}
```

This is a common mistake when implementing a _switch _statement\. The programmer expects that if the _list\_id _variable is found to be equal to the value _MODULEWHITELIST_ from the _ListType_ enumeration, the string pointed to by the _list _pointer will be initialized to the value _kModuleWhitelist _and execution will leave the _switch _statement\. However, because of the missing _break _statement, the execution will move on to the next _case _label, _RESOURCEBLACKLIST_, which will result in associating \*_list_ with the _kResourceBlacklist_ string instead\.

## Conclusions

Chromium is as cool as it used to be, but PVS\-Studio can still catch bugs in its code, again and again\. Static analysis can help you detect bugs as early as at the coding stage, before testing\.

What static analysis tools to use? Well, there are actually [lots of them](https://en.wikipedia.org/wiki/list_of_tools_for_static_code_analysis)\. As for me, I naturally suggest trying PVS\-Studio\. It can integrate smoothly with the Visual Studio IDE or, alternatively, any build system\. There is also a Linux version available since recently\. More information about the Windows and Linux versions can be found [here](https://pvs-studio.com/en/pvs-studio/) and [here](https://pvs-studio.com/en/blog/posts/0415/)\.