PVS-Studio vs Chromium. 3-rd Check

Andrey Karpov
Articles: 326



The Chromium browser is developing very fast. When we checked the solution for the first time in 2011, it included 473 projects. Now it includes 1169 projects. We were curious to know if Google developers had managed to keep the highest quality of their code with Chromium developing at such a fast rate. Well, they had.

Chromium

Chromium is an open-source web browser developed by the Google company. It is used as a basis for the Google Chrome browser. Visit the "Get the Code" page for information on downloading the solution source codes.

Some General Information

We checked Chromium earlier and reported those checks in two articles: the first check (23.05.2011), the second check (13.10.2011). Each time we managed to find a number of errors - this is a subtle hint about the usefulness of code analyzers.

Currently (we downloaded the source codes in July 2013) Chromium consists of 1169 projects. The total size of the C/C++ source code is 260 Mbytes plus 450 Mbytes more of the third-party libraries.

If you study our first analysis-report for Chromium of 2011, you will notice that the size of the third-party libraries has not changed much since that. But the size of the project code itself has grown from 155 Mbytes to 260 Mbytes.

Calculating the Cyclomatic Complexity Just from Curiosity

The PVS-Studio analyzer is capable of searching for functions with big cyclomatic complexity values. These functions are usually the first candidates to be refactored. Having checked 1160 projects, I was naturally curious to find out which of them held the record for having "the most complex function".

In Chromium, the function ValidateChunkAMD64() has the highest cyclomatic complexity (2782). However, we had to disqualify it from the contest because it is located in the validator_x86_64.c file which is an autogenerated one. That's a pity: it could have been an epic record-holder. I have never seen such a large cyclomatic complexity value in my life.

Thus, the first three places go to the following functions:

  • The WebKit library. The HTMLTokenizer::nextToken() function in the file htmltokenizer.cpp. Cyclomatic complexity is 1106.
  • The Mesa library. The _mesa_glsl_lex() function in the file glsl_lexer.cc. Cyclomatic complexity is 1088.
  • The usrsctplib library (this player is unknown). The sctp_setopt() function in the file htmltokenizer.cpp. Cyclomatic complexity is 1026.

If you have never come across cyclomatic complexity of 1000, you'd better never have to for your psychic health's sake :). It's just too much, you know.

Code Quality

What can be said about the quality of the Chromium project's code? It is perfect as always. There are some bugs indeed, just as in any other large project; but if you calculate their density (by dividing their number by the code size) you'll see that it is very trifling. This is a very good code with pretty few bugs. So, we award a medal to the Chromium developers for their clear code. The previous medal was awarded to the Casablanca (C++ REST SDK) project by Microsoft.

Figure 1. A medal for the Chromium developers.

Figure 1. A medal for the Chromium developers.

Along with the project code itself, I also checked the third-party libraries used by Chromium. However, describing errors found in them is not very interesting, especially considering that I just glanced through the report very quickly. You may think I'm a mean guy, but I'm not. I'd like to watch you studying carefully the analysis report for all the 1169 projects. The bugs I did notice were added into our bug database. This article describes only those errors that I found in Chromium itself (its plugins and so on).

The Chromium project being so perfect, what for should I describe its bugs at all? It's simple: I want to show you how powerful the PVS-Studio analyzer is. Since it has managed to catch some bugs in Chromium with its fine code, it is surely worth your attention.

The analyzer chewed up dozens of thousands of files with the total size 710 Mbytes and still survived. Although the Chromium project is being developed by highly skilled developers and checked by various verifying tools, PVS-Studio still managed to catch some defects. And that's an awesome achievement! And the last thing: it took it a reasonable time (about 5 hours) to complete the analysis, as the check ran in parallel (AMD FX-8320/3.50 GHz/eight-core processor, 16.0 GB RAM).

Selected Examples of Detected Bugs

I invite you to study selected code samples that caught my glance when looking through the analysis report. I'm sure that a more thorough examination will have much more interesting results.

Noticed Bugs No. 1 - Misprints

