# Chromium: Miscellaneous Defects

This article is the last in our series of articles where we share tips on how to write high\-quality code, using the bugs found in the Chromium project as examples\. Now, with 6 articles behind, there still remain a lot of errors we haven't discussed yet\. They are too diverse to be put into specific categories, so in this 7th article, I'll just pick and talk about the most interesting defects\.

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

As I mentioned in the [introductory article](https://pvs-studio.com/en/blog/posts/cpp/0552/), I've read the PVS\-Studio report and have found about 250 bugs in the Chromium project and the third\-party libraries it uses\. Since I only glanced through the report, there are actually much more bugs to be found there\. 

After the introductory article, I wrote 6 more on various bug patterns\. Those articles were rich in examples, but there are still about 70 bugs left, which I can't put into particular groups\. Maybe I'm just tired\. Well, there's another reason: a [report](http://cppfiles.com/xnu.txt) for [XNU](https://en.wikipedia.org/wiki/XNU) is waiting for me, which I can't wait to get to\.

That's why I decided to finish the series with this final post, where I show you the most interesting of the remaining bugs\. As a reminder, here's the complete list of the bugs: [chromium\.txt](http://cppfiles.com/chromium.txt)\.

## Undefined and unspecified behavior

Chromium project\.

```cpp
void DeviceMediaAsyncFileUtil::CreateOrOpen(
    std::unique_ptr<FileSystemOperationContext> context, ....) {
  ....
  CreateSnapshotFile(
      std::move(context), url,
      base::Bind(
        &NativeMediaFileUtil::CreatedSnapshotFileForCreateOrOpen,
        base::RetainedRef(context->task_runner()),
        file_flags, callback));
}
```

PVS\-Studio diagnostic message: [V522](https://pvs-studio.com/en/docs/warnings/v522/) CWE\-476 Dereferencing of the null pointer 'context' might take place\. device\_media\_async\_file\_util\.cc 322

Depending on the user's luck, or rather the compiler used, this code may both work well and end up with null\-pointer dereference\.

Whether the null pointer will be dereferenced depends on the order of evaluation of the arguments when calling to the _CreateSnapshotFile _function\. In C\+\+, the evaluation order of function arguments isn't specified \(unspecified behavior\)\. If _std::move\(context\)_ argument happens to be evaluated first, then null\-pointer dereference will occur in _context\-\>task\_runner\(\)_\.

**Tip\.** Don't try to squeeze as many operations as possible into one line, as this often leads to errors\. With a more straightforward style, you have more chances to write code without mistakes\.

Chromium project\.

```cpp
std::unordered_map<std::string, int> thread_colors_;

std::string TraceLog::EventToConsoleMessage(....) {
  ....
  thread_colors_[thread_name] = (thread_colors_.size() % 6) + 1;
  ....
}
```

PVS\-Studio diagnostic message: [V708](https://pvs-studio.com/en/docs/warnings/v708/) CWE\-758 Dangerous construction is used: 'm\[x\] \= m\.size\(\)', where 'm' is of 'unordered\_map' class\. This may lead to undefined behavior\. trace\_log\.cc 1343

This code is so complicated that you can't even figure out if its behavior is undefined or not\. The reason is that the C\+\+ standard is changing and some constructs, once defined as causing undefined behavior, become correct\. Here are a few simple examples to explain this:

```cpp
i = ++i + 2;       // undefined behavior until C++11
i = i++ + 2;       // undefined behavior until C++17
f(i = -2, i = -2); // undefined behavior until C++17
f(++i, ++i);       // undefined behavior until C++17,
                   // unspecified after C++17
i = ++i + i++;     // undefined behavior 
cout << i << i++;  // undefined behavior until C++17
a[i] = i++;        // undefined behavior until C++17
n = ++i + i;       // undefined behavior
```

Getting back to the Chromium code, the _thread\_colors\_\[thread\_name\]_ expression may create a new element in the container or fail to do so, returning a reference to an already existing element instead\. The main point here is that _thread\_colors\_\[thread\_name\]_ could change the number of elements in the associative container\.

The result of the _\(thread\_colors\_\.size\(\) % 6\) \+ 1_ expression depends on the number of elements in the associative container _thread\_colors\__\.

You'll get different values depending on which operand of the assignment operator, \=, is evaluated first\.

What does the evaluation order depend on? It depends on the language version used\. But whatever the version, it's a bad idea to write code like that because it's very hard to read\.

**Tip\.** It's the same: don't try to squeeze as many operations as possible into one line\.

ICU library\.

```cpp
U_DRAFT uint32_t U_EXPORT2 ubiditransform_transform(....)
{
  ....
  const UBiDiAction *action = NULL;
  ....
  if (action + 1) {
    updateSrc(....);
  }
  ....
}
```

PVS\-Studio diagnostic message: [V694](https://pvs-studio.com/en/docs/warnings/v694/) CWE\-571 The condition \(action \+ 1\) is only false if there is pointer overflow which is undefined behavior anyway\. ubiditransform\.cpp 502

The condition is always true\. In theory, an overflow could make it false, but this would result in undefined behavior\.

WebRTC library\.

```cpp
std::vector<SdpVideoFormat>
StereoDecoderFactory::GetSupportedFormats() const
{
  std::vector<SdpVideoFormat> formats = ....;
  for (const auto& format : formats) {           // <=
    if (cricket::CodecNamesEq(....)) {
      ....
      formats.push_back(stereo_format);          // <=
    }
  }
  return formats;
}
```

PVS\-Studio diagnostic message: [V789](https://pvs-studio.com/en/docs/warnings/v789/) CWE\-672 Iterators for the 'formats' container, used in the range\-based for loop, become invalid upon the call of the 'push\_back' function\. stereocodecfactory\.cc 89

The analyzer detected invalidation of the iterator in a range\-based for loop\. The code above is equivalent to this:

```cpp
for (auto format = begin(formats), __end = end(formats); 
     format != __end; ++format) {
  if (cricket::CodecNamesEq(....)) {
    ....
    formats.push_back(stereo_format);
  }
}
```

Now you can see that, when calling to the _push\_back_ function, the iterators _format_ and _\_\_end_ may get invalidated if the storage is reallocated inside the vector\.

**Tip\.** Remember that you must not change the number of container elements in range\-based, as well as iterator\-based, _for_ loops\.

## Logic Errors

Chromium project\.

```cpp
STDMETHOD(GetInputScopes)(InputScope** input_scopes,
                          UINT* count) override
{
  if (!count || !input_scopes)
    return E_INVALIDARG;
  *input_scopes = static_cast<InputScope*>(CoTaskMemAlloc(
      sizeof(InputScope) * input_scopes_.size()));
  if (!input_scopes) {
    *count = 0;
    return E_OUTOFMEMORY;
  }
  ....
}
```

PVS\-Studio diagnostic message: [V649](https://pvs-studio.com/en/docs/warnings/v649/) CWE\-561 There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains function return\. This means that the second 'if' statement is senseless\. Check lines: 67, 71\. tsf\_input\_scope\.cc 71

The second check of the _input\_scopes_ pointer doesn't make sense because if it happens to be null, the check in the beginning of the function will notice this and the function will return _E\_INVALIDARG_\.

The error here has to do with the missing \* operator in "_if \(\!input\_scopes\)_"\. Because of that, the pointer returned by the _CoTaskMemAlloc_ function is not checked\. This is what the code should look like:

```cpp
*input_scopes = static_cast<InputScope*>(CoTaskMemAlloc(
    sizeof(InputScope) * input_scopes_.size()));
if (!*input_scopes) {
 *count = 0;
  return E_OUTOFMEMORY;
}
```

Skia library\.

```cpp
SkOpSpan* SkOpContour::undoneSpan() {
  SkOpSegment* testSegment = &fHead;
  bool allDone = true;
  do {
    if (testSegment->done()) {
      continue;
    }
    allDone = false;
    return testSegment->undoneSpan();
  } while ((testSegment = testSegment->next()));
  if (allDone) {
    fDone = true;
  }
  return nullptr;
}
```

PVS\-Studio finds this code suspicious for two reasons at once:

* [V547](https://pvs-studio.com/en/docs/warnings/v547/) CWE\-571 Expression 'allDone' is always true\. skopcontour\.cpp 43
* [V1001](https://pvs-studio.com/en/docs/warnings/v1001/) CWE\-563 The 'allDone' variable is assigned but is not used until the end of the function\. skopcontour\.cpp 40

It's very suspicious, but I can't figure out how exactly it should work and where the error is\. If you wish, you can try to find out for yourself what the _undoneSpan_ function should actually look like\.

WebKit library\.

```cpp
WebString StringConstraint::ToString() const {
  ....
  bool first = true;
  for (const auto& iter : exact_) {
    if (!first)
      builder.Append(", ");
    builder.Append('"');
    builder.Append(iter);
    builder.Append('"');
  }
  ....
}
```

PVS\-Studio diagnostic message: V547 CWE\-570 Expression '\!first' is always false\. webmediaconstraints\.cpp 302

Since the _first_ variable is always _true_, no commas will be added between the elements\. Correct version:

```cpp
bool first = true;
for (const auto& iter : exact_) {
  if (first)
    first = false;
  else
    builder.Append(", ");
  builder.Append('"');
  builder.Append(iter);
  builder.Append('"');
}
```

ICU library\.

```cpp
uint32_t CollationDataBuilder::setPrimaryRangeAndReturnNext(....)
{
  ....
  } else {
    // Short range: Set individual CE32s.
    for(;;) {
      utrie2_set32(....);
      ++start;
      primary = Collation::incThreeBytePrimaryByOffset(....);
      if(start > end) { return primary; }
    }
    modified = TRUE;       // <=
  }
}
```

PVS\-Studio diagnostic message: [V779](https://pvs-studio.com/en/docs/warnings/v779/) CWE\-561 Unreachable code detected\. It is possible that an error is present\. collationdatabuilder\.cpp 392

The loop can be interrupted only by calling the _return_ statement\. It means the assignment operation after the loop will never be executed\.

Ced library\.

```cpp
void HzBoostWhack(DetectEncodingState* destatep,
                  uint8 byte1, uint8 byte2)
{
  if ((byte2 == '{') || (byte2 == '}')) {
    // Found ~{ or ~}
    Boost(destatep, F_HZ_GB_2312, kBoostOnePair);         
  } else if ((byte2 == '~') || (byte2 == '\n')) {
    // neutral
    destatep->enc_prob[F_HZ_GB_2312] += 0;                
  } else {
    // Illegal pair
    Whack(destatep, F_HZ_GB_2312, kBadPairWhack);         
  }
}
```

PVS\-Studio diagnostic message: [V751](https://pvs-studio.com/en/docs/warnings/v751/) Parameter 'byte1' is not used inside function body\. compact\_enc\_det\.cc 2559

It's very suspicious that the _byte1_ argument is not used in the function\. I don't know if this is an error, but even if not, one shouldn't write code like that, as it may confuse both maintainers and compilers\.

## Wrong Assumptions

Sometimes programmers have wrong assumptions about how certain functions or language constructs work\. Let's take a look at some of the errors resulting from such assumptions\.

Chromium project\.

```cpp
void OnConvertedClientDisconnected() {
  // We have no direct way of tracking which
  // PdfToEmfConverterClientPtr got disconnected as it is a
  // movable type, short of using a wrapper.
  // Just traverse the list of clients and remove the ones
  // that are not bound.

  std::remove_if(
    g_converter_clients.Get().begin(),
    g_converter_clients.Get().end(),
    [](const mojom::PdfToEmfConverterClientPtr& client) {
      return !client.is_bound();
    });
}
```

PVS\-Studio diagnostic message: V530 CWE\-252 The return value of function 'remove\_if' is required to be utilized\. pdf\_to\_emf\_converter\.cc 44

The _remove\_if_ function doesn't remove anything but only moves the elements inside the container\. The code should probably look like this:

```cpp
auto trash = std::remove_if(........);
g_converter_clients.Get().erase(trash,
                                g_converter_clients.Get().end());
```

V8 engine\.

```cpp
void StringStream::Add(....) {
  ....
    case 'f': case 'g': case 'G': case 'e': case 'E': {
      double value = current.data_.u_double_;
      int inf = std::isinf(value);
      if (inf == -1) {
        Add("-inf");
      } else if (inf == 1) {
        Add("inf");
      } else if (std::isnan(value)) {
        Add("nan");
      } else {
        EmbeddedVector<char, 28> formatted;
        SNPrintF(formatted, temp.start(), value);
        Add(formatted.start());
      }
      break;
    }  ....
}
```

PVS\-Studio diagnostic message: V547 CWE\-570 Expression 'inf \=\= \- 1' is always false\. string\-stream\.cc 149

Here's the description of the _std::isinf_ function: [isinf](https://en.cppreference.com/w/cpp/numeric/math/isinf)\.

As you can see, _std::isinf_ returns a value of type _bool_, so the way it is used here is obviously incorrect\.

Skia library\.

```cpp
GrGLProgram* GrGLProgramBuilder::finalize() {
  ....
  std::unique_ptr<char> binary(new char[length]);
  ....
}
```

PVS\-Studio diagnostic message: [V554](https://pvs-studio.com/en/docs/warnings/v554/) CWE\-762 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. grglprogrambuilder\.cpp 272

The storage is allocated by the _operator_ _new\[\]_ but freed by the _operator_ _delete_\. The _unique\_ptr_ class needs a hint as to how to manage memory\. Correct version:

```cpp
std::unique_ptr<char[]> binary(new char[length]);
```

Another slip\-up found in the same library:

```cpp
GrGLProgram* GrGLProgramBuilder::finalize() {
  ....
  std::unique_ptr<uint8_t> data((uint8_t*) malloc(dataLength));
  ....
}
```

PVS\-Studio diagnostic message: V554 CWE\-762 Incorrect use of unique\_ptr\. The memory allocated with 'malloc' will be cleaned using 'delete'\. grglprogrambuilder\.cpp 275

It seems one of the developers discovered the _std::unique\_ptr_ class but didn't have enough time to learn how to use it properly :\)\. The storage is allocated by the _malloc_ function but freed by the _operator delete_\.

Correct code:

```cpp
std::unique_ptr<uint8_t, void (*)(void*)>
  data((uint8_t*) malloc(dataLength), std::free);
```

WebKit library\.

```cpp
struct ScrollAnchorData {
  WebString selector_;
  WebFloatPoint offset_;
  uint64_t simhash_;

  ScrollAnchorData(const WebString& selector,
                   const WebFloatPoint& offset,
                   uint64_t simhash)
    : selector_(selector), offset_(offset), simhash_(simhash) {}

  ScrollAnchorData() { 
    ScrollAnchorData(WebString(), WebFloatPoint(0, 0), 0); }
};
```

PVS\-Studio diagnostic message: [V603](https://pvs-studio.com/en/docs/warnings/v603/) CWE\-665 The object was created but it is not being used\. If you wish to call constructor, 'this\-\>ScrollAnchorData::ScrollAnchorData\(\.\.\.\.\)' should be used\. webscrollanchordata\.h 49

It's not calling one constructor from another\. It's just that one unnamed object is created and immediately destroyed\.

## Incorrect Pointer Checks

Incorrect checks for null pointers are very common in applications\. There are two types of this bug:

**The first type** is when the pointer is first dereferenced and only then checked:

```cpp
p[n] = 1;
if (!p) return false;
```

**The second type** is when the programmer checks the pointer before the first use but forgets to do so before a second use:

```cpp
if (p) p[0] = x;
p[1] = y;
```

The former are detected by the [V595](https://pvs-studio.com/en/docs/warnings/v595/) diagnostic, the latter by the [V1004](https://pvs-studio.com/en/docs/warnings/v1004/) diagnostic\.

Such defects aren't always that bad\. Firstly, some pointers can never become null\. In that case, there's no error at all \- just an extra check confusing both programmers and code analyzers\. Secondly, some pointers may become null only in very rare cases, so the error does not affect the program under normal conditions\.

That said, the developers should still pay attention to V595 and V1004 warnings and fix them where necessary\. PVS\-Studio issued a lot of such messages on the code of Chromium and the libraries\. Unfortunately, as I mentioned in the introductory article, they are mostly false positives because of the use of the _DCHECK_ macro\. So, I got bored of working through them pretty soon\. V595 and V1004 warnings need to be examined more carefully after some tweaking of the analyzer's settings\.

In any case, I assure you that there are a lot of bugs there that have to do with incorrect pointer checks\. Some of them are cited in [chromium\.txt](http://cppfiles.com/chromium.txt)\. Finding the rest would take a few heroes who could configure the analyzer and study the new report\.

I won't cite all the bugs I found, as they all look pretty much the same\. Instead, I'll show you just two examples for each diagnostic so you can grasp the idea of what type of errors I'm talking about\.

V595, example one, Chromium project\.

```cpp
template <typename T>
void PaintOpReader::ReadFlattenable(sk_sp<T>* val) {
  // ....
  // Argument val is not used nor checked here.
  // ....  
  val->reset(static_cast<T*>(SkValidatingDeserializeFlattenable(
      const_cast<const char*>(memory_), bytes,
      T::GetFlattenableType())));
  if (!val)
    SetInvalid();
  ....
}
```

PVS\-Studio diagnostic message: V595 CWE\-476 The 'val' pointer was utilized before it was verified against nullptr\. Check lines: 124, 126\. paint\_op\_reader\.cc 124

The _val_ pointer is dereferenced without being checked for _nullptr_\.

V595, example two, Chromium project\.

```cpp
void HttpAuthHandlerRegistryFactory::RegisterSchemeFactory(
    const std::string& scheme, 
    HttpAuthHandlerFactory* factory)
{
  factory->set_http_auth_preferences(http_auth_preferences());
  std::string lower_scheme = base::ToLowerASCII(scheme);
  if (factory)
    factory_map_[lower_scheme] = base::WrapUnique(factory);
  else
    factory_map_.erase(lower_scheme);
}
```

PVS\-Studio diagnostic message: V595 CWE\-476 The 'factory' pointer was utilized before it was verified against nullptr\. Check lines: 122, 124\. http\_auth\_handler\_factory\.cc 122

The _factory_ pointer is dereferenced without being checked for _nullptr_\.

V1004, example one, PDFium library\.

```cpp
void CFX_PSRenderer::SetClip_PathStroke(....,
  const CFX_Matrix* pObject2Device, ....)
{
  ....
  if (pObject2Device) {
    ....
  }
  ....
  m_ClipBox.Intersect(
    pObject2Device->TransformRect(rect).GetOuterRect());
  ....
}
```

PVS\-Studio diagnostic message: V1004 CWE\-476 The 'pObject2Device' pointer was used unsafely after it was verified against nullptr\. Check lines: 237, 248\. cfx\_psrenderer\.cpp 248

The _pObject2Device_ pointer could be null, which is indicated by the _nullptr_ check\. However, this pointer is dereferenced a few lines later without such a check\.

V1004, example two, SwiftShader library\.

```cpp
VertexProgram::VertexProgram(...., const VertexShader *shader)
  : VertexRoutine(state, shader),
    shader(shader),
    r(shader->dynamicallyIndexedTemporaries)
{
  ....
  if(shader && shader->containsBreakInstruction())
  {
    enableBreak = ....;
  }

  if(shader && shader->containsContinueInstruction())
  {
    enableContinue = ....;
  }

  if(shader->isInstanceIdDeclared())
  {
    instanceID = ....;
  }  
}
```

PVS\-Studio diagnostic message: V1004 CWE\-476 The 'shader' pointer was used unsafely after it was verified against nullptr\. Check lines: 43, 53\. vertexprogram\.cpp 53

The _shader_ pointer could be null, which is indicated by the _nullptr_ checks\. However, the pointer is dereferenced a few lines later without such a check\.

## Greetings to Google Developers\!

We \- the PVS\-Studio team \- are greeting Google developers and would like to say that we are open to cooperation\. There are at least two possibilities:

1. Google could buy a PVS\-Studio license for use by all the developers of Chrome, Chromium, and third\-party libraries employed by those projects\. Well, we could even make it available to all Google employees\.
1. We could draw up a contract under which our team would configure the PVS\-Studio analyzer to meet Google's needs, fix all the bugs found by our analyzer, and carry out regular audit of the code and fixing of new bugs\.

Welcome to try PVS\-Studio\. Feel free to [email us](https://pvs-studio.com/en/about-feedback/)\. We will help you in checking your projects and give you a temporary license so that you can test it in full\.

## Conclusion

Thanks to everyone who has made it through the whole series\. I hope you enjoyed it\.

As you can see, even such a high\-quality project as Chromium has many errors that PVS\-Studio can detect\. Why don't you start using it with your projects too?