Стряхиваем пыль с нерегулярной серии статей о проверке проекта Chromium. Посмотрим на качество кода в последнем релизе Chromium и проверим новые возможности анализатора PVS-Studio.
Введение
Chromium — бесплатный браузер с открытым исходным кодом. Иногда его называют конструктором браузера, потому что он является идеальной основой для создания собственного браузера. Он поддерживает новейшие веб-технологии. Chromium не имеет сторонних функций, но имеет бесконечные возможности настройки. Разработано сообществом Chromium и Google. Официальный репозиторий.
Некоторые из вас помнят, что это наша седьмая проверка Chromium. Почему к этому проекту такое внимание? Это просто. Chromium славится своим размером и тщательным подходом к качеству кода. Разработчики даже создали общедоступную документацию, в которой показано, как использовать C++ более безопасным способом. Он называется Безопасное использование C++ и регулярно обновляется. Вы можете прочитать это здесь".
Вот ссылки на предыдущие статьи нашей нерегулярной проверки Chromium:
- PVS-Studio vs Chromium (май 2011 г.)
- PVS-Studio vs Chromium — продолжение (октябрь 2011 г.)
- PVS-Studio против Chromium. 3-я проверка (август 2013 г.)
- Пытаемся продать PVS-Studio Google, или Новые баги в Chromium (декабрь 2013)
- На пути к рекорду: Chromium, 5-я проверка (октябрь 2016 г.)
- Chromium: шестая проверка проекта и 250 багов (январь 2018)
Как видите, последняя проверка была почти три года назад. Chromium все это время развивался, как и PVS-Studio. Сегодня мы протестируем нашу новую функцию — межмодульный анализ — и посмотрим на самые интересные ошибки.
Кстати о межмодульном анализе. Это новая функция нашего анализатора, которая учитывает результаты вызова методов, объявленных в других единицах трансляции. Благодаря этой возможности анализатор распознает поведение функций и переменных, объявленных в других файлах. Анализатор теперь может выдавать предупреждение, например, за разыменование нулевого указателя, переданного в качестве аргумента внешней функции.
Мои товарищи по команде написали отличную статью о том, как работает эта магия — Межмодульный анализ C++ проектов в PVS-Studio. Чужую статью пересказывать смысла нет, в этой материала хоть отбавляй :)
Как мы проверяли
На этот раз мы проверили Chromium в Windows с помощью инструмента C and C++ Compiler Monitoring UI. Этот инструмент отслеживает все вызовы компилятора во время сборки проекта. Когда сборка завершена, инструмент проверяет все задействованные файлы. Для проведения анализа в этой конфигурации запускаем Standalone и после этого — полную сборку проекта. Подробнее об этом и других способах проверки проектов вы можете прочитать в нашей документации.
Сборка прошла без проблем — на официальном сайте есть довольно подробная инструкция.
Пара важных пояснений перед основным текстом:
- все найденные нами ошибки относятся к этому состоянию репозитория;
- мы исключили из проверки сторонние библиотеки, расположенные в папках src/buildtools и src/first_party. Многие из них заслуживают отдельной проверки. Мой товарищ по команде, например, сделал один. Об этом можно прочитать в статье Protocol Buffers, брутальный протокол от Google, vs. PVS-Studio, статический анализатор кода».
- Фрагменты кода в этой статье могут немного отличаться от тех, что в официальном репозитории. Мы изменили форматирование кода в некоторых местах для удобства чтения. Мы также добавили некоторые пояснения в комментариях.
Ну а теперь приступим к ошибкам, которые мы нашли в сборке Chromium.
Ошибки в работе с указателями
Начнем с ошибок, которые мы обнаружили при межмодульном анализе. Отличительной их особенностью является то, что триггеры находятся в разных скомпилированных файлах. Таким образом, эти файлы закомментированы над файлами functions.
Дело N1
V595 Указатель client_ использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: «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 Указатель «родительский» использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: «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 Создание экземпляра WasmFullDecoder ‹ Decoder::kFullValidation, WasmGraphBuildingInterface ›: может иметь место разыменование «результата» нулевого указателя. Нулевой указатель передается в функцию UnOp. Проверьте четвертый аргумент. Отметьте строки: «graph-builder-interface.cc:349», «function-body-decoder-impl.h:5372». интерфейс графа-построителя.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 Может иметь место разыменование нулевого указателя. Нулевой указатель передается в функцию NaClTlsSetCurrentThread. Проверьте первый аргумент. Проверьте строки: «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 Вероятно, внутри оператора for увеличивается неправильная переменная. Подумайте о том, чтобы просмотреть «это». 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 Слева и справа от оператора «&&» одинаковые подвыражения «user_blocking_count_ == 0». 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;
}Разработчик дважды проверял одну и ту же переменную на соответствие 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 Неправильное использование unique_ptr. Память, выделенная с помощью «нового []», будет очищена с помощью «удалить». встроенные-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_;Если вы сомневаетесь в моих словах, то можете прочитать, например, черновик стандарта C++20, параграф 7.6.2.9.2 (PDF). Или можете почитать мой любимый cppreference.com, раздел удалить выражение.
Старые добрые сравнения
Дело N8
V501 Имеются идентичные подвыражения ‘file.MatchesExtension(L”.xlsb”)’ слева и справа от оператора ‘||’. 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 Слева и справа от оператора ‘&&’ находятся одинаковые подвыражения. 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
- зашифрованный_пароль
- имя_пользователя_may_use_prefilled_placeholder
Разработчики должны определить, как будет вести себя эта функция. Кстати, вы можете прочитать статью моего товарища по команде: Зло внутри функций сравнения». О самых распространенных ошибках в функциях сравнения и о том, как их исправить.
Так как есть много других предупреждений, я просто перечислю их:
- V501 Слева и справа от оператора «||» есть идентичные подвыражения «card.record_type() == CreditCard::VIRTUAL_CARD». full_card_request.cc 107
- V501 Слева и справа от оператора «||» есть идентичные подвыражения ‘!event-›target()’. accelerator_filter.cc 28
- V501 Слева и справа от оператора «||» есть идентичные подвыражения «generation_id-›empty()». record_handler_impl.cc 393
- V501 Слева и справа от оператора «&&» есть идентичные подвыражения «JSStorNamedNode::ObjectIndex() == 0». js-native-context-specialization.cc 1102
- V501 Слева и справа от оператора «&&» есть идентичные подвыражения «num_previous_succeeded_connections_ == 0». websocket_throttler.cc 63
Всегда верно/ложно
Дело N10
V616 В побитовой операции используется именованная константа ‘extensions::Extension::NO_FLAGS’ со значением 0. 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 Выражение ‘entry_size › 0’ всегда верно. объекты-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 Слева и справа от оператора «-» есть идентичные подвыражения «StandardFrameConstants::kCallerPCOffset». связь.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. Первый аргумент всегда равен нулю. Ведь при вычислении первого аргумента переменная 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 Значение переменной flags не изменяется. Рассмотрите возможность проверки выражения. Возможно, вместо «0» должна стоять «1». usb_device_handle_win.cc 58
V684 Значение переменной flags не изменяется. Рассмотрите возможность проверки выражения. Возможно, вместо «0» должна стоять «1». 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 Обнаружено использование шаблона «if (A) {…} else if (A) {…}». Существует вероятность наличия логической ошибки. Контрольные линии: 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 Условные выражения операторов if, расположенных рядом друг с другом, идентичны. Проверьте строки: 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); .... }
И напоследок — небольшой пример копипаста. Я не совсем понимаю этот фрагмент кода. Либо просто еще раз скопировали код, либо во втором случае нужно что-то менять. Думаю, разработчики быстро разберутся, для чего нужен этот фрагмент кода.
Заключение
Что ж, мои ожидания от такой огромной проверки проекта оправдались. Я хотел проверить интересный проект, и я его получил :) На самом деле, я удивлен качеством кода такого гигантского проекта. Моё почтение разработчикам.
Кто-то, наверное, заметил, что в предыдущих статьях было гораздо больше ошибок. Например, в последнем было 250. В этом 15… Анализатор не сработал?
Совсем нет😊! Было много ошибок и, если честно, много ложных срабатываний. Вопрос в том, интересно ли вам читать стену текста? Я думаю, что это будет интересно прочитать только разработчикам Chromium. Именно поэтому в этой статье я перечислил только самые интересные ошибки. Все самое лучшее для моих читателей.
Это пока все! Не стесняйтесь обсуждать эту статью в комментариях. И дайте нам знать, как вам нравятся статьи о проверке проекта. Чистого кода вам!