Vector3dF
Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const
{
  // The matrix must be symmetric.
  const float epsilon = std::numeric_limits<float>::epsilon();
  if (std::abs(data_[M01] - data_[M10]) > epsilon ||
      std::abs(data_[M02] - data_[M02]) > epsilon ||
      std::abs(data_[M12] - data_[M21]) > epsilon) {
    NOTREACHED();
    return Vector3dF();
  }
  ....
}

V501 There are identical sub-expressions to the left and to the right of the '-' operator: data_[M02] - data_[M02] matrix3_f.cc 128

We need to check that a 3x3 matrix is symmetric.

Figure 2. 3x3 matrix.

Figure 2. 3x3 matrix.

To do that we should compare the following items:

  • M01 and M10
  • M02 and M20
  • M12 and M21

The code must have been written with the use of the Copy-Paste technology, which resulted in M02 cell being compared to itself. That's a funny matrix class.

Another plain misprint:

bool IsTextField(const FormFieldData& field) {
  return
    field.form_control_type == "text" ||
    field.form_control_type == "search" ||
    field.form_control_type == "tel" ||
    field.form_control_type == "url" ||
    field.form_control_type == "email" ||
    field.form_control_type == "text";
}

V501 There are identical sub-expressions 'field.form_control_type == "text"' to the left and to the right of the '||' operator. autocomplete_history_manager.cc 35

A comparison to the "text" string is executed twice, which is strange. One of these lines is not necessary or there must be some other comparison instead.

Noticed Bugs No. 2 - Opposite Conditions

