Мы уже не раз проверяли Chromium, и те, кто следит за нашим блогом, могли резонно спросить: «Зачем еще одна проверка? Разве их не хватило? " Конечно, исходный код Chromium особенно чист, что было показано каждой из предыдущих проверок, но новые ошибки неизбежно продолжают появляться. Повторные проверки доказывают, что чем чаще вы используете статический анализ, тем лучше. Хорошая практика - использовать анализатор каждый день. Еще лучшая практика - анализировать новый код сразу после того, как вы закончите его писать (автоматический анализ недавно измененного кода).

Немного истории

Мы уже проверили Chromium четыре раза:

Все предыдущие проверки проводились с помощью Windows-версии PVS-Studio. Теперь он также поддерживает Linux, и в этот раз мы использовали именно эту версию.

Решение Chromium разрасталось с годами: на момент третьей проверки количество проектов достигло отметки 1169. На момент написания статьи было 4420 проектов. Исходный код тоже немного увеличился в размерах и теперь составляет 370 Мбайт (260 Мбайт в 2013 году).

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

Специфика анализа

Поговорим о деталях анализа Chromium с помощью PVS-Studio. На этот раз мы сделаем это под Linux. После того, как вы загрузили исходные файлы с помощью depot_tools и подготовили их для анализа (подробности см. Здесь перед разделом Строительство), соберите решение:

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

После этого выполните следующую команду (в одну строку):

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

где опция «-j» инициализирует анализ в многопоточном режиме. Рекомендуемое количество потоков - это количество физических ядер ЦП плюс одно (например, «-j5» для четырехъядерного ЦП).

По окончании проверки PVS-Studio выдаст журнал анализа. Воспользуйтесь утилитой PlogConverter, поставляемой с пакетом PVS-Studio, чтобы преобразовать этот журнал в один из трех форматов, которые можно удобно просматривать в других приложениях: xml, файл ошибок, список задач. В этой статье мы будем использовать формат списка задач. Здесь нас интересуют только предупреждения общего анализа каждого уровня серьезности (высокий, средний, низкий). Вот как должна выглядеть команда преобразования (в одну строку):

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

Более подробную информацию о параметрах PlogConverter можно найти здесь. Чтобы открыть список задач chromium.tasks в QtCreator (вам необходимо установить его заранее), выполните следующую команду:

qtcreator path/to/saved/chromium.tasks

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

Вот как журнал отображается в QtCreator:

Рисунок 1. Просмотр результатов анализа в QtCreator (щелкните, чтобы увеличить)

Статистика анализа

PVS-Studio выдал 2312 предупреждений. На диаграмме ниже показано распределение предупреждений по уровням серьезности:

Рисунок 2. Распределение предупреждений по уровням серьезности

Кратко прокомментирую этот график: анализатор выдал 171 предупреждение высокого уровня, 290 предупреждений среднего уровня и 1851 предупреждение низкого уровня.

Несмотря на кажущееся большое количество предупреждений, на самом деле это мало для такого масштабного проекта. Общее количество SLOC без связанных библиотек составляет 6468751. Если рассматривать предупреждения только высокого и среднего уровней, я бы сказал, что среди них всего 220 настоящих ошибок. Что ж, это статистика, а реальная плотность ошибок составляет 0,034 на 1000 LOC. Однако на этом рисунке учтены только те ошибки, которые обнаружил PVS-Studio, а точнее, попались мне на глаза при просмотре лога.

Другие проекты обычно имеют более высокую плотность ошибок, поэтому разработчики Chromium преуспели! Но даже в этом случае не расслабляйтесь: ошибки все равно есть, и они далеко не безобидны.

Наиболее интересные из них обсуждаются ниже.

Новые ошибки

Копировать вставить

Предупреждение PVS-Studio: V501 Слева и справа от оператора && есть идентичные подвыражения request_body_send_buf_ == nullptr. http_stream_parser.cc 1222

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

