Михаил Гельвих

01 Дек 2021

Теги:

#Cpp

Проверка Chromium спустя три года. Ну и как оно?

01 Дек 2021

Автор: Михаил Гельвих

Введение
Как проверяли
Ошибки в работе с указателями
Опечатки
Неправильная работа с типами
Любимые сравнения
Always true/false
Всякое-разное
Заключение

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

Введение

Chromium – это свободный браузер с открытым исходным кодом. Иногда его ещё называют браузером-конструктором из-за того, что он является идеальной базой для создания собственного браузера. В нём поддерживаются все самые актуальные веб-технологии, отсутствуют сторонние дополнения и присутствует возможность бесконечной кастомизации. Разрабатывается компанией Google и сообществом Chromium. Официальный репозиторий.

Немногие помнят, но данная статья является уже седьмой нашей проверкой Chromium. С какой стати столько внимания? Всё просто – данный проект интересен своими размерами и основательным подходом к качеству кода. Как раз недавно наша команда переводила план по работе над надёжностью, написанный разработчиками Chromium/Chrome. Ознакомиться с ним можно здесь.

Также, думаю, будет нелишним дать ссылки на прошлые статьи нашего нерегулярного цикла:

PVS-Studio vs Chromium (Май 2011)
PVS-Studio vs Chromium – продолжение (Октябрь 2011)
Третья проверка кода проекта Chromium с помощью анализатора PVS-Studio (Август 2013)
Как мы пытаемся продать PVS-Studio в Google или очередные ошибки в Chromium (Декабрь 2013)
Идём на рекорд: пятая проверка Chromium (Октябрь 2016)
Chromium: шестая проверка проекта и 250 багов (Январь 2018)

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

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

Недавно мой коллега написал отличную статью о том, как эта "магия" работает – "Межмодульный анализ C++ проектов в PVS-Studio". Ну а я не вижу смысла пересказывать чужую статью, ведь и в этой материала более чем достаточно :)

Как проверяли

В этот раз проверка проводилась на Windows системе с помощью инструмента "C and C++ Compiler Monitoring UI". Кратко напомню, что он позволяет отследить все вызовы компилятора при сборке проекта и уже по её окончанию проверить все задействованные файлы. Для анализа в такой конфигурации необходимо запустить Standalone, а после этого полную сборку проекта. Более подробно ознакомиться с этим и другими способами проверки проектов можно в нашей документации.

Что до самой сборки – она прошла без каких-либо проблем, благо на официальном сайте расположена довольно подробная инструкция.

Ну и несколько важных пояснений перед основным текстом:

все найденные ошибки относятся к этому состоянию репозитория;
из проверки были исключены сторонние библиотеки, расположенные в папках src/buildtools и src/third_party, т. к. многие из них достойны отдельного разбора. Один из таких, например, недавно сделал мой коллега – "Брутальный Protocol Buffers от Google vs статический анализ кода".
Фрагменты кода, приведённые в статье, могут незначительно отличаться от тех, что расположены в официальном репозитории. В некоторых местах изменено форматирование кода для удобства чтения статьи, а где-то добавлены поясняющие комментарии.

Ну а теперь, собственно, перейдём к анализу некоторых ошибок, найденных в свежей сборке Chromium.

Ошибки в работе с указателями

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

Случай N1

V595 The 'client_' pointer was utilized before it was verified against nullptr. Check lines: 'password_manager_util.cc:119', 'password_manager.cc:1216', 'password_manager.cc:1218'. password_manager.cc 1216

// File: src\components\password_manager\core\browser\password_manager_util.cc
bool IsLoggingActive(const password_manager::PasswordManagerClient* client)
{
  const autofill::LogManager* log_manager = client->GetLogManager();
  return log_manager && log_manager->IsLoggingActive();
}

// File: src\components\password_manager\core\browser\password_manager.cc
void PasswordManager::RecordProvisionalSaveFailure(
    PasswordManagerMetricsRecorder::ProvisionalSaveFailure failure,
    const GURL& form_origin) 
  {
  std::unique_ptr<BrowserSavePasswordProgressLogger> logger;
  if (password_manager_util::IsLoggingActive(client_)) {            // <=
    logger = std::make_unique<BrowserSavePasswordProgressLogger>(
        client_->GetLogManager());
  }
  if (client_ && client_->GetMetricsRecorder()) {                   // <=
    ....
  }
}