static void ParseRequestCookieLine(
    const std::string& header_value,
    ParsedRequestCookies* parsed_cookies)
{
  std::string::const_iterator i = header_value.begin();
  ....
  if (*i == '"') {
    while (i != header_value.end() && *i != '"') ++i;
  ....
}

V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 500, 501. web_request_api_helpers.cc 500

I guess this code was intended to skip a text framed by double quotes. But it actually does nothing, as the condition is false right away. I've written a small pseudo-code fragment to clarify the point:

if ( A == 'X' ) {
  while ( .... && A != 'X' ) ....;

The programmer must have forgotten to move the pointer by one character, so the fixed code should look like this:

if (*i == '"') {
  ++i;
  while (i != header_value.end() && *i != '"') ++i;

Noticed Bugs No. 3 - Unsuccessfully Removed Items

void ShortcutsProvider::DeleteMatchesWithURLs(
  const std::set<GURL>& urls)
{
  std::remove_if(matches_.begin(),
                 matches_.end(),
                 RemoveMatchPredicate(urls));
  listener_->OnProviderUpdate(true);
}

V530 The return value of function 'remove_if' is required to be utilized. shortcuts_provider.cc 136

To remove items from the container the function std::remove_if() is used, yet it is used incorrectly. The function remove_if() actually removes nothing; it only shifts items to the beginning and moves the iterator back to garbage which you need to remove manually by calling the erase() function of the container. See also the Wikipedia-article "Erase-remove idiom" for details.

The fixed code:

matches_.erase(std::remove_if(.....), matches_.end());

Noticed Bugs No. 4 - This Eternal Mess-up with SOCKET

SOCKET in the Linux world is an integer SIGNED data type.

SOCKET in the Windows world is an integer UNSIGNED data type.

In Visual C++ header files, the SOCKET type is declared in this way:

typedef UINT_PTR SOCKET;

But programmers are constantly forgetting this and keep writing code like this:

class NET_EXPORT_PRIVATE TCPServerSocketWin {
   ....
   SOCKET socket_;
   ....
};

int TCPServerSocketWin::Listen(....) {
  ....
  socket_ = socket(address.GetSockAddrFamily(),
                   SOCK_STREAM, IPPROTO_TCP);
  if (socket_ < 0) {
    PLOG(ERROR) << "socket() returned an error";
    return MapSystemError(WSAGetLastError());
  }
  ....
}

V547 Expression 'socket_ < 0' is always false. Unsigned type value is never < 0. tcp_server_socket_win.cc 48

An unsigned variable is always above or equal to zero. It means that the 'socket_ < 0' check is meaningless. If the socket fails to be opened while the program is running, this situation will be handled incorrectly.

Noticed Bugs No. 5 - Mess-up with operations ~ and !

enum FontStyle {
  NORMAL = 0,
  BOLD = 1,
  ITALIC = 2,
  UNDERLINE = 4,
};

void LabelButton::SetIsDefault(bool is_default) {
  ....
  style = is_default ? style | gfx::Font::BOLD :
                       style & !gfx::Font::BOLD;
  ....
}

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. label_button.cc 131

I guess the code was intended to work in the following way:

  • If the 'is_default' variable is true, the bit responsible for the BOLD type must always be set to 1.
  • If the 'is_default' variable is false, the bit responsible for the BOLD type must always be set to 0.

The expression "style & !gfx::Font::BOLD", however, works quite differently than the programmer expects. The result of the "!gfx::Font::BOLD" operation will be 'false', i.e. 0. The code above is equivalent to this one:

style = is_default ? style | gfx::Font::BOLD : 0;

For it to work correctly the '~' operation must be used:

style = is_default ? style | gfx::Font::BOLD :
                     style & ~gfx::Font::BOLD;

Noticed Bugs No. 6 - Temporary Objects Created in a Strange Way

base::win::ScopedComPtr<IDirect3DSurface9> 
  scaler_scratch_surfaces_[2];

bool AcceleratedSurfaceTransformer::ResizeBilinear(
  IDirect3DSurface9* src_surface, ....)
{
  ....
  IDirect3DSurface9* read_buffer = (i == 0) ?
    src_surface : scaler_scratch_surfaces_[read_buffer_index];
  ....
}

V623 Consider inspecting the '?:' operator. A temporary object of the 'ScopedComPtr' type is being created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391

This code will hardly cause any bugs, but it is worth discussing: I suppose some programmers will discover a new C++ trap they will find interesting.

It's all simple at first sight: depending on the condition, either the 'src_surface' pointer or one of the 'scaler_scratch_surfaces_' array's items is chosen. The array is comprised by objects of the base::win::ScopedComPtr<IDirect3DSurface9> type which can be automatically cast to the pointer to IDirect3DSurface9.

The devil is in the details.

The ternary operator '?:' cannot return different types depending on the conditions. Here is a simple example to explain the point.

int A = 1;
auto X = v ? A : 2.0;

The ?: operator returns the 'double' type. Because of that, the 'X' variable will also be double. But it's not the point. The point is that the 'A' variable will be implicitly extended to the 'double' type!

The trouble occurs if you write a thing like this:

CString s1(L"1");
wchar_t s2[] = L"2";
bool a = false;
const wchar_t *s = a ? s1 : s2;

Execution of this code fragment results in the 's' variable referring to data inside a temporary object of the CString type. The problem is that this object will be immediately destroyed.

Now let's go back to Chromium's source code.

IDirect3DSurface9* read_buffer = (i == 0) ?
    src_surface : scaler_scratch_surfaces_[read_buffer_index];

If the 'i == 0' condition is true, the next thing occurs:

  • the pointer 'src_surface' is used to create a temporary object of the base::win::ScopedComPtr<IDirect3DSurface9> type;
  • the temporary object is implicitly cast to the pointer of the IDirect3DSurface9 type and put into the read_buffer variable;
  • the temporary object is destroyed.

I'm not familiar with the logic of the program and the ScopedComPtr class and I can't tell for sure if any negative consequences will occur. The most probable thing is that the counter of the reference number will be incremented in the constructor and decremented in the destructor. So, everything will be OK.

If not, you risk getting a non-valid pointer or broken reference counter.

So, even if there is no error in this particular case, I will be glad if anyone of the readers has learned something new. As you can see, ternary operators are much more dangerous than one may think.

Here is one more suspicious fragment like the previous one:

typedef
  GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;

DWORD HandlePolicy::DuplicateHandleProxyAction(....)
{
  ....
  base::win::ScopedHandle remote_target_process;
  ....
  HANDLE target_process =
    remote_target_process.IsValid() ?
      remote_target_process : ::GetCurrentProcess();
  ....
}

V623 Consider inspecting the '?:' operator. A temporary object of the 'GenericScopedHandle' type is being created and subsequently destroyed. Check third operand. handle_policy.cc 81

Noticed Bugs No. 7 - Repeating Checks

string16 GetAccessString(HandleType handle_type,
                         ACCESS_MASK access) {
  ....
  if (access & FILE_WRITE_ATTRIBUTES)
    output.append(ASCIIToUTF16("\tFILE_WRITE_ATTRIBUTES\n"));
  if (access & FILE_WRITE_DATA)
    output.append(ASCIIToUTF16("\tFILE_WRITE_DATA\n"));
  if (access & FILE_WRITE_EA)
    output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
  if (access & FILE_WRITE_EA)
    output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
  ....
}

V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 176, 178. handle_enumerator_win.cc 178

If the flag FILE_WRITE_EA is enabled, the string "\tFILE_WRITE_EA\n" will be added twice. That's very strange.

A similar strange thing happens in the following fragment as well:

static bool PasswordFormComparator(const PasswordForm& pf1,
                                   const PasswordForm& pf2) {
  if (pf1.submit_element < pf2.submit_element)
    return true;
  if (pf1.username_element < pf2.username_element)
    return true;
  if (pf1.username_value < pf2.username_value)
    return true;
  if (pf1.username_value < pf2.username_value)
    return true;
  if (pf1.password_element < pf2.password_element)
    return true;
  if (pf1.password_value < pf2.password_value)
    return true;

  return false;
}

V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261

The check "pf1.username_value < pf2.username_value" is executed twice. Perhaps one string is just not needed, but it's also probable that the programmer wanted to check something else and some different condition is missing here.

Noticed Bugs No. 8 - One-Time Loops

ResourceProvider::ResourceId
PictureLayerImpl::ContentsResourceId() const
{
  ....
  for (PictureLayerTilingSet::CoverageIterator iter(....);
       iter;
       ++iter)
  {
    if (!*iter)
      return 0;

    const ManagedTileState::TileVersion& tile_version = ....;

    if (....)
      return 0;

    if (iter.geometry_rect() != content_rect)
      return 0;

    return tile_version.get_resource_id();
  }
  return 0;
}

V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638

Something is not right with this loop: it iterates only once. There is the unconditional operator return at the end of the loop, which may be due to the following reasons:

  • That was just the idea, which I doubt. What for did the programmer need to create a loop, an iterator and so on?
  • One of the 'return's must be replaced with 'continue'. But that's hardly as well.
  • Most likely, some condition is missing before the last 'return'.

There are some other strange loops iterating only once:

scoped_ptr<ActionInfo> ActionInfo::Load(....)
{
  ....
  for (base::ListValue::const_iterator iter = icons->begin();
        iter != icons->end(); ++iter)
  {
    std::string path;
    if (....);
      return scoped_ptr<ActionInfo>();
    }

    result->default_icon.Add(....);
    break;
  }
  ....
}

V612 An unconditional 'break' within a loop. action_info.cc 76

const BluetoothServiceRecord* BluetoothDeviceWin::GetServiceRecord(
    const std::string& uuid) const
{
  for (ServiceRecordList::const_iterator iter =
         service_record_list_.begin();
       iter != service_record_list_.end();
       ++iter)
  {
    return *iter;
  }
  return NULL;
}

V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224

Noticed Bugs No. 9 - Uninitialized Variables

HRESULT IEEventSink::Attach(IWebBrowser2* browser) {
  DCHECK(browser);
  HRESULT result;
  if (browser) {
    web_browser2_ = browser;
    FindIEProcessId();
    result = DispEventAdvise(web_browser2_, &DIID_DWebBrowserEvents2);
  }
  return result;
}

V614 Potentially uninitialized variable 'result' used. ie_event_sink.cc 240

If the pointer 'browser' equals zero, the function will return an uninitialized variable.

Another code fragment:

void SavePackage::GetSaveInfo() {
  ....
  bool skip_dir_check;
  ....
  if (....) {
    ....->GetSaveDir(...., &skip_dir_check);
  }
  ....
  BrowserThread::PostTask(BrowserThread::FILE,
                          FROM_HERE,
                          base::Bind(..., skip_dir_check, ...));
}

V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth actual argument of the 'Bind' function. save_package.cc 1326

The variable 'skip_dir_check' may remain uninitialized.

Noticed Bugs No. 10 - Code Alignment Does Not Correspond to Program Logic

void OnTraceNotification(int notification) {
  if (notification & TraceLog::EVENT_WATCH_NOTIFICATION)
    ++event_watch_notification_;
    notifications_received_ |= notification;
}

V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. trace_event_unittest.cc 57

When examining this code, you cannot understand whether or not curly brackets are missing here. Even if it is correct, it should be changed a bit so that it doesn't confuse other programmers.

Here is a couple of fragments with a TOO strange code alignment:

  • nss_memio.c 152
  • nss_memio.c 184

Noticed Bugs No. 11 - Checking a Pointer after New

Many programs contain legacy code written in those old times when the 'new' operator did not throw an exception in case of memory shortage. It used to return a null pointer instead.

Chromium is no exception in that aspect - it also has such checks. The trouble is not that these checks are meaningless but that returning a null pointer implied performing some actions or returning certain values by functions. Now the program logic is different because of the practice of exception generation: the code that was given control in case of a memory allocation error now stays idle.

Have a look at this example:

static base::DictionaryValue* GetDictValueStats(
    const webrtc::StatsReport& report)
{
  ....
  DictionaryValue* dict = new base::DictionaryValue();
  if (!dict)
    return NULL;

  dict->SetDouble("timestamp", report.timestamp);

  base::ListValue* values = new base::ListValue();
  if (!values) {
    delete dict;
    return NULL;
  }
  ....
}

V668 There is no sense in testing the 'dict' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164

V668 There is no sense in testing the 'values' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169

The first check "if (!dict) return NULL;" doesn't seem harmful. But the second check is not safe. If memory fails to be allocated when the "new base::ListValue()" function creates an object, the exception 'std::bad_alloc' will be thrown and the GetDictValueStats() function will terminate.

As a result, this code:

if (!values) {
  delete dict;
  return NULL;
}

will never destroy the object whose address is stored in the 'dict' variable.

To fix the code we need to refactor it and use smart pointers.

Examine another code fragment:

bool Target::Init() {
{
  ....
  ctx_ = new uint8_t[abi_->GetContextSize()];

  if (NULL == ctx_) {
    Destroy();
    return false;
  }
  ....
}

V668 There is no sense in testing the 'ctx_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73

In case of a memory allocation error, the function Destroy() won't be called.

I find this type of bugs not very much interesting to write about any further. Instead, I give you a list of other fragments of that kind I've noticed:

  • 'data' pointer. target.cc 109
  • 'page_data' pointer. mock_printer.cc 229
  • 'module' pointer. pepper_entrypoints.cc 39
  • 'c_protocols' pointer. websocket.cc 44
  • 'type_enum' pointer. pin_base_win.cc 96
  • 'pin_enum' pointer. filter_base_win.cc 75
  • 'port_data'. port_monitor.cc 388
  • 'xcv_data' pointer. port_monitor.cc 552
  • 'monitor_data'. port_monitor.cc 625
  • 'sender_' pointer. crash_service.cc 221
  • 'cache' pointer. crash_cache.cc 269
  • 'current_browser' pointer. print_preview_dialog_controller.cc 403
  • 'udp_socket' pointer. network_stats.cc 212
  • 'popup_' pointer. try_chrome_dialog_view.cc 90

Noticed Bugs No. 12 - Bad Tests

Unit tests are a wonderful method of software quality enhancement. But tests themselves often have errors, which results in their failure. Making tests for tests is just too much; so, static code analysis will be of use in these cases. I discussed this idea in more detail in the article "How to complement TDD with static analysis".

Below are some examples of errors I have found in tests for Chromium:

std::string TestAudioConfig::TestValidConfigs() {
  ....
  static const uint32_t kRequestFrameCounts[] = {
    PP_AUDIOMINSAMPLEFRAMECOUNT,
    PP_AUDIOMAXSAMPLEFRAMECOUNT,
    1024,
    2048,
    4096
  };
  ....
  for (size_t j = 0;
    j < sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts);
    j++) {
  ....
}

V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)' to the left and to the right of the '/' operator. test_audio_config.cc 56

Only one test is executed in the loop. The error is this: "sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts)" equals one. The correct expression is "sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts[0])".

Another incorrect test:

void DiskCacheEntryTest::ExternalSyncIOBackground(....) {
  ....
  scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
  scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize2));
  ....
  EXPECT_EQ(0, memcmp(buffer2->data(), buffer2->data(), 10000));
  ....
}