Это классика. Указатель request_body_send_buf_ дважды сравнивается с nullptr. Это должна быть опечатка, поэтому есть другой член класса, который следует сравнить с nullptr.

Предупреждение PVS-Studio: V766 уже добавлен элемент с таким же ключом colorSectionBorder. ntp_resource_cache.cc 581

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

Анализатор обнаружил странную двойную инициализацию объекта, связанного с ключом «colorSectionBorder». Переменная подстановки здесь представляет собой ассоциативный массив. При инициализации переменная color_section_border типа SkColor (определенная как uint32_t) преобразуется в строковое представление RGBA (как предлагается имя метода SkColorToRGBAString) и сопоставлен с ключом «colorSectionBorder». После этого color_section_border приводится к другому строковому формату (метод SkColorToRGBComponents) и сопоставляется с тем же ключом. Это означает, что предыдущее значение, связанное с ключом «colorSectionBorder», будет потеряно. Если это то, что задумал программист, то одно из назначений следует убрать. В противном случае компоненты цвета должны быть сопоставлены с разными ключами.

Примечание. Кстати, это первая ошибка, обнаруженная диагностикой V766 в реальном проекте. Это особый тип ошибок, но Chromium настолько велик, что там можно найти даже такие экзотические ошибки.

Неправильная обработка указателя

А теперь небольшая разминка для мозгов. Посмотрите на приведенный ниже код и попробуйте найти ошибку самостоятельно.

// 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: V522 могло произойти разыменование нулевого указателя item. action_wait.cc 41

Авторы кодекса приняли сознательное решение выстрелить себе в ногу. Код выполняет итерацию по очереди queue, состоящей из идентификаторов, представленных в виде строк. Идентификатор извлекается из очереди, а затем вызывается метод FindUpdateItemById для возврата указателя на объект типа CrxUpdateItem, связанный с этим идентификатором. Если FindUpdateItemById завершится неудачно, он вернет nullptr, который затем будет разыменован в ветви then инструкции if.

Это фиксированный код:

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

Предупреждение PVS-Studio: V620 Необычно, что выражение sizeof (T) * N kind суммируется с указателем на тип T. string_conversion.cc 62

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;
  ....
}

Анализатор обнаружил фрагмент кода со странной адресной арифметикой. Как следует из названия, функция преобразует символы из формата UTF-8 в UTF-16. Текущий стандарт Unicode 6.x подразумевает расширение символа UTF-8 до четырех байтов, поэтому символ UTF-8 декодируется как два символа UTF-16 (символы UTF-16 жестко закодированы двумя байтами). Декодирование выполняется с использованием четырех указателей: два указывают на начало, а два других указывают на конец массивов in и out. Указатели на конец массивов действуют как итераторы STL: они указывают на место после последнего элемента массива. Хотя указатель source_end_ptr вычисляется правильно, для target_end_ptr все усложняется. Он должен был указывать на место после второго элемента массива out (т. Е. Перемещаться на четыре байта по отношению к указателю out), но что это будет на самом деле указывает на адрес после четвертого элемента (т.е. out будет сдвинут на восемь байтов).

Это запланированная логика:

И вот что происходит на самом деле:

Фиксированный код:

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;
  ....
}

Также анализатор выявил еще один потенциальный дефект такого типа:

  • V620 Необычно, что выражение sizeof (T) * N kind суммируется с указателем на тип T. string_conversion.cc 106

Разное

Очередная разминка. Можете ли вы найти ошибку в приведенном ниже коде?

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

Предупреждение PVS-Studio: V591 Непустая функция должна возвращать значение. memory_allocator.h 39

Здесь мы имеем дело с неопределенным поведением. Стандарт C ++ говорит, что любой непустой метод должен возвращать значение. А как насчет нашего примера? В операторе присваивания текущий объект проверяется на равенство самому себе (объекты сравниваются с использованием их указателей), и поля копируются (если указатели разные). Однако метод не возвращает ссылку на себя (return * this).

