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

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

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

## Введение

Chromium – это свободный браузер с открытым исходным кодом\. Иногда его ещё называют браузером\-конструктором из\-за того, что он является идеальной базой для создания собственного браузера\. В нём поддерживаются все самые актуальные веб\-технологии, отсутствуют сторонние дополнения и присутствует возможность бесконечной кастомизации\. Разрабатывается компанией Google и сообществом Chromium\. [Официальный репозиторий](https://chromium.googlesource.com/chromium/src)\.

Немногие помнят, но данная статья является уже седьмой нашей проверкой Chromium\. С какой стати столько внимания? Всё просто – данный проект интересен своими размерами и основательным подходом к качеству кода\. Как раз недавно наша команда переводила план по работе над надёжностью, написанный разработчиками Chromium/Chrome\. Ознакомиться с ним можно [здесь](https://habr.com/ru/company/pvs-studio/blog/580762/)\.

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

1. [PVS\-Studio vs Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/a0074/) \(Май 2011\)
1. [PVS\-Studio vs Chromium – продолжение](https://pvs-studio.ru/ru/blog/posts/cpp/0113/) \(Октябрь 2011\)
1. [Третья проверка кода проекта Chromium с помощью анализатора PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/cpp/0205/) \(Август 2013\)
1. [Как мы пытаемся продать PVS\-Studio в Google или очередные ошибки в Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/0225/) \(Декабрь 2013\)
1. [Идём на рекорд: пятая проверка Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/0442/) \(Октябрь 2016\)
1. [Chromium: шестая проверка проекта и 250 багов](https://pvs-studio.ru/ru/blog/posts/cpp/0552/) \(Январь 2018\)

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

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

Недавно мой коллега написал отличную статью о том, как эта "магия" работает – "[Межмодульный анализ C\+\+ проектов в PVS\-Studio](https://habr.com/ru/company/pvs-studio/blog/572304/)"\. Ну а я не вижу смысла пересказывать чужую статью, ведь и в этой материала более чем достаточно :\)

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

В этот раз проверка проводилась на Windows системе с помощью инструмента "C and C\+\+ Compiler Monitoring UI"\. Кратко напомню, что он позволяет отследить все вызовы компилятора при сборке проекта и уже по её окончанию проверить все задействованные файлы\. Для анализа в такой конфигурации необходимо запустить Standalone, а после этого полную сборку проекта\. Более подробно ознакомиться с этим и другими способами проверки проектов можно [в нашей документации](https://pvs-studio.ru/ru/docs/)\.

Что до самой сборки – она прошла без каких\-либо проблем, благо [на официальном сайте](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/windows_build_instructions.md) расположена довольно подробная инструкция\.

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

* все найденные ошибки относятся к этому [состоянию](https://source.chromium.org/chromium/chromium/src/+/009194036844bd47216b62ce1344764ab8511796:) репозитория;
* из проверки были исключены сторонние библиотеки, расположенные в папках _src/buildtools_ и _src/third\_party_, т\. к\. многие из них достойны отдельного разбора\. Один из таких, например, недавно сделал мой коллега – "[Брутальный Protocol Buffers от Google vs статический анализ кода](https://habr.com/en/company/pvs-studio/blog/587486/)"\.
* Фрагменты кода, приведённые в статье, могут незначительно отличаться от тех, что расположены в официальном репозитории\. В некоторых местах изменено форматирование кода для удобства чтения статьи, а где\-то добавлены поясняющие комментарии\.

Ну а теперь, собственно, перейдём к анализу некоторых ошибок, найденных в свежей сборке 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

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

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

```cpp
// 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_ в качестве аргумента происходит в коде, приведённом ниже:

```cpp
// 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 делает какую\-то особую магию с нашим указателем, то боюсь вас огорчить\. Его магия никак не воздействует на аргументы функции :\) Если не верите мне, то можете заглянуть в исходный код макроса по [ссылке](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/wasm/function-body-decoder-impl.h;l=2206;drc=5539ecff898c79b0771340051d62bf81649e448d)\.

**Случай N4**

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

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

```cpp
// 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,_ явно приведёт к неопределённому поведению\. Почему именно к неопределённому поведению, а не падению? Этот вопрос ещё несколько лет назад подробно разобрал мой коллега в статье "[Разыменовывание нулевого указателя приводит к неопределённому поведению](https://pvs-studio.ru/ru/blog/posts/cpp/0306/)"\. Не вижу смысла повторяться, т\. к\. статья там вышла основательная и заслуживает отдельного внимания\.

## Опечатки

**Случай N5**

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

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

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

![0893_chromium_N7_ru/image3.png](https://import.viva64.com/docx/blog/0893_chromium_N7_ru/image3.png)

**Случай 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

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

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

```cpp
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_:

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

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

![0893_chromium_N7_ru/image4.png](https://import.viva64.com/docx/blog/0893_chromium_N7_ru/image4.png)

Чувствуете, чем пахнет? Да это же утечка памяти и неопределённое поведение в одном флаконе\. Где? А в объявлении _unique\_ptr_\! В данном случае объявлен умный указатель на _uint8\_t_, а выше в него пытаются положить массив\. В итоге мало того, что не будет очищена память, занятая элементами массива, так ещё и вызов оператора _delete_ вместо _delete\[\]_ приведёт к [неопределённому поведению](https://pvs-studio.ru/ru/blog/terms/0066/)\!

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

```cpp
std::unique_ptr<uint8_t[]> allocated_;
```

Если вы сомневаетесь в моих словах, то можете почитать, например, draft стандарта С\+\+ 20 [пункт 7\.6\.2\.9\.2](https://timsong-cpp.github.io/cppwp/n4868/expr.delete) \([PDF](http://open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4868.pdf)\) или мой любимый [cppreference\.com](https://en.cppreference.com/w/) в разделе "[delete expression](https://en.cppreference.com/w/cpp/language/delete)"\.

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

**Случай 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

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

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

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

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

Определение корректности такого поведения функции сравнения оставим на суд разработчикам\. А я тем временем хочу посоветовать статью моего коллеги с разбором наиболее распространённых ошибок, встречающихся в функциях сравнения и методах борьбы с ними – "[Зло живёт в функциях сравнения](https://pvs-studio.ru/ru/blog/posts/cpp/0509/)"\.

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

* 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

```cpp
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_, но оно раскрывается в ноль, а следовательно, всегда будет вычисляться, как ложь, и никогда не выполнится\. 

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

```cpp
creation_flags == extensions::Extension::NO_FLAGS
```

**Случай N11**

V547 Expression 'entry\_size \> 0' is always true\. objects\-printer\.cc 1195

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

```cpp
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_ из себя же\. Скорее всего, это опечатка, потому что рядом с этой функцией расположено несколько сильно похожих функций, но уже с другими переменными:

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

```cpp
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 раскрываются в ноль, что не имеет смысла при операции ИЛИ\.

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

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

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

```cpp
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, и то не факт\. Именно поэтому в данной статье я рассказал только о тех ошибках, которые показались интересными мне и моим коллегам\. Так сказать, выбрал для вас, читателей, самый сок\.

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