V549 The first argument of 'memcmp' function is equal to the second argument. entry_unittest.cc 393

The function "memcmp()" compares the buffer to itself. This results in the test failing to perform the necessary check. I guess the code should look like this:

EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000));

And the next test is the one that may unexpectedly spoil the other tests:

static const int kNumPainters = 3;

static const struct {
  const char* name;
  GPUPainter* painter;
} painters[] = {
  { "CPU CSC + GPU Render", new CPUColorPainter() },
  { "GPU CSC/Render", new GPUColorWithLuminancePainter() },
};

int main(int argc, char** argv) {
  ....
  // Run GPU painter tests.
  for (int i = 0; i < kNumPainters; i++) {
    scoped_ptr<GPUPainter> painter(painters[i].painter);
  ....  
}

V557 Array overrun is possible. The value of 'i' index could reach 2. shader_bench.cc 152

The 'painters' array perhaps used to consist of three items earlier. Now it has only two, but the value of the 'kNumPainters' constant is still 3.

Here is a list of some other incorrect code fragments in tests which I find worth considering:

V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790

V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800

V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810

V595 The 'browser' pointer was utilized before it was verified against nullptr. Check lines: 5489, 5493. testing_automation_provider.cc 5489

V595 The 'waiting_for_.get()' pointer was utilized before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205

V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34

V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16

V595 The 'textfield_view_' pointer was utilized before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182

V595 The 'message_loop_' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53

Noticed Bugs No. 13 - Variadic Function

In any program many defects are found in code fragments responsible for handling errors and reacting to incorrect input data. This is due to the fact that these fragments are difficult to test, so they are usually not. Because of this, programs start behaving in a very intricate way, which was not planned by the programmer.

For example:

DWORD GetLastError(VOID);

void TryOpenFile(wchar_t *path, FILE *output) {
  wchar_t path_expanded[MAX_PATH] = {0};
  DWORD size = ::ExpandEnvironmentStrings(
    path, path_expanded, MAX_PATH - 1);
  if (!size) {
    fprintf(output,
            "[ERROR] Cannot expand \"%S\". Error %S.\r\n",
            path, ::GetLastError());
  }
  ....
}

V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of wchar_t type symbols is expected. fs.cc 17

If the variable 'size' equals zero, the program tries to write a text message into a file. But this message will most probably contain some abracadabra at the end. Moreover, this code may cause an access violation.

Writing is performed by the function fprintf() which cannot control the types of its arguments. It expects that the last argument should be a pointer to a string. But the actual argument is a number (error code) which will be converted into an address, and no one can predict how the program will behave after that.

Unnoticed Bugs

I remind you once again that I only looked through the list of warning messages and described in this article only what caught my attention. What's more, I've shown just a part of what I've found. If I described all those bugs, the article would become too big. And it's already big enough.

I decided not to mention many code fragments which I found of no interest to the readers. Here you are a couple of examples to explain what I mean.

bool ManagedUserService::UserMayLoad(
  const extensions::Extension* extension,
  string16* error) const
{
  if (extension_service &&
      extension_service->GetInstalledExtension(extension->id()))
    return true;

  if (extension) {
    bool was_installed_by_default =
      extension->was_installed_by_default();
    .....
  }
}

V595 The 'extension' pointer was utilized before it was verified against nullptr. Check lines: 277, 280. managed_user_service.cc 277

The pointer 'extension' gets dereferenced in the "extension->id()" expression in the beginning. After that it is being checked for being a null pointer.

Such code fragments are usually harmless, for the pointer simply cannot be equal to zero, so the check is meaningless. That's why I find it unreasonable to mention these fragments because I may be mistaken and confuse a correct code for an incorrect one.

This is one more example of a diagnostic I preferred not to notice:

bool WebMClusterParser::ParseBlock(....)
{
  int timecode = buf[1] << 8 | buf[2];
  ....
  if (timecode & 0x8000)
    timecode |= (-1 << 16);
  ....
}

V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. webm_cluster_parser.cc 217

Formally, a shift of a negative value leads to undefined behavior. But many compilers work stably and behave the way the programmer expects them to. It results in the code working well and long, though it shouldn't. I don't feel like fighting these troubles now, so I'll better skip such messages. Those of you who want to study the issue in detail, see the article "Wade not in unknown waters - part three".

About False Positives

I am often asked the question:

You do it very cleverly citing samples of detected bugs in your articles without telling the total number of warning messages produced by the tool. It's a usual thing with static analyzers to generate too many false positives so that one has hard time trying to pick out real errors among them. How many false positives does PVS-Studio generate?

I always hesitate to answer. You see, I have two opposite answers: the first is "many", the second is "few". It all depends on how you approach to viewing the list of warning messages. I will try to explain this duality by the example of Chromium.

The PVS-Studio analyzer has generated 3582 first-level warnings (the set of General Analysis rules) for this project. This number is very big. Most of them are false positives. If you attack them frontally and start examining each item of the list, you'll get bored very soon. Your impression of the tool will be awful: almost all the warnings are false positives looking very alike. Nothing interesting. The tool is bad.

The typical mistake of a user who thinks like that is that he has not performed even minimum customization of the tool. Yes, we try to make PVS-Studio such a tool that can be efficiently run immediately after the setup. We try to make it in such a way that you don't have to customize anything; you just check your project and study the list of warnings.

But sometimes it can't be done. That was the case with Chromium. It was the macro 'DVLOG' that triggered so many false positives. This macro is responsible for logging something and is written in such a tricky way that PVS-Studio believes it to have a bug. Since the macro is used very often in the program, I got quite many false positives. In fact, the number of false warnings in the analysis report coincides with the number of times the DVLOG macro is used; namely, it triggered about 2300 false positives under the "V501 There are identical sub-expressions....." diagnostic.

You can suppress these warnings by adding the comment //-V:DVLOG:501 in the header file opposite to the macro declaration.

This simple operation lets us subtract 2300 false positives from the total number of messages (3528). We have at one instant eliminated 65% of messages. Now we don't have to waste time examining them.

It won't take you much time and effort to perform some more subtle customizations like that. As a result, most of the false positives will be removed from the report. Customizing some diagnostics requires analysis relaunch, while others don't. All this is described in detail in the documentation section "Suppression of false alarms". In particular, analysis relaunch is required for diagnostics detecting errors in macros.

I hope you now understand why I have two opposite answers. It all depends on whether or not the programmer is ready to spend just a bit of his time studying the product and ways to get rid of irrelevant messages.

The Final Word to the Readers

Taking the opportunity, I want to send my best regards to my parents... Oh, sorry, I meant: taking the opportunity, I want to send my best regards to programmers and remind them a few things:

  • The answer to the question "Did you inform the developers about the errors found in their project?" can be found in the post "FAQ for those who have read our articles".
  • The best way to contact us and ask any questions you want is through the feedback form on our website. Please don't use twitter for that purpose as well as comments to our articles on third-party sites and so on.
  • I invite you to follow us in twitter: @Code_Analysis. I regularly collect and post various links to interesting materials in the area of programming and the C++ language.


Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Andrey Karpov
Articles: 326


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;