How developers were checking projects for bugs using PVS-Studio




Pinguem.ru and the PVS-Studio team recently held a contest where programmers were to use PVS-Studio static analyzer for one month to find and fix bugs in the source code of open-source projects. Their efforts have helped make a great many applications a tiny bit safer and more reliable. In this article, we will discuss a few of the most interesting bugs found with the help of PVS-Studio.

Picture 1

So, how did it go?

The contest was held for the Russian-speaking community from October 23 to November 27, 2017, and was split into two stages. At the first stage, the contestants were to submit as many pull requests to the project authors as possible. The second stage was a bit more challenging: they were asked to find a bug and describe the sequence of steps to reproduce it. Nikolay Shalakin was the one who scored the most points and won the contest. Congratulations, Nikolay!

During the contest, the participants submitted a lot of really useful pull requests, all of which are listed here. As for this article, we invite you to take a look at the most interesting bugs found by the contestants at the second stage.

QtCreator

How many of you use QtCreator when coding in Python? Like many other IDEs, it highlights some of the built-in functions and objects. Let's run QtCreator 4.4.1 and write a few keywords:

Picture 3

What's that? Why doesn't it highlight built-in functions oct and chr? Let's look into their code:

// List of python built-in functions and objects
static const QSet<QString> builtins = {
"range", "xrange", "int", "float", "long", "hex", "oct" "chr", "ord",
"len", "abs", "None", "True", "False"
};

The function declarations are fine; what's wrong then? PVS-Studio clarifies the issue:

V653 A suspicious string consisting of two parts is used for initialization. It is possible that a comma is missing. Consider inspecting this literal: "oct" "chr". pythonscanner.cpp 205

Indeed, the programmer forgot to write a comma between literals "oct" and "chr", so the two have merged into one, "octchr", and it is this literal that QtCreator highlights:

Picture 4

The bug-fix pull request can be found here.

ConEmu

Suppose you are working on a ConEmu project and want to check on some of the settings in the debug version (click on the animation to enlarge):

Picture 7

Let's look into the code to find out why we get the "ListBox was not processed" message:

INT_PTR CSetPgViews::OnComboBox(HWND hDlg, WORD nCtrlId, WORD code)
{
  switch (code)
  {
  ....
  case CBN_SELCHANGE:
    {
      ....
      UINT val;
      INT_PTR nSel = SendDlgItemMessage(hDlg, 
                                        nCtrlId, 
                                        CB_GETCURSEL,
                                        0,
                                        0);
      switch (nCtrlId)
      {
        ....
        case tThumbMaxZoom:
          gpSet->ThSet.nMaxZoom = max(100,((nSel+1)*100));
        default:
          _ASSERTE(FALSE && "ListBox was not processed");
      }
    }
  }
}

Because of the missing break statement, the control will pass to the default branch after executing the expressions in the tThumbMaxZoom branch. That's just what PVS-Studio warns us about:

V796 It is possible that 'break' statement is missing in switch statement. setpgviews.cpp 183

The bug-fix pull request can be found here.

UniversalPauseButton

This project is quite an interesting one and is especially useful for gamers. When you click on the Pause key, the program pauses the foreground window's operation:

https://www.youtube.com/watch?v=bItgybhViQM

You can reassign the pause/resume function to another key by tweaking the settings.txt file:

Picture 2

If you enter a key code whose length is no less than 20 characters and no greater than 30 characters, this will result in a stack buffer overflow (click on the animation to enlarge):

Picture 6

Let's find out why it happens. We are interested in function LoadPauseKeyFromSettingsFile:

int LoadPauseKeyFromSettingsFile(_In_ wchar_t* Filename)
{
  HANDLE FileHandle = CreateFile(Filename, 
                                 GENERIC_READ,
                                 FILE_SHARE_READ,
                                 NULL,
                                 OPEN_EXISTING,
                                 FILE_ATTRIBUTE_NORMAL,
                                 NULL);

  if (FileHandle == INVALID_HANDLE_VALUE)
  {
    goto Default;
  }
  
  char  KeyLine[32] = { 0 };
  char  Buffer[2]   = { 0 };
  DWORD ByteRead    = 0;

  do
  {
    if (!ReadFile(FileHandle, Buffer, 1, &ByteRead, NULL))
    {
      goto Default;
    }

    if (Buffer[0] == '\r' || Buffer[0] == '\n')
    {
      break;
    }

    size_t Length = strlen(KeyLine);
    if (Length > 30)                                            // <=
    {
      goto Default;
    }

    KeyLine[Length] = Buffer[0];    
    memset(Buffer, 0, sizeof(Buffer));
  } while (ByteRead == 1);

  if (!StringStartsWith_AI(KeyLine, "KEY="))
  {
    goto Default;
  }

  char KeyNumberAsString[16] = { 0 };                           // <=

  for (DWORD Counter = 4; Counter < strlen(KeyLine); Counter++) // <=
  {
    KeyNumberAsString[Counter - 4] = KeyLine[Counter];
  }
  ....

  Default:
  if (FileHandle != INVALID_HANDLE_VALUE && FileHandle != NULL)
  {
    CloseHandle(FileHandle);    
  }
  return(0x13);
}

In the loop above, the first string is read byte by byte. If its length is greater than 30 characters, control passes to the Default label, releasing the resource and returning character code 0x13. If the string has been read successfully and the first string starts with "KEY=", the substring following the "=" character is copied into 16-byte buffer KeyNumberAsString. Entering a key code from 20 to 30 characters long would result in a buffer overflow. That's just what PVS-Studio warns us about:

V557 Array overrun is possible. The value of 'Counter - 4' index could reach 26. main.cpp 146

The bug-fix pull request can be found here.

Explorer++

The bug found in this project has to do with bookmark sort (click on the animation to enlarge):

Picture 9

Let's examine the code performing the sort:

int CALLBACK SortByName(const NBookmarkHelper::variantBookmark_t
                          BookmarkItem1,
                        const NBookmarkHelper::variantBookmark_t
                          BookmarkItem2)
{
  if (   BookmarkItem1.type() == typeid(CBookmarkFolder)
      && BookmarkItem2.type() == typeid(CBookmarkFolder))
  {
    const CBookmarkFolder &BookmarkFolder1 =
      boost::get<CBookmarkFolder>(BookmarkItem1);
    const CBookmarkFolder &BookmarkFolder2 =
      boost::get<CBookmarkFolder>(BookmarkItem2);

    return BookmarkFolder1.GetName()
           .compare(BookmarkFolder2.GetName());
  }
  else
  {
    const CBookmark &Bookmark1 = 
      boost::get<CBookmark>(BookmarkItem1);
    const CBookmark &Bookmark2 =
      boost::get<CBookmark>(BookmarkItem1);

    return Bookmark1.GetName().compare(Bookmark2.GetName());
  }
}

The programmer made a mistake in the else branch and used BookmarkItem1 twice instead of using BookmarkItem2 in the second case. That's just what PVS-Studio warns us about:

  • V537 Consider reviewing the correctness of 'BookmarkItem1' item's usage. bookmarkhelper.cpp 535
  • another 5 warnings.

The bug-fix pull request can be found here.

Conclusion

The PVS-Studio team is very grateful to all the participants. You did a tremendous job eliminating bugs in open-source projects, making them better, safer, and more reliable. Perhaps someday we'll have a similar contest for the English-speaking community as well.

All the rest are welcome to download and try PVS-Studio analyzer. It is very easy to use and could help you a lot.



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;


Do you make errors in the code?

Check your code
with PVS-Studio

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

goto PVS-Studio;
We use cookies for the analysis of events to improve our content and make user interaction more convenient. By continuing the view of our web-pages you accept the terms of using these files. You can find out more about cookie-files and privacy policy or close the notification, by clicking on the button. Learn More →
Do not show