Здесь анализатор обнаружил небезопасный вызов функции IsLoggingActive, в результате которого она может получить нулевой указатель в качестве аргумента, который потом без проверки и разыменовывает. А небезопасным этот вызов анализатор посчитал, потому что если посмотреть код чуть ниже, то можно увидеть, что этот же указатель проверяется – и в зависимости от его состояния выполняются дальнейшие действия.

Случай N2

V595 The 'parent' pointer was utilized before it was verified against nullptr. Check lines: 'visibility_controller.cc:95', 'native_web_contents_modal_dialog_manager_views.cc:72', 'native_web_contents_modal_dialog_manager_views.cc:75'. native_web_contents_modal_dialog_manager_views.cc 72

// File: src\ui\wm\core\visibility_controller.cc
void SetChildWindowVisibilityChangesAnimated(aura::Window* window)
{
  window->SetProperty(kChildWindowVisibilityChangesAnimatedKey, true);
}

// File: src\components\constrained_window
//       \native_web_contents_modal_dialog_manager_views.cc
void NativeWebContentsModalDialogManagerViews::ManageDialog()
{
  views::Widget* widget = GetWidget(dialog());
  ....
#if defined(USE_AURA)
  ....
  gfx::NativeView parent = widget->GetNativeView()->parent();
  wm::SetChildWindowVisibilityChangesAnimated(parent);
  ....
  if (parent && parent->parent())
  {
    parent->parent()->SetProperty(aura::client::kAnimationsDisabledKey, true);
  }
  ....
#endif
}

Ситуация один в один, как и выше: берётся указатель и без проверки передаётся в функцию, где вновь без проверки разыменовывается. Также передаётся указатель в функцию, а уже потом проверятся. Если подразумевалось, что указатель parent не может быть нулевым, то зачем он тогда проверяется ниже? Однозначно, подозрительный код, который следует проверить разработчикам.

Случай N3

V522 Instantiation of WasmFullDecoder < Decoder::kFullValidation, WasmGraphBuildingInterface >: Dereferencing of the null pointer 'result' might take place. The null pointer is passed into 'UnOp' function. Inspect the fourth argument. Check lines: 'graph-builder-interface.cc:349', 'function-body-decoder-impl.h:5372'. graph-builder-interface.cc 349

// File: src\v8\src\wasm\graph-builder-interface.cc
void UnOp(FullDecoder* decoder, WasmOpcode opcode,
          const Value& value, Value* result)
{
  result->node = builder_->Unop(opcode, value.node, decoder->position());
}

Здесь анализатор обнаружил разыменование нулевого указателя result в функции UnOp. Сам вызов UnOp с nullptr в качестве аргумента происходит в коде, приведённом ниже:

// File: src\v8\src\wasm\function-body-decoder-impl.h
int BuildSimpleOperator(WasmOpcode opcode, ValueType return_type,
                        ValueType arg_type)
{
  Value val = Peek(0, 0, arg_type);
  if (return_type == kWasmVoid)
  {
    CALL_INTERFACE_IF_OK_AND_REACHABLE(UnOp, opcode, val, nullptr);  // <=
    Drop(val);
  }
  ....
}

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

Случай N4

V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'NaClTlsSetCurrentThread' function. Inspect the first argument. Check lines: 'nacl_tls_64.c:285', 'nacl_app_thread.c:161'. nacl_tls_64.c 285

// File: src\native_client\src\trusted\service_runtime\arch\x86_64\nacl_tls_64.c
void NaClTlsSetCurrentThread(struct NaClAppThread *natp) {
  nacl_current_thread = &natp->user;
}

// File: src\native_client\src\trusted\service_runtime\nacl_app_thread.c
void NaClAppThreadTeardown(struct NaClAppThread *natp)
{
  ....
  /*
  * Unset the TLS variable so that if a crash occurs during thread
  * teardown, the signal handler does not dereference a dangling
  * NaClAppThread pointer.
  */
  NaClTlsSetCurrentThread(NULL);
  ....
}

