Kirill Yudintsev

Jul 13 2016

Tags:

#Cpp

Checking the Source Code of Nana Library with PVS-Studio

Jul 13 2016

Author: Kirill Yudintsev

Introduction
Typos in conditions
Using a null pointer (not a bug here, but code like that is always worth checking)
Incorrect use of a smart pointer
Redundant comparison
On dangerous uses of pointers one more time
Using the SuspendThread() function
Conclusion

With the appearance of new C++ standards, C++ developers started to move to the new programming style, known as Modern C++, and projects that make use of the new style started to appear. Static code analyzers have to keep up to date to be able to detect errors in modern C++ code, which holds true for PVS-Studio as well. With Nana project as a test sample, we'll try to find out if PVS-Studio analyzer can cope with modern C++.

Introduction

To begin with, I'd like to say a few words about the project. Nana is a cross-platform C++11 library for creating graphical user interfaces. The library is small - 74 KLOC. It supports Windows and Linux (X11) platforms and provides experimental macOS support. Nana is an open-source software product distributed under the Boost Software License. We took version 1.3.0 for the check; its source code can be downloaded here: https://sourceforge.net/projects/nanapro/files/latest/download.

Typos in conditions

Typos in conditional statements are rather common, and Nana has a few of those too, for example:

V501 There are identical sub-expressions 'fgcolor.invisible()' to the left and to the right of the '&&' operator. text_editor.cpp 1316

void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && fgcolor.invisible())  // <=
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}

The analyzer detected two identical conditional expressions. This code was very likely written using Copy-Paste: the programmer copied the fgcolor.invisible() expression but forgot to change the fgcolor variable's name. Judging by the function's parameters, the programmer wanted to use argument bgcolor instead of fgcolor in the second subexpression. In that case, the fixed code should look like this:

void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && bgcolor.invisible())
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}

There's almost no way we can do without Copy-Paste when writing code, so you just have to be more careful when inspecting copy-pasted and amended code. If you have still made a mistake because of copy-paste, static analysis could help save time when tracking it down.

Using a null pointer (not a bug here, but code like that is always worth checking)

The analyzer detected a code fragment where a null pointer is used.

V522 Dereferencing of the null pointer 'debug' might take place. text_token_stream.hpp 669

This is what it looks like:

void parse(....)
{
  ....
  switch(tk)
  {
    ....
    default:
      int * debug = 0;  //for debug.
      *debug = 0;
  }
  ....
}

As the comment suggests, this trick with the debug pointer was done for debugging purposes, but I still felt I should cite this fragment as an example. You must be very careful with things like that, as program logic might change later and you'd be unpleasantly surprised. Anyway, this code should be re-checked just in case.

Incorrect use of a smart pointer

We have finally got to an issue related to the C++11 standard. The analyzer detected a situation where using a smart pointer might cause undefined behavior, in particular damaging the heap or a program crash. The reason is that the programmer uses different methods to allocate and free memory.

V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. text_editor.cpp 3137

void text_editor::_m_draw_string(....) const
{
  ....
  for (auto & ent : reordered)
  {
    ....
    std::size_t len = ent.end - ent.begin;
    ....
    if (....)
    {
      ....
    }
    else if (pos <= a.x && a.x < str_end)
    {
      ....
      std::unique_ptr<unsigned> pxbuf_ptr(new unsigned[len]);  // <=
    }
  }
}

The unique_ptr [RU] class is used to manage the memory block allocated for the array. When freeing that block, it uses the delete operator by default, which results in undefined behavior. To fix this error, we need to use partial specialization of unique_ptr for the array. In that case, memory will be freed by calling the delete[] operator. This is what the fixed code should look like:

std::unique_ptr<unsigned[]> pxbuf_ptr(new unsigned[len]);

Redundant comparison

Sometimes conditional statements use redundant checks, which may contain potential errors. The analyzer detected a redundant comparison in the code and issued the following warning:

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. window_manager.cpp 467

void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if((wd->other.category == category::root_tag::value) ||
     (wd->other.category != category::frame_tag::value))  // <=
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}

Here's a simple example to explain the point:

if (a == 1 || a != 5)

The condition will execute if a != 5. The first part of the expression makes no sense. If you examine the code closely, you will come to one of the following conclusions: either the expression should be simplified by removing the first part - the code would then look like this:

if (a != 5)

or there is an error in the expression, in which case it should be fixed like this:

if (a == 1 || not_a_var != 5)

In our example, the first situation is more probable, so it should be simplified in the following way:

void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if(wd->other.category != category::frame_tag::value)
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}

On dangerous uses of pointers one more time

You must be especially careful when using pointers in C/C++. If you are not sure there are any data found at the address pointed to by a pointer, make sure you verify it against null. Accessing memory pointed by a null pointer results in undefined behavior or a program crash.

V595 The 'owner' pointer was utilized before it was verified against nullptr. Check lines: 299, 315. window_manager.cpp 299

window_manager::core_window_t*
window_manager::create_root(core_window_t* owner,    )
{
  ....
  if (nested)
  {
    wd->owner = nullptr;
    wd->parent = owner;
    wd->index = static_cast<unsigned>(owner->children.size());
    owner->children.push_back(wd);  // <=
  }
  ....
  if (owner 
      && owner->other.category == category::frame_tag::value)  // <=
    insert_frame(owner, wd);
  ....
}

V595 is perhaps the most common warning across all the projects we check. Here's another similar issue found in this one:

V595 The 'wd' pointer was utilized before it was verified against nullptr. Check lines: 1066, 1083. window_manager.cpp 1066

Using the SuspendThread() function

The analyzer detected a code fragment where the SuspendThread() function is used:

V720 It is advised to utilize the 'SuspendThread' function only when developing a debugger (see documentation for details). pool.cpp 225

void _m_suspend(pool_throbj* pto)
{
  pto->thr_state = state::idle;
#if defined(NANA_WINDOWS)
  ::SuspendThread(pto->handle);  // <=
#elif defined(NANA_POSIX)
  std::unique_lock<std::mutex> lock(pto->wait_mutex);
  pto->suspended = true;
  pto->wait_cond.wait(lock);
  pto->suspended = false;
#endif
}

A call to this function is not an error in itself; however, developers often use it inappropriately, which might result in unexpected behavior. The SuspendThread() function is meant to help developers create debuggers and other similar utilities. If you use it for synching purposes, you are very likely to get a bug.

For more information on misuses of the SuspendThread() function, see the following articles:

Conclusion

Nana is a small project and there aren't many errors in it. However, some of the fragments do need to be checked. Among the errors found, there is one related to the use of the C++11 standard. One error is, of course, not enough to estimate PVS-Studio's capabilities for analyzing C++11 projects, so we would be glad to receive any suggestions from you. If you know any projects written in modern C++, please let us know, and we'll try to check them. Use the feedback form to contact us.

To sum up, I'd like to warn you against limiting yourself to tests and code review when writing program code. Using static analysis helps write better code and save time when searching for bugs. So, you are welcome to try PVS-Studio on your projects written in C, C++, or C#.

#Cpp