Еще два непустых метода, которые не возвращаются:

  • V591 Непустая функция должна возвращать значение. sandbox_bpf.cc 115
  • V591 Непустая функция должна возвращать значение. events_x.cc 73

Предупреждение PVS-Studio: V583 Оператор ?:, независимо от его условного выражения, всегда возвращает одно и то же значение: 1. configurator_impl.cc 133

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

Этот код всегда возвращает 1 в качестве времени задержки. Возможно, это просто неполный код, который предстоит разработать позже, но текущая реализация тернарного оператора не приносит никакой пользы.

Предупреждение PVS-Studio: V590 Рассмотрите возможность проверки ‘rv == OK || rv! = ERR_ADDRESS_IN_USE ’выражение. Выражение является чрезмерным или содержит опечатку. udp_socket_posix.cc 735

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));
}

Анализатор предупреждает нас о возможном избыточном сравнении. Приведенный выше код отображает IP на случайный порт. Успешное сопоставление завершает цикл (который подсчитывает количество попыток сопоставления). Удаление одного из сравнений не повлияет на логику кода (в текущей версии цикл останавливается, если сопоставление прошло успешно или если не было выдано никакой ошибки о сопоставлении порта с другим IP-адресом).

Предупреждение PVS-Studio: V523 Оператор then эквивалентен оператору else.

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;
}

Анализатор обнаружил идентичные операторы в ветвях then и else оператора if. Каковы возможные последствия? Текущая логика предполагает, что некэшированный URL-запрос (net :: URLRequest * request) будет завершен так же, как и кешированный. Если это именно то, что имел в виду программист, то оператор else можно безопасно удалить:

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

В противном случае будет вызван неправильный метод, что может привести к тому, что вы проведете множество бессонных ночей и выпьете тонны кофе, пытаясь отладить код.

Предупреждение PVS-Studio: V609 Разделить на ноль. Диапазон знаменателя [0..4096]. адрес h 159

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; // <=
}

А что насчет этого кода? Это может привести к неуловимой ошибке. Метод RequiredBlocks выполняет деление по значению переменной block_size (оценивается методом BlockSizeForFileType). Оператор switch в методе BlockSizeForFileType сравнивает значение перечисления FileType, переданное методу, с некоторыми значениями и возвращает одно из них, но там также является значением по умолчанию, 0. Предположим, программист решил добавить новое значение в перечисление FileType, но забыл добавить соответствующую метку case к переключателю тело оператора. Эта ошибка может привести к неопределенному поведению: стандарт C ++ не подразумевает создание исключения программного обеспечения при делении на ноль. Вместо этого будет вызвано аппаратное исключение, которое не может быть обнаружено с помощью стандартного блока try / catch (вместо этого используются обработчики сигналов; дополнительную информацию можно найти Здесь и здесь).

Предупреждение PVS-Studio: V519 Переменной * list два раза подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 136, 138. util.cc 138

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;
  }
  ....
}

Это распространенная ошибка при реализации оператора switch. Программист ожидает, что если переменная list_id окажется равной значению MODULEWHITELIST из перечисления ListType, строка, на которую указывает list будет инициализирован значением kModuleWhitelist, и выполнение оставит оператор switch. Однако из-за отсутствия оператора break выполнение перейдет к следующей метке case, RESOURCEBLACKLIST, что приведет к связыванию * list со строкой kResourceBlacklist.

Выводы

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

Какие инструменты статического анализа использовать? Что ж, их на самом деле много. Лично я, естественно, предлагаю попробовать PVS-Studio. Он может легко интегрироваться с Visual Studio IDE или, альтернативно, с любой системой сборки. Также с недавнего времени доступна версия для Linux. Более подробную информацию о версиях для Windows и Linux можно найти здесь и здесь.

Филипп Ханделянц