Явная передача нулевого указателя в функцию с его последующим разыменованием. Судя по рядом расположенному комментарию, NULL передаётся сознательно, но вот конструкция, используемая в функции NaClTlsSetCurrentThread, явно приведёт к неопределённому поведению. Почему именно к неопределённому поведению, а не падению? Этот вопрос ещё несколько лет назад подробно разобрал мой коллега в статье "Разыменовывание нулевого указателя приводит к неопределённому поведению". Не вижу смысла повторяться, т. к. статья там вышла основательная и заслуживает отдельного внимания.

Опечатки

Случай N5

V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'it'. tree_synchronizer.cc 143

template <typename Iterator>
static void PushLayerPropertiesInternal(Iterator source_layers_begin,
                                        Iterator source_layers_end,
                                        LayerTreeHost* host_tree,
                                        LayerTreeImpl* target_impl_tree) 
{
  for (Iterator it = source_layers_begin; it != source_layers_end; ++it) 
  {
    auto* source_layer = *it;
    ....
    if (!target_layer) {
      bool host_set_on_source =
        source_layer->layer_tree_host() == host_tree;

      bool source_found_by_iterator = false;
      for (auto host_tree_it = host_tree->begin();
           host_tree_it != host_tree->end(); ++it)    // <=
      {
        if (*host_tree_it == source_layer) 
        {
          source_found_by_iterator = true;
          break;
        }
      }
      ....
    }
    ....
  }
}

Хм... Во вложенном цикле инкрементируется итератор внешнего цикла... Где-то у меня была подходящая картинка...

Случай N6

V501 There are identical sub-expressions 'user_blocking_count_ == 0' to the left and to the right of the '&&' operator. process_priority_aggregator.cc 98

bool ProcessPriorityAggregator::Data::IsEmpty() const {
#if DCHECK_IS_ON()
  if (lowest_count_)
    return false;
#endif
  return user_blocking_count_ == 0 && user_blocking_count_ == 0;
}

Программист два раза проверил одну и ту же переменную на соответствие нулю. Странно, да? Думаю, стоит заглянуть в класс, к которому относится данная функция:

class ProcessPriorityAggregator::Data 
{
  ....
private:
  ....
#if DCHECK_IS_ON()
  ....
  uint32_t lowest_count_ = 0;
#endif
  uint32_t user_visible_count_ = 0;
  uint32_t user_blocking_count_ = 0;
};

Ну вот, всё встало на свои места. Скорее всего, во втором случае должна была использоваться переменная user_visible_count, которая находится рядом с user_blocking_count:

return user_blocking_count_ == 0 && user_visible_count_ == 0;

Неправильная работа с типами

Случай N7

V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. builtins-trace.cc 64

class MaybeUtf8
{
  ....
  private:

    void AllocateSufficientSpace(int len)
    {
      if (len + 1 > MAX_STACK_LENGTH)
      {
        allocated_.reset(new uint8_t[len + 1]);  // <=
        buf_ = allocated_.get();
      }
    }

    ....
    std::unique_ptr<uint8_t> allocated_;         // <=
}

Чувствуете, чем пахнет? Да это же утечка памяти и неопределённое поведение в одном флаконе. Где? А в объявлении unique_ptr! В данном случае объявлен умный указатель на uint8_t, а выше в него пытаются положить массив. В итоге мало того, что не будет очищена память, занятая элементами массива, так ещё и вызов оператора delete вместо delete[] приведёт к неопределённому поведению!

Для исправления проблемы надо заменить строку объявления умного указателя на такую:

std::unique_ptr<uint8_t[]> allocated_;

Если вы сомневаетесь в моих словах, то можете почитать, например, draft стандарта С++ 20 пункт 7.6.2.9.2 (PDF) или мой любимый cppreference.com в разделе "delete expression".

Любимые сравнения

Случай N8

V501 There are identical sub-expressions 'file.MatchesExtension(L".xlsb")' to the left and to the right of the '||' operator. download_type_util.cc 60

