Checking Chromium after three years. How’s it going?

Introduction

How we checked

  • all errors we found relate to this repository state;
  • we excluded from the check the third-party libraries located in the src/buildtools and src/third_party folders. Many of them deserve a separate check. My teammate, for example, made one. You can read about it in article “Protocol Buffers, a brutal protocol from Google, vs. PVS-Studio, a static code analyzer”.
  • The code fragments in this article may slightly differ from the ones in the official repository. We changed the code formatting in some places for readability. We also added some explanations in the commentaries.

Errors in working with pointers

// 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()) { // <=
....
}
}
// 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
}
// 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());
}
// 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);
}
....
}
// 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);
....
}

Typos

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;
}
}
....
}
....
}
}
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;
};
return user_blocking_count_ == 0 && user_visible_count_ == 0;

Incorrect work with types

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_; // <=
}
std::unique_ptr<uint8_t[]> allocated_;

Good old comparisons

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

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,
....
};
creation_flags == extensions::Extension::NO_FLAGS
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;
}

Miscellaneous

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());
}
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());
}
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;
}
#define BMREQUEST_HOST_TO_DEVICE 0
....
#define BMREQUEST_STANDARD 0
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;
}
....
}
}
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);
....
}

Conclusion

--

--

--

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.

Love podcasts or audiobooks? Learn on the go with our new app.

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.

More from Medium

MuditaOS: Will your alarm clock go off? Part I

Securing open source require better programming languages

Why Learn Elixir

Photo by Negative Space from Pexels https://www.pexels.com/@negativespace Blue, white, purple, and yellow computer code on a black screen background.

How PVS-Studio prevents rash code changes, example N3