Андрей Карпов

25 Янв 2018

Теги:

#Cpp

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

25 Янв 2018

Автор: Андрей Карпов

Предыстория
Проверка проекта
Что я нашёл
Почему я не осилил просмотреть отчёт внимательно?
Процент ложных срабатываний
Другие публикации

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

Предыстория

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

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

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

Май 2011. Андрей Карпов. PVS-Studio vs Chromium.
Октябрь 2011. Андрей Карпов. PVS-Studio vs Chromium - продолжение.
Август 2013. Андрей Карпов. Третья проверка кода проекта Chromium с помощью анализатора PVS-Studio.

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

Декабрь 2013. Евгений Рыжков. Как мы пытаемся продать PVS-Studio в Google или очередные ошибки в Chromium.
Октябрь 2016. Филипп Хандельянц. Идем на рекорд: пятая проверка Chromium.

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

Январь 2017. Андрей Карпов. 31 февраля.

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

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

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

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

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

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

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

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

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

Что я нашёл

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

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

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

Ссылка на файл с описанием замеченных дефектов: chromium.txt.

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

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

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

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

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

V547 CWE-570 Expression 'truncated' is always false. objects.cc 2867

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

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) всегда ложно. Давайте сократим функцию, оставив самую суть:

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-ю часть. Поэтому, если взяться внимательно изучать, будет найдено гораздо больше ошибок, чем я выписал.

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

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

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

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

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

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, анализатор выдал целую гору сообщений V560. Это предупреждение сообщает, что всегда истинно или ложно не всё условие, а какая-то его часть.

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

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

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

Условие:

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

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

if (!error->empty())

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

  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% ложных срабатываний".

Выполнить подобную настройку для Chromium, конечно, можно, но нерационально делать это только для того, чтобы продемонстрировать какие-то числа в статье. Это большая работа, которую мы готовы выполнить, но за плату. Компания Google вполне может привлечь нашу команду для настройки анализатора и, заодно, для правки всех найденных ошибок. Да, считайте это намёком.

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

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

#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 – это просто какая-то проверка условия и набор действий, после которых продолжается выполнение остального кода.

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

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

PVS-Studio сообщает: 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 – не понимает. Чтобы лучше пояснить происходящее, напишу псевдокод:

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

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

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

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

#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 используется огромное количество раз.