ClientDownloadRequest::DownloadType GetDownloadType(const base::FilePath& file)
{
  ....
  if (file.MatchesExtension(FILE_PATH_LITERAL(".apk")))
    return ClientDownloadRequest::ANDROID_APK;
  ....
  else if (file.MatchesExtension(FILE_PATH_LITERAL(".pdf")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".doc")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docb")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dot")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xls")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||    // <=
           file.MatchesExtension(FILE_PATH_LITERAL(".xlt")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||    // <=
           file.MatchesExtension(FILE_PATH_LITERAL(".xla")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xll")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlw")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppt")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pot")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pps")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".sldx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".rtf")))
    return ClientDownloadRequest::DOCUMENT;
  ....
}

Не раз слышал мнение, что форматирование таблицей спасает от ошибок с повторением элементов при однотипной записи. Как видим, этого недостаточно. Немного улучшить ситуацию можно, просто отсортировав записи. Попробуйте поискать ошибки в коде ниже (да, она там не одна). Я даже специально уберу маркеры ошибки.

ClientDownloadRequest::DownloadType GetDownloadType(const base::FilePath& file)
{
  ....
  if (file.MatchesExtension(FILE_PATH_LITERAL(".apk")))
    return ClientDownloadRequest::ANDROID_APK;
  ....
  else if (file.MatchesExtension(FILE_PATH_LITERAL(".doc"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docb")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dot"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pdf"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pot"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pps"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppt"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".rtf"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".sldx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xla"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xll"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlm"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xls"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlt"))  ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlw")))
    return ClientDownloadRequest::DOCUMENT;
  ....
}

Случай N9

V501 There are identical sub-expressions to the left and to the right of the '&&' operator. password_form.cc 265

bool operator==(const PasswordForm& lhs, const PasswordForm& rhs) {
  return lhs.scheme == rhs.scheme && lhs.signon_realm == rhs.signon_realm &&
         lhs.url == rhs.url && lhs.action == rhs.action &&
         lhs.submit_element == rhs.submit_element &&
         lhs.username_element == rhs.username_element &&
         lhs.username_element_renderer_id == rhs.username_element_renderer_id &&
         lhs.username_value == rhs.username_value &&
         lhs.all_possible_usernames == rhs.all_possible_usernames &&
         lhs.all_possible_passwords == rhs.all_possible_passwords &&
         lhs.form_has_autofilled_value == rhs.form_has_autofilled_value &&
         lhs.password_element == rhs.password_element &&
         lhs.password_element_renderer_id == rhs.password_element_renderer_id &&
         lhs.password_value == rhs.password_value &&
         lhs.new_password_element == rhs.new_password_element &&
         lhs.confirmation_password_element_renderer_id ==                // <=
             rhs.confirmation_password_element_renderer_id &&            // <=
         lhs.confirmation_password_element ==
             rhs.confirmation_password_element &&
         lhs.confirmation_password_element_renderer_id ==                // <=
             rhs.confirmation_password_element_renderer_id &&            // <=
         lhs.new_password_value == rhs.new_password_value &&
         lhs.date_created == rhs.date_created &&
         lhs.date_last_used == rhs.date_last_used &&
         lhs.date_password_modified == rhs.date_password_modified &&
         lhs.blocked_by_user == rhs.blocked_by_user && lhs.type == rhs.type &&
         lhs.times_used == rhs.times_used &&
         lhs.form_data.SameFormAs(rhs.form_data) &&
         lhs.generation_upload_status == rhs.generation_upload_status &&
         lhs.display_name == rhs.display_name && lhs.icon_url == rhs.icon_url &&
         // We compare the serialization of the origins here, as we want unique
         // origins to compare as '=='.
         lhs.federation_origin.Serialize() ==
             rhs.federation_origin.Serialize() &&
         lhs.skip_zero_click == rhs.skip_zero_click &&
         lhs.was_parsed_using_autofill_predictions ==
             rhs.was_parsed_using_autofill_predictions &&
         lhs.is_public_suffix_match == rhs.is_public_suffix_match &&
         lhs.is_affiliation_based_match == rhs.is_affiliation_based_match &&
         lhs.affiliated_web_realm == rhs.affiliated_web_realm &&
         lhs.app_display_name == rhs.app_display_name &&
         lhs.app_icon_url == rhs.app_icon_url &&
         lhs.submission_event == rhs.submission_event &&
         lhs.only_for_fallback == rhs.only_for_fallback &&
         lhs.is_new_password_reliable == rhs.is_new_password_reliable &&
         lhs.in_store == rhs.in_store &&
         lhs.moving_blocked_for_list == rhs.moving_blocked_for_list &&
         lhs.password_issues == rhs.password_issues;
}

Думаю, совет про форматирование таблицей здесь неуместен :) Здесь поможет только качественный рефакторинг. Кстати, путём нехитрых манипуляций с текстовым редактором и Python удалось выяснить, что в операторе сравнения не проверяются следующие поля класса:

accepts_webauthn_credentials
new_password_element_renderer_id
server_side_classification_successful
encrypted_password
username_may_use_prefilled_placeholder

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

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

V501 There are identical sub-expressions 'card.record_type() == CreditCard::VIRTUAL_CARD' to the left and to the right of the '||' operator. full_card_request.cc 107
V501 There are identical sub-expressions '!event->target()' to the left and to the right of the '||' operator. accelerator_filter.cc 28
V501 There are identical sub-expressions 'generation_id->empty()' to the left and to the right of the '||' operator. record_handler_impl.cc 393
V501 There are identical sub-expressions 'JSStoreNamedNode::ObjectIndex() == 0' to the left and to the right of the '&&' operator. js-native-context-specialization.cc 1102
V501 There are identical sub-expressions 'num_previous_succeeded_connections_ == 0' to the left and to the right of the '&&' operator. websocket_throttler.cc 63

Always true/false

Случай N10

V616 The 'extensions::Extension::NO_FLAGS' named constant with the value of 0 is used in the bitwise operation. extensions_internals_source.cc 98

base::Value CreationFlagsToList(int creation_flags)
{
  base::Value flags_value(base::Value::Type::LIST);
  if (creation_flags & extensions::Extension::NO_FLAGS)  // <=
    flags_value.Append("NO_FLAGS");
  if (creation_flags & extensions::Extension::REQUIRE_KEY)
    flags_value.Append("REQUIRE_KEY");
  if (creation_flags & extensions::Extension::REQUIRE_MODERN_MANIFEST_VERSION)
    flags_value.Append("REQUIRE_MODERN_MANIFEST_VERSION");
  if (creation_flags & extensions::Extension::ALLOW_FILE_ACCESS)
    flags_value.Append("ALLOW_FILE_ACCESS");
  ....
  return flags_value;
}

// File: src\extensions\common\extension.h
enum InitFromValueFlags
{
  NO_FLAGS = 0,
  REQUIRE_KEY = 1 << 0,
  REQUIRE_MODERN_MANIFEST_VERSION = 1 << 1,
  ALLOW_FILE_ACCESS = 1 << 2,
  ....
};

В данном примере прошу обратить внимание на первое выражение условного оператора. В нём происходит побитовое умножение с extensions::Extension::NO_FLAGS, но оно раскрывается в ноль, а следовательно, всегда будет вычисляться, как ложь, и никогда не выполнится.

Скорее всего, первая проверка должна была быть написана так:

creation_flags == extensions::Extension::NO_FLAGS

Случай N11

V547 Expression 'entry_size > 0' is always true. objects-printer.cc 1195

void FeedbackVector::FeedbackVectorPrint(std::ostream& os)
{
  ....
  FeedbackMetadataIterator iter(metadata());
  while (iter.HasNext()) {
    ....
    int entry_size = iter.entry_size();
    if (entry_size > 0) os << " {";         // <=
    for (int i = 0; i < entry_size; i++)
    {
      ....
    }
    if (entry_size > 0) os << "\n  }";      // <=
  }
  os << "\n";
}

int FeedbackMetadataIterator::entry_size() const
{
  return FeedbackMetadata::GetSlotSize(kind());
}

int FeedbackMetadata::GetSlotSize(FeedbackSlotKind kind) {
  switch (kind) {
    case FeedbackSlotKind::kForIn:
    ....
      return 1;

    case FeedbackSlotKind::kCall:
    ....
      return 2;

    case FeedbackSlotKind::kInvalid:
    ....
      UNREACHABLE();
  }
  return 1;
}

Ну и на закуску, небольшой пример работы механизма DataFlow.

Анализатор говорит нам, что значение переменной entry_size всегда будет больше ноля, а следовательно, условный код, проверяющий переменную, будет выполняться всегда. Откуда анализатор узнал результат вычисления переменной? Просто вычислил диапазон возможных значений переменной после выполнения функций FeedbackMetadataIterator::entry_size и FeedbackMetadata::GetSlotSize.

Всякое-разное

Случай N12

V501 There are identical sub-expressions 'StandardFrameConstants::kCallerPCOffset' to the left and to the right of the '-' operator. linkage.h 90

static LinkageLocation ForCalleeFrameSlot(int32_t slot, MachineType type)
{
  // TODO(titzer): bailout instead of crashing here.
  DCHECK(slot >= 0 && slot < LinkageLocation::MAX_STACK_SLOT);
  return LinkageLocation(STACK_SLOT, slot, type);
}

static LinkageLocation ForSavedCallerReturnAddress()
{
  return ForCalleeFrameSlot((StandardFrameConstants::kCallerPCOffset  // <=
                           - StandardFrameConstants::kCallerPCOffset) // <=
                           / kSystemPointerSize,
                             MachineType::Pointer());
}

Функция ForSavedCallerReturnAddress внутри себя вызывает функцию ForCalleeFrameSlot c первым аргументом всегда равным нулю. Ведь при вычислении первого аргумента происходит вычитание переменной kCallerPCOffset из себя же. Скорее всего, это опечатка, потому что рядом с этой функцией расположено несколько сильно похожих функций, но уже с другими переменными:

static LinkageLocation ForSavedCallerFramePtr() 
{
  return ForCalleeFrameSlot((StandardFrameConstants::kCallerPCOffset -
                             StandardFrameConstants::kCallerFPOffset) /
                             kSystemPointerSize,
                             MachineType::Pointer());
}

static LinkageLocation ForSavedCallerConstantPool() 
{
  DCHECK(V8_EMBEDDED_CONSTANT_POOL);
  return ForCalleeFrameSlot((StandardFrameConstants::kCallerPCOffset -
                             StandardFrameConstants::kConstantPoolOffset) /
                             kSystemPointerSize,
                             MachineType::AnyTagged());
}

static LinkageLocation ForSavedCallerFunction() 
{
  return ForCalleeFrameSlot((StandardFrameConstants::kCallerPCOffset -
                             StandardFrameConstants::kFunctionOffset) /
                             kSystemPointerSize,
                             MachineType::AnyTagged());
}

Случай N13

V684 A value of the variable 'flags' is not modified. Consider inspecting the expression. It is possible that '1' should be present instead of '0'. usb_device_handle_win.cc 58

V684 A value of the variable 'flags' is not modified. Consider inspecting the expression. It is possible that '1' should be present instead of '0'. usb_device_handle_win.cc 67

uint8_t BuildRequestFlags(UsbTransferDirection direction,
                          UsbControlTransferType request_type,
                          UsbControlTransferRecipient recipient)
{
  uint8_t flags = 0;

  switch (direction) {
    case UsbTransferDirection::OUTBOUND:
      flags |= BMREQUEST_HOST_TO_DEVICE << 7;    // <=
      break;
    case UsbTransferDirection::INBOUND:
      flags |= BMREQUEST_DEVICE_TO_HOST << 7;
      break;
  }

  switch (request_type) {
    case UsbControlTransferType::STANDARD:
      flags |= BMREQUEST_STANDARD << 5;          // <=
      break;
    case UsbControlTransferType::CLASS:
      flags |= BMREQUEST_CLASS << 5;
      break;
    ....
  }
  ....
  return flags;
}

BMREQUEST_HOST_TO_DEVICE и BMREQUEST_STANDARD раскрываются в ноль, что не имеет смысла при операции ИЛИ.

Сначала я подумал, что значения этих макросов как-то по-разному определяются в разных файлах, но беглый поиск по всей папке с исходниками показал единственное их определение:

#define BMREQUEST_HOST_TO_DEVICE 0
....
#define BMREQUEST_STANDARD 0

Честно говоря, не уверен, что здесь ошибка, но как минимум обратить внимание разработчиков хотелось бы.

Случай N14

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1969, 1971. objects.cc 1969

void HeapObject::HeapObjectShortPrint(std::ostream& os)
{
  ....
  switch (map().instance_type()) {
    ....
    case FEEDBACK_CELL_TYPE: {
      {
        ReadOnlyRoots roots = GetReadOnlyRoots();
        os << "<FeedbackCell[";
        if (map() == roots.no_closures_cell_map()) {           // <=
          os << "no feedback";
        } else if (map() == roots.no_closures_cell_map()) {    // <=
          os << "no closures";
        } else if (map() == roots.one_closure_cell_map()) {
          os << "one closure";
        } else if (map() == roots.many_closures_cell_map()) {
          os << "many closures";
        } else {
          os << "!!!INVALID MAP!!!";
        }
        os << "]>";
      }
      break;
    }
    ....
  }
}

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

Очень похоже на ошибку, но корректного исправления я предложить не могу, т. к. все функции, имеющие в названии "_cell_map" (по аналогии с остальными), уже были использованы в этом операторе сравнения, что делает код ещё более странным.

Случай N15

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 144, 148. heap-controller.cc 148

template <typename Trait>
size_t MemoryController<Trait>::CalculateAllocationLimit(
    Heap* heap, size_t current_size, size_t min_size, size_t max_size,
    size_t new_space_capacity, double factor,
    Heap::HeapGrowingMode growing_mode)
{
  ....
  if (FLAG_heap_growing_percent > 0) {
    factor = 1.0 + FLAG_heap_growing_percent / 100.0;
  }

  if (FLAG_heap_growing_percent > 0) {
    factor = 1.0 + FLAG_heap_growing_percent / 100.0;
  }

  CHECK_LT(1.0, factor);
  ....
}

Ну и напоследок – небольшой пример copy-paste. Лично мне здесь непонятно: то ли просто лишний раз скопировали код, то ли во втором случае нужно что-то поменять. Я думаю, разработчики быстрее меня разберутся в том, что должен делать этот код на самом деле.

Заключение

Ну что ж, мои ожидания от проверки такого огромного проекта полностью оправдались. Хотел интересный проект на проверку – получил :) На самом деле, я очень удивлён тому, насколько качественный код у проекта несмотря на его поистине гигантский размер. Моё уважение разработчикам.

Кстати, возможно, кто-нибудь заметил, но предыдущие статьи цикла были кратно больше по количеству ошибок. Например, в прошлой было 250, а сейчас жалкие 15... Неужели анализатор "сдулся"?

Вовсе нет 😊. Ошибок нашлось много, да и чего уж греха таить, ложных срабатываний тоже набралось достаточно. Только вот... Было бы вам интересно читать эту огромную простыню текста? Думаю, такое времяпрепровождение заинтересовало бы только разработчиков Chromium, и то не факт. Именно поэтому в данной статье я рассказал только о тех ошибках, которые показались интересными мне и моим коллегам. Так сказать, выбрал для вас, читателей, самый сок.

Ну а засим прощаюсь и приглашаю вас обсудить статью в комментариях, а заодно и подискутировать на тему, какой формат статей-проверок нравится вам больше. Чистого вам кода!

#Cpp

Теги:

#Cpp

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Тип желаемой лицензии:

Проверка Chromium спустя три года. Ну и как оно?

Введение

Как проверяли

Ошибки в работе с указателями

Опечатки

Неправильная работа с типами

Любимые сравнения

Always true/false

Всякое-разное

Заключение

Комментарии (0)

Хотите получить ключ на использование анализатора в пробный период?

Достижения

PVS-Studio

Лицензирование

Компания