# Chromium: другие ошибки

Мы завершаем цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium\. После написания 6 статей, у нас всё ещё остаётся много нерассмотренных ошибок\. Эти ошибки разнородны и про них сложно составить материал на определённую тему\. Поэтому в этой седьмой статье будут просто выбраны и рассмотрены наиболее интересные дефекты\.

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

Как я писал в [вводной статье](https://pvs-studio.ru/ru/blog/posts/cpp/0552/), просматривая отчёт, выданный анализатором PVS\-Studio, я заметил около 250 ошибок в проекте Chromium и использованных в нём библиотеках\. Отчёт я смотрел достаточно бегло, поэтому, на самом деле, ошибок можно найти намного больше\.

После вводной статьи я написал еще 6, где рассматривал различные паттерны ошибок\. Писал я эти статьи, писал, и всё равно, у меня остаётся около 70 примеров ошибок\. Сделать на основании оставшихся багов статьи на разные темы я затрудняюсь\. Возможно, я устал\. Впрочем, есть и другая причина \- меня ждёт [отчёт](http://cppfiles.com/xnu.txt) о проверке [XNU](https://ru.wikipedia.org/wiki/XNU) и мне не терпится им позаниматься\.

Поэтому я решил завершить цикл статей про Chromium и написать эту последнюю статью, где рассмотрю наиболее интересные из оставшихся ошибок\. Напоминаю, что полный список ошибок приводится здесь: [chromium\.txt](http://cppfiles.com/chromium.txt)\.

## Undefined и unspecified behavior

Проект Chromium\.

```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: [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) CWE\-476 Dereferencing of the null pointer 'context' might take place\. device\_media\_async\_file\_util\.cc 322

В зависимости от везения, а точнее от используемого компилятора, этот код может как работать правильно, так и приводить к разыменовыванию нулевого указателя\.

Будет разыменование нулевого указателя или нет, зависит от того, в каком порядке будут вычисляться аргументы при вызове функции _CreateSnapshotFile_\. В C\+\+ порядок вычисления аргументов функции не определен \(unspecified behavior\)\. Если первым окажется вычислен аргумент _std::move\(context\)_, то здесь _context\-\>task\_runner\(\)_ произойдёт разыменование нулевого указателя\.

**Рекомендация\.** Не надо стараться напихать в одну строку как можно больше действий – это часто подводит\. Пишите простой код, и он с большей вероятностью получится правильным\.

Проект Chromium\.

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

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

Предупреждение PVS\-Studio: [V708](https://pvs-studio.ru/ru/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

Этот код настолько сложен, что даже непонятно, есть ли сейчас в нём неопределённое поведение или нет\. Дело в том, что стандарт C\+\+ меняется и некоторые конструкции, которые ранее приводили к неопределённому поведению, становятся корректными\. Поясню это на простых примерах:

```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
```

Вернёмся к коду из проекта Chromium\. Выражение _thread\_colors\_\[thread\_name\]_ может создать новый элемент в контейнере, а может и не создать, и вернуть ссылку на уже существующий элемент\. Главное, что _thread\_colors\_\[thread\_name\]_ может изменять количество элементов в ассоциативном контейнере\.

Выражение _\(thread\_colors\_\.size\(\) % 6\) \+ 1_ зависит от количества элементов в ассоциативном контейнере _thread\_colors\__\.

Будут получаться разные значения в зависимости от того, вычисляется первым выражение слева от оператора присваивания \= или выражение справа\.

От чего зависит порядок вычисления? От версии используемого языка\. В любом случае так писать не надо\. Этот код очень сложен для понимания\.

**Рекомендация\.** Совет всё тот же: не надо стараться напихать в одну строку как можно больше действий\.

Библиотека ICU\.

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

Предупреждение PVS\-Studio: [V694](https://pvs-studio.ru/ru/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

Условие всегда истинно\. Теоретически, ложным оно может стать, если произойдёт переполнение, но это приводит к undefined behavior\.

Библиотека WebRTC\.

```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: [V789](https://pvs-studio.ru/ru/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

Анализатор обнаружил инвалидацию итератора в range\-based for цикле\. Приведённый выше код аналогичен следующему:

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

Теперь видно, что при вызове функции _push\_back_ может произойти инвалидация итераторов _format_ и _\_\_end_, если произойдёт реаллокация памяти внутри вектора\.

**Рекомендация\.** Помните, что в range\-based for циклах нельзя, как и в циклах организованных с помощью итераторов, менять количество элементов в контейнере\.

## Ошибки в логике

Проект Chromium\.

```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: [V649](https://pvs-studio.ru/ru/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

Нет смысла повторно проверять указатель _input\_scopes_, так как если он будет нулевым, то сработает проверка в начале функции, и функция вернёт статус _E\_INVALIDARG_\.

Ошибка в том, что в "_if \(\!input\_scopes\)_" забыли оператор \*\. Из\-за этого не проверятся указатель, который возвращает функция _CoTaskMemAlloc_\. Код должен быть таким:

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

Библиотека Skia\.

```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 считает этот код подозрительным сразу по двум причинам:

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

Очень подозрительный код, но мне сложно понять, как он должен работать и в чём именно здесь ошибка\. Желающие могут сами попробовать разобраться, как должна выглядеть функция _undoneSpan_ на самом деле\.

Библиотека WebKit\.

```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: V547 CWE\-570 Expression '\!first' is always false\. webmediaconstraints\.cpp 302

Так как переменная _first_ всегда равна _true_, то запятые между элементами добавляться не будут\. Корректный вариант кода:

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

Библиотека ICU\.

```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: [V779](https://pvs-studio.ru/ru/docs/warnings/v779/) CWE\-561 Unreachable code detected\. It is possible that an error is present\. collationdatabuilder\.cpp 392

Цикл может быть прерван только вызовом оператора _return_\. Это значит, что присваивание, расположенное после цикла, никогда не будет выполнено\.

Библиотека Ced\.

```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: [V751](https://pvs-studio.ru/ru/docs/warnings/v751/) Parameter 'byte1' is not used inside function body\. compact\_enc\_det\.cc 2559

Очень подозрительно, что аргумент _byte1_ не используется в функции\. Не знаю, ошибка это или нет\. Даже если не ошибка, то лучше так не писать\. Такой код озадачит и тех, кто будет его сопровождать, и различные анализаторы\.

## Неправильные ожидания

Иногда программисты ошибочно представляют себе, как работают некоторые функции или конструкции языка\. Давайте посмотрим на некоторые такие ошибки\.

Проект Chromium\.

```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: V530 CWE\-252 The return value of function 'remove\_if' is required to be utilized\. pdf\_to\_emf\_converter\.cc 44

Функция _remove\_if_ ничего не удаляет, а только переставляет элементы в контейнере\. Скорее всего, здесь должно быть написано:

```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: V547 CWE\-570 Expression 'inf \=\= \- 1' is always false\. string\-stream\.cc 149

Описание функции _std::isinf_: [isinf](https://en.cppreference.com/w/cpp/numeric/math/isinf)\.

Как видите, функция _std::isinf_ возвращает тип _bool_\. Таким образом, эта функция явно используется неправильно\.

Библиотека Skia\.

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

Предупреждение PVS\-Studio: [V554](https://pvs-studio.ru/ru/docs/warnings/v554/) CWE\-762 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. grglprogrambuilder\.cpp 272

Память выделяется с помощью оператора _new \[\]_, а освобождаться будет с помощью оператора _delete_\. Классу _unique\_ptr_ надо подсказать, как управлять памятью\. Правильный код:

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

Ещё один схожий ляп в библиотеке Skia:

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

Предупреждение PVS\-Studio: V554 CWE\-762 Incorrect use of unique\_ptr\. The memory allocated with 'malloc' will be cleaned using 'delete'\. grglprogrambuilder\.cpp 275

Явно кто\-то из программистов узнал о существовании класса _std::unique\_ptr_, но  пользоваться им ещё не умеет :\)\. Память выделяется с помощью функции _malloc_, а освобождается с помощью оператора _delete_\.

Правильный код:

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

Библиотека WebKit\.

```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: [V603](https://pvs-studio.ru/ru/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

Это не вызов одного конструктора из другого конструктора\. Просто создаётся и тут же уничтожается неименованный объект\.

## Неправильные проверки указателей

Очень часто в программах неправильно написаны проверки того, является ли указатель нулевым или нет\. Ошибки этого типа можно разделить на два основных варианта:

**Первый вариант** – это когда вначале указатель разыменовывают, а уже потом проверяют:

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

**Второй вариант** – это когда вначале проверяли указатель перед использованием, а потом забыли это сделать:

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

Ошибки первого типа выявляет диагностика [V595](https://pvs-studio.ru/ru/docs/warnings/v595/), а второго типа \- диагностика [V1004](https://pvs-studio.ru/ru/docs/warnings/v1004/)\.

Подобные ошибки далеко не всегда приводят к проблемам\. Во\-первых, бывает, что указатель никогда не станет нулевым\. В этом случае ошибки вообще нет, а есть только лишняя проверка, которая сбивает с толку и людей читающих код, и анализаторы кода\. Во\-вторых, бывает, что указатель становится нулевым в крайне редких случаях, и поэтому ошибка не влияет на типовые сценарии работы программы\.

Тем не менее, сообщения V595 и V1004 заслуживают пристального внимания и правки кода\. Анализатор PVS\-Studio выдал кучу таких сообщений для кода Chromium и библиотек\. К сожалению, как я описал в вводной статье, из\-за макроса _DCHECK_ большинство из них являются ложными срабатываниями\. Поэтому мне очень быстро надоело просматривать эти сообщения\. Более внимательно подойти к сообщениям V595 и V1004 следует после настройки анализатора\.

В любом случае, уверяю, что ошибок, связанных с неправильной проверкой указателей, полно\. Некоторые из них вы сможете найти, заглянув в файл [chromium\.txt](http://cppfiles.com/chromium.txt)\. Чтобы найти остальные, нужны герои, которые настроят анализатор и изучат новый отчёт\.

Я не буду приводить здесь все замеченные ошибки, так как они достаточно однотипны\. Покажу только по два примера к каждой диагностике, чтобы полностью было понятно, о каких ошибках идёт речь\.

V595, первый пример, проект Chromium\.

```cpp
template <typename T>
void PaintOpReader::ReadFlattenable(sk_sp<T>* val) {
  // ....
  // Здесь аругмент val не используется и не проверяется.
  // ....  
  val->reset(static_cast<T*>(SkValidatingDeserializeFlattenable(
      const_cast<const char*>(memory_), bytes,
      T::GetFlattenableType())));
  if (!val)
    SetInvalid();
  ....
}
```

Предупреждение PVS\-Studio: V595 CWE\-476 The 'val' pointer was utilized before it was verified against nullptr\. Check lines: 124, 126\. paint\_op\_reader\.cc 124

Указатель _val_ разыменовывается без проверки на равенство _nullptr_\.

V595, второй пример, проект Chromium\.

```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: V595 CWE\-476 The 'factory' pointer was utilized before it was verified against nullptr\. Check lines: 122, 124\. http\_auth\_handler\_factory\.cc 122

Указатель _factory_ разыменовывается без проверки на равенство _nullptr_\.

V1004, первый пример, библиотека PDFium\.

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

Предупреждение PVS\-Studio: V1004 CWE\-476 The 'pObject2Device' pointer was used unsafely after it was verified against nullptr\. Check lines: 237, 248\. cfx\_psrenderer\.cpp 248

Указатель _pObject2Device_ может быть нулевым, о чём свидетельствует проверка этого указателя на равенство _nullptr_\. Однако далее указатель разыменовывается без предварительной проверки\.

V1004, второй пример, библиотека SwiftShader\.

```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: V1004 CWE\-476 The 'shader' pointer was used unsafely after it was verified against nullptr\. Check lines: 43, 53\. vertexprogram\.cpp 53

Указатель _shader_ может быть нулевым, о чём свидетельствуют проверки этого указателя на равенство _nullptr_\. Однако далее указатель разыменовывается без предварительной проверки\.

## Передаём привет разработчикам компании Google

Команда PVS\-Studio передаёт привет разработчикам компании Google и готова к сотрудничеству\. Возможно обсудить как минимум два варианта:

1. Можно лицензировать продукт PVS\-Studio, чтобы его могли использовать все разработчики Chrome, Chromium, а также авторы сторонних библиотек, используемых в этих проектах\. Да и вообще, можно сделать, чтобы все сотрудники Google имели лицензию\.
1. Можно заключить контракт, в рамках которого наша команда настроит анализатор PVS\-Studio под нужды компании, исправит все ошибки, которые сможет найти, будет регулярно проводить аудит кода и исправлять новые ошибки\.

Пробуйте PVS\-Studio\. [Пишите нам](https://pvs-studio.ru/ru/about-feedback/)\. Мы поможем с проверкой проектов и выдадим временную лицензию для подробного ознакомления\.

## Заключение

Спасибо всем, кто добрался до конца цикла статей\. Надеюсь, вам было интересно\.

Как видите, даже в таком качественном проекте как Chromium есть масса ошибок, которые способен выявлять анализатор PVS\-Studio\. Предлагаю и вам начать использовать его в своих проектах\.