# Chromium: шестая проверка проекта и 250 багов

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

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

## Предыстория

Меня зовут Андрей Карпов, и я являюсь евангелистом статического анализа в целом и инструмента статического анализа PVS\-Studio в частности\. Впрочем, термин "технический евангелист" уже устаревает и на его смену приходит "developer advocate"\.

Я уделяю много времени написанию материала, посвященного улучшению качества кода и повышению надежности программ\. Сейчас у меня есть новый повод написать несколько статей на эту тему \- проверка открытого проекта [Chromium](https://www.chromium.org/) с помощью анализатора PVS\-Studio\. Это большой проект, а в любом большом проекте живут баги разнообразнейших видов\. Такое разнообразие позволяет рассмотреть сразу несколько интересных тем, связанных с причинами возникновения этих багов и способами их предотвращения\.

Стоит отметить, что это далеко не первая статья, посвященная проекту Chromium\. Мои предыдущие публикации:

* Май 2011\. Андрей Карпов\. [PVS\-Studio vs Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/a0074/)\.
* Октябрь 2011\. Андрей Карпов\. [PVS\-Studio vs Chromium \- продолжение](https://pvs-studio.ru/ru/blog/posts/cpp/0113/)\.
* Август 2013\. Андрей Карпов\. [Третья проверка кода проекта Chromium с помощью анализатора PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/cpp/0205/)\.

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

* Декабрь 2013\. Евгений Рыжков\. [Как мы пытаемся продать PVS\-Studio в Google или очередные ошибки в Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/0225/)\.
* Октябрь 2016\. Филипп Хандельянц\. [Идем на рекорд: пятая проверка Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/0442/)\.

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

* Январь 2017\. Андрей Карпов\. [31 февраля](https://pvs-studio.ru/ru/blog/posts/cpp/0550/)\.

Каждый раз, когда мы проверяли проект, в нём находилось большое количество ошибок\. Новая проверка не стала исключением\. Более того, поскольку анализатор PVS\-Studio всё лучше выявляет ошибки, я просто поначалу не знал, что со всеми ними делать\. Бегло просматривая отчёт, я выписал около 250 ошибок и задумался\. Описать все 250 ошибок в одной статье? Это будет какой\-то ужас: длинно, скучно и неинтересно\. Разбить описание не несколько статей? Лучше тоже не станет, вместо одной скучной статьи появится несколько\.

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

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

## Проверка проекта

В самом конце 2017 года мой коллега Святослав Размыслов скачал исходные коды проекта Chromium, как\-то поколдовал с ними и выдал мне сгенерированный проект для Visual Studio и отчёт PVS\-Studio\. К сожалению, работать с решением в среде Visual Studio оказалось невозможно\. Среда не пережила решение, включающее в себя 5021 проект\.

![0552_Chromium_0_Preamble_ru/image2.png](https://import.viva64.com/docx/blog/0552_Chromium_0_Preamble_ru/image2.png)

Всё неимоверно тормозит, и вообще среда через некоторое время падает\. Поэтому я изучал отчёт, используя PVS\-Studio Standalone\. Это, конечно, не так удобно, как использовать привычную мне среду Visual Studio, но вполне приемлемо\.

Надо понимать, что проект Chromium \- это большой проект\. Даже не так сказал\. Это БОЛЬШОЙ ПРОЕКТ\.

Проект Chromium и используемые в нём библиотеки состоят из 114 201 файлов на языке C и C\+\+\. Количество строк кода 30 263 757\. Из них комментарии составляют 16%\.

Только то, что PVS\-Studio может проверить проект такого размера, уже является достижением :\)\. 

## Что я нашёл

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

Важно, что я нашел много ошибок, которых хватит на написание нескольких статей\. К моменту, когда я закончу публиковать последнюю строку, информация о найденных ошибках в проекте может немного устареть\. Но это не имеет значения\. Моя цель \- продемонстрировать возможности методологии [статического анализа кода](https://pvs-studio.ru/ru/blog/terms/0046/) и поделиться с читателями некоторыми рекомендациями по стилю кодирования\.

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

Ссылка на файл с описанием замеченных дефектов: [chromium\.txt](http://cppfiles.com/chromium.txt)\.

## Почему я не осилил просмотреть отчёт внимательно?

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

Более того, я пропускал фрагменты кода, где сразу не понятно \- есть ошибка или нет\.  Сообщений много, а я один\. Если бы я начал внимательно всматриваться в код, я написал бы статьи только через несколько месяцев\.

Давайте я продемонстрирую на примерах, почему некоторые предупреждения сложно понять, особенно если это незнакомый код\. А в Chromium мне не знаком ВЕСЬ код\.

Итак, анализатор PVS\-Studio выдал предупреждение на один из файлов проекта V8:

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) CWE\-570 Expression 'truncated' is always false\. objects\.cc 2867

Найдена ошибка, или это ложное срабатывание? Попробуйте самостоятельно понять в чём тут дело\. Я добавил комментарий "// <\=" туда, куда указывает анализатор\.

```cpp
void String::StringShortPrint(StringStream* accumulator,
                              bool show_details) {
  int len = length();
  if (len > kMaxShortPrintLength) {
    accumulator->Add("<Very long string[%u]>", len);
    return;
  }

  if (!LooksValid()) {
    accumulator->Add("<Invalid String>");
    return;
  }

  StringCharacterStream stream(this);

  bool truncated = false;
  if (len > kMaxShortPrintLength) {
    len = kMaxShortPrintLength;
    truncated = true;
  }
  bool one_byte = true;
  for (int i = 0; i < len; i++) {
    uint16_t c = stream.GetNext();

    if (c < 32 || c >= 127) {
      one_byte = false;
    }
  }
  stream.Reset(this);
  if (one_byte) {
    if (show_details)
      accumulator->Add("<String[%u]: ", length());
    for (int i = 0; i < len; i++) {
      accumulator->Put(static_cast<char>(stream.GetNext()));
    }
    if (show_details) accumulator->Put('>');
  } else {
    // Backslash indicates that the string contains control
    // characters and that backslashes are therefore escaped.
    if (show_details)
      accumulator->Add("<String[%u]\\: ", length());
    for (int i = 0; i < len; i++) {
      uint16_t c = stream.GetNext();
      if (c == '\n') {
        accumulator->Add("\\n");
      } else if (c == '\r') {
        accumulator->Add("\\r");
      } else if (c == '\\') {
        accumulator->Add("\\\\");
      } else if (c < 32 || c > 126) {
        accumulator->Add("\\x%02x", c);
      } else {
        accumulator->Put(static_cast<char>(c));
      }
    }
    if (truncated) {                      // <=
      accumulator->Put('.');
      accumulator->Put('.');
      accumulator->Put('.');
    }
    if (show_details) accumulator->Put('>');
  }
  return;
}
```

Разобрались? Сложно?

Сложно\! И именно поэтому я в одиночку не могу изучить все предупреждения анализатора\.

Для тех, кто поленился разбираться, поясню в чём суть\.

Итак, анализатор утверждает, что условие _if \(truncated\)_ всегда ложно\. Давайте сократим функцию, оставив самую суть:

```cpp
void F() {
  int len = length();
  if (len > kMaxShortPrintLength)
    return;

  bool truncated = false;

  if (len > kMaxShortPrintLength)
    truncated = true;

  if (truncated) {                      // <=
    accumulator->Put('.');
    accumulator->Put('.');
    accumulator->Put('.');
  }
}
```

Флаг _truncated_ должен быть равен _true_, если текст слишком длинный, то есть если выполняется условие _if \(len \> kMaxShortPrintLength\)_\. Однако, если текст слишком длинный, то выше происходит выход из функции\.

Именно поэтому _truncated_ всегда равен _false_ и три точки в конце не будут добавлены\.

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

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

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

```cpp
void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request,
                                         int bytes_read) {
  DCHECK_NE(net::ERR_IO_PENDING, bytes_read);

  if (bytes_read <= 0) {
    FinishRequest(request);
    return;
  }

  if (bytes_read > 0)
    ReadFullResponse(request);
}
```

Предупреждение PVS\-Studio: V547 CWE\-571 Expression 'bytes\_read \> 0' is always true\. resource\_prefetcher\.cc 308

В отличие от предыдущего случая, здесь всё просто\. Анализатор точно прав, заявляя, что второе условие всегда истинно\.

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

Если бы анализатор использовался регулярно, то такой избыточный код, скорее всего, даже не попал бы в систему контроля версий\. Программист увидел бы предупреждение и написал более изящно\. Например, так:

```cpp
void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request,
                                         int bytes_read) {
  DCHECK_NE(net::ERR_IO_PENDING, bytes_read);

  if (bytes_read <= 0)
    FinishRequest(request);
  else
    ReadFullResponse(request);
}
```

Анализатор здесь промолчит\. При этом код стал короче, проще и понятней\. 

Помимо [V547](https://pvs-studio.ru/ru/docs/warnings/v547/), анализатор выдал целую гору сообщений [V560](https://pvs-studio.ru/ru/docs/warnings/v560/)\. Это предупреждение сообщает, что всегда истинно или ложно не всё условие, а какая\-то его часть\.

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

Пример скучной избыточной проверки:

```cpp
template <typename ConditionT, typename ActionT>
std::unique_ptr<DeclarativeRule<ConditionT, ActionT>>
DeclarativeRule<ConditionT, ActionT>::Create(....) {
  ....
  bool bad_message = false;                                 // <=
  std::unique_ptr<ActionSet> actions = ActionSet::Create(
      browser_context, extension, rule->actions, error,
      &bad_message);                                        // <=
  if (bad_message) {                                        // <=
    *error = "An action of a rule set had an invalid "
             "structure that should have been caught "
             "by the JSON validator.";
    return std::move(error_result);
  }
  if (!error->empty() || bad_message)                       // <=
    return std::move(error_result);
  ....
}
```

Предупреждение PVS\-Studio: V560 CWE\-570 A part of conditional expression is always false: bad\_message\. declarative\_rule\.h 472

Условие:

```cpp
if (!error->empty() || bad_message)
```

можно упростить до:

```cpp
if (!error->empty())
```

Ещё есть вариант переписать код вот так:

```cpp
  if (bad_message) {
    *error = "An action of a rule set had an invalid "
             "structure that should have been caught "
             "by the JSON validator.";
  }
  if (!error->empty() || bad_message)
    return std::move(error_result);
```

Надеюсь, я смог пояснить, почему не изучил отчёт внимательно\. Это большая работа, требующая много времени\.

## Процент ложных срабатываний

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

Если настроить анализатор PVS\-Studio, то можно ожидать 10\-15% ложных срабатываний\. Пример подобной настройки описан в статье "[Характеристики анализатора PVS\-Studio на примере EFL Core Libraries, 10\-15% ложных срабатываний](https://pvs-studio.ru/ru/blog/posts/cpp/0523/)"\.

Выполнить подобную настройку для Chromium, конечно, можно, но нерационально делать это только для того, чтобы продемонстрировать какие\-то числа в статье\. Это большая работа, которую мы готовы выполнить, но за плату\. Компания Google вполне может привлечь нашу команду для настройки анализатора и, заодно, для правки всех найденных ошибок\. Да, считайте это [намёком](https://pvs-studio.ru/ru/blog/posts/0500/)\.

Настройка, однозначно, возможна и даст хороший результат\. Например, около ПОЛОВИНЫ всех ложных срабатываний связано с использованием в коде макроса DCHECK\.

Вот как выглядит этот макрос:

```cpp
#define LAZY_STREAM(stream, condition)                            \
!(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)

#define DCHECK(condition)                                         \
 LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition))\
   << "Check failed: " #condition ". "
```

С точки зрения анализатора PVS\-Studio – это просто какая\-то проверка условия и набор действий, после которых продолжается выполнение остального кода\. 

В результате анализатор выдаёт ложные предупреждения, например, для такого кода:

```cpp
bool Value::Equals(const Value* other) const {
  DCHECK(other);
  return *this == *other;
}
```

PVS\-Studio сообщает: [V1004](https://pvs-studio.ru/ru/docs/warnings/v1004/) CWE\-476 The 'other' pointer was used unsafely after it was verified against nullptr\. Check lines: 621, 622\. values\.cc 622

С точки зрения анализатора, выполняется проверка указателя _other_ на равенство _nullptr_\. Но независимо от того, является _other_ нулевым указателем или нет, далее произойдёт его разыменование\. Подобные действия анализатор считает подозрительными\.

Макрос _DCHECK_ является разновидностью _assert_\-макросов\. Но если про _assert_ анализатор знает, то что такое _DCHECK_ – не понимает\. Чтобы лучше пояснить происходящее, напишу псевдокод:

```cpp
bool Equals(T* ptr) const
{
  if (!ptr)
    LogMessage();
  return *this == *ptr;
}
```

Вот как видит код анализатор PVS\-Studio\. Вначале указатель проверяется на равенство _nullptr_\. Если указатель нулевой, то вызывается функция _LogMessage_\. При этом функция не помечена как не возвращающая управление\. Значит, независимо от того, является ли _ptr_ нулевым указателем или нет, функция продолжит выполняться\.

Далее указатель разыменовывается\. Но ведь была проверка, где его проверяли на равенство нулю\! Следовательно, указатель может быть нулевым и анализатор сигнализирует о проблеме в коде\. Вот так анализатор выдаёт много совершенно верных, но бесполезных сообщений\.

Кстати, такая реализация макроса сбивает с толку не только PVS\-Studio\. Поэтому для анализатора, встроенного в Visual Studio, сделана специальная "подпорка":

```cpp
#if defined(_PREFAST_) && defined(OS_WIN)
// See comments on the previous use of __analysis_assume.

#define DCHECK(condition)                    \
  __analysis_assume(!!(condition)),          \
      LAZY_STREAM(LOG_STREAM(DCHECK), false) \
          << "Check failed: " #condition ". "

#define DPCHECK(condition)                    \
  __analysis_assume(!!(condition)),           \
      LAZY_STREAM(PLOG_STREAM(DCHECK), false) \
          << "Check failed: " #condition ". "
#else  // !(defined(_PREFAST_) && defined(OS_WIN))
```

Если сделать аналогичную подпорку и для анализатора PVS\-Studio, то картина с ложными срабатываниями резко изменится\. По моим прикидкам, половина ложных срабатываний сразу исчезнет\. Да, именно половина\. Всё дело в том, что макрос _DCHECK_ используется огромное количество раз\.

## Другие публикации

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

1. [Красивый Chromium и корявый memset](https://pvs-studio.ru/ru/blog/posts/cpp/0553/)\.
1. [break и fallthrough](https://pvs-studio.ru/ru/blog/posts/cpp/0554/)\.
1. [Chromium: утечки памяти](https://pvs-studio.ru/ru/blog/posts/cpp/0555/)\.
1. [Chromium: опечатки](https://pvs-studio.ru/ru/blog/posts/cpp/0556/)\.
1. [Chromium: использование недостоверных данных](https://pvs-studio.ru/ru/blog/posts/cpp/0557/)\.
1. [Почему важно проверять, что вернула функция malloc](https://pvs-studio.ru/ru/blog/posts/cpp/0558/)\.
1. [Chromium: другие ошибки](https://pvs-studio.ru/ru/blog/posts/cpp/0559/)\.