We dust off the irregular series of articles about the Chromium project check. Let's look at the code quality in the latest Chromium release and check the new features of the PVS-Studio analyzer.

chromium_N7/image2.png

Introduction

Chromium is a free open-source browser. Sometimes it's called a browser constructor because it is a perfect base for creating your own browser. It supports the latest web technologies. Chromium doesn't have third-party features but has infinite customization options. Developed by the Chromium community and Google. Official repository.

Some of you remember that it's our seventh Chromium check. Why does this project have so much attention? It's simple. Chromium is famous for its size and a thorough approach to the code quality. The developers even created a public documentation where they show how to use C++ in a safer way. It's called "Safer Usage of C++" and it's regularly updated. You can read it here.

Here are the links to the previous articles of our irregular Chromium check:

PVS-Studio vs Chromium (May 2011)
PVS-Studio vs Chromium — Continuation (October 2011)
PVS-Studio vs Chromium. 3-rd Check (August 2013)
Trying to Sell PVS-Studio to Google, or New Bugs in Chromium (December 2013)
Heading for a Record: Chromium, the 5th Check (October 2016)
Chromium: the Sixth Project Check and 250 Bugs (January 2018)

As you see, the last check was almost three years ago. Chromium has been evolving all this time, same as PVS-Studio. Today we'll test our new feature — intermodular analysis — and look at the most interesting errors.

Speaking of intermodular analysis. It's a new feature of our analyzer which takes into account the results of calling methods declared in other translation units. With this feature, the analyzer recognizes the behavior of functions and variables declared in other files. The analyzer now can issue a warning, for example, for dereferencing of a null pointer passed as an argument to an external function.

My teammates wrote an excellent article about how this "magic" works — "Intermodular analysis of C++ projects in PVS-Studio". There's no point in retelling someone else's article, this one has more than enough material :)

How we checked

This time we checked Chromium on Windows with the "C and C++ Compiler Monitoring UI" tool. This tool tracks all compiler calls during the project build. When the build is completed, the tool checks all the files involved. To perform analysis in this configuration, we run Standalone and after that — a complete project build. You can read more about this and other ways of checking projects in our documentation.

The build completed without any problems — the official website has pretty detailed instructions.

A couple of important explanations before the main text:

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.

Well, now let's proceed to the errors we found in the Chromium build.

Errors in working with pointers

Let's start with the errors we found with intermodular analysis. Their distinctive feature is that the triggers are located in different compiled files. Thus, these files are commented above the functions.

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

Here the analyzer has detected an unsafe call of the IsLoggingActive function. As a result of this call, the function can obtain a null pointer as an argument and then it dereferences a null pointer without any check. Why did the analyzer consider this call unsafe? If you look at the code below, you'll see that this pointer is checked. The further actions depend on the state of this check.

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

The same situation as above: we pass the pointer to the function where it's dereferenced without any check. Besides, a pointer is passed to the function and only then it's checked. If the parent pointer shouldn't be null, why was it checked below? Definitely suspicious code, the developers should check it.

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

Here the analyzer detected the dereference of the null result pointer in the UnOp function. The UnOp call with nullptr as an argument takes place in the following fragment:

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

If you think that the CALL_INTERFACE_IF_OK_AND_REACHABLE macro does some magic with our pointer, I have bad news for you. Its magic doesn't affect the function's arguments :) If you don't believe me, you can look at the macro's source code here.

Case N4

chromium_N7/image3.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

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

An obvious error. Here the null pointer is passed to the function where it's dereferenced later. Judging by the nearby comment, NULL is passed deliberately. However, if we call the expression used in the NaClTlsSetCurrentThread, this will result in undefined behavior. Why undefined behavior and not the application crash? My teammate answered this question several years ago in article "Null pointer dereferencing causes undefined behavior". Since this article thoroughly describes such a situation, I see no point in repeating it here.

Typos

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

Hmm… The iterator of the external loop is incremented in the nested loop… I think I have a picture for that...

chromium_N7/image4.png

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

The developer checked the same variable for compliance with 0 twice. Weird, right? I think we should look into the class to which this function belongs:

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

Well, everything's clear now. In the second case, the developer should have used the user_visible_count variable which is located next to user_blocking_count:

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

Incorrect work with types

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

chromium_N7/image5.png

Do you feel it? It's the memory leak and undefined behavior mixed together. Where? In the unique_ptr declaration! In this case, a smart pointer to uint8_t is declared. Besides, above it, the developer tries to put an array in it. As a result, the memory occupied by the array elements is not cleared. Besides, if we call the delete operator instead of delete[], it leads to undefined behavior!

To fix the problem, we need to replace the declaration string with the following one:

std::unique_ptr<uint8_t[]> allocated_;

If you doubt my words, you can read, for example, the draft of the C++20 standard, paragraph 7.6.2.9.2 (PDF). Or you can read my favorite cppreference.com, section "delete expression".

Good old comparisons

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

Many developers say: if you format code as a table, it helps avoid repeating similar-looking elements. As you can see, it's not enough. We can improve the situation by simply sorting the entries. Try to find errors in the code below (yes, there are more than one). I'll even remove the error markers.

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

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

I think formatting code as a table won't help here :) Only high-quality refactoring. By the way, after simple manipulations with a text editor and Python we found out that the comparison operator doesn't check the following class fields:

accepts_webauthn_credentials
new_password_element_renderer_id
server_side_classification_successful
encrypted_password
username_may_use_prefilled_placeholder

It's up to developers to determine how this function would behave. By the way, you can read my teammate's article: "The evil within the comparison functions". It's about the most common errors found in comparison functions and how to fix them.

Since there are lots of other warnings, I'll simply list them:

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

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

In this code fragment I want you to pay attention to the first expression of the conditional operator. In this expression, a bitwise multiplication with extensions::Extension::NO_FLAGS takes place. However, it expands to zero, and therefore it will always be false. It will never be executed.

Most likely, the first check should have been written like this:

creation_flags == extensions::Extension::NO_FLAGS

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

A small example of the DataFlow mechanism's work.

The analyzer says that the value of the entry_size variable is always greater than zero. Therefore, the code that checks the variable is always executed. How did the analyzer know the result of calculating the variable? It just calculated the range of possible values of the variable after executing the FeedbackMetadataIterator::entry_size and FeedbackMetadata::GetSlotSize functions.

Miscellaneous

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

The ForSavedCallerReturnAddress function calls the ForCalleeFrameSlot function inside itself. The first argument is always zero. After all, when calculating the first argument, the kCallerPCOffset variable is subtracted from itself. Most likely, this is a typo. Next to this function there are several very similar functions, but with different variables:

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

Case 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 and BMREQUEST_STANDARD expand to zero, which doesn't make sense with the OR operation.

At first, I thought that the values of these macros were defined differently in different files. However, when I ran through the source folder, I found their only definition:

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

Honestly, I'm not sure if it's an error, but it still worth the developers' attention.

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

Here the if operator has the same condition for two different code branches. This leads to the fact that if it is true, the code from the overlying branch will always be called.

It looks very much like an error, but I can't offer a correct fix. All the functions that have "_cell_map" in the name (by analogy with the others) have already been used in this comparison operator. This makes the code even more strange.

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

And finally — a small example of a copy-paste. I don't quite understand this code fragment. Either they just copied the code once again, or something needs to be changed in the second case. I think developers would quickly figure out what this code fragment was supposed to do.

Conclusion

Well, my expectations from such a huge project check were justified. I wanted an interesting project to check, and I got it :) Actually, I'm surprised by the code quality of such a giant project. My respect to the developers.

Someone has probably noticed that previous articles contained a lot more errors. For example, the last one contained 250. This one contains 15… Has the analyzer failed?

Not at all?! There was a lot of errors and, to be honest, a lot of false positives. The question is...Would you be interested in reading a wall of text? I think only the Chromium developers would be interested in reading this. That's why in this article I listed only the most interesting errors. All the good stuff for my fellow readers.

That's all for now! Feel free to discuss this article in the comments. And let us know how you like the project check articles. Clean code to you!

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