To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
How a PVS-Studio developer defended a b…

How a PVS-Studio developer defended a bug in a checked project

Nov 01 2021

The PVS-Studio developers often check open-source projects and write articles about that. Sometimes, when writing an article, we come across interesting situations or epic errors. Of course, we want to write a small note about it. This is one of those cases.

0880_one_case_bug/image1.png

Introduction

At the moment I'm writing an article about checking the DuckStation project. This is an emulator of the Sony PlayStation console. The project is quite interesting and actively developing. I found some interesting bugs and want to share a story about one with you. This article demonstrates:

  • that even experts can make mistakes.
  • that static analysis may save a person from making such mistakes.

Example of an error

PVS-Studio has issued a warning: V726 An attempt to free memory containing the 'wbuf' array by using the 'free' function. This is incorrect as 'wbuf' was created on stack. log.cpp 216

template<typename T>
static ALWAYS_INLINE void FormatLogMessageAndPrintW(....)
{
  ....
  wchar_t wbuf[512];
  wchar_t* wmessage_buf = wbuf;
  ....
  if (wmessage_buf != wbuf)
  {
    std::free(wbuf); // <=
  }
  if (message_buf != buf)
  {
    std::free(message_buf);
  }
  ....
}

In the original version of the article, I described this bug the following way:

Here the analyzer detected code with an error. In this code fragment, we see an attempt to delete an array allocated on the stack. Since the memory has not been allocated on the heap, you don't need to call any special functions like std::free to clear it. When the object is destroyed, the memory is cleared automatically.

This may seem like a great error for an article — static buffer and dynamic memory release. What could have gone wrong? I'll tell you now.

In our company, a developer writes an article and gives it to a more experienced teammate. They review the article and give recommendations on how to improve it. This case is no exception. Look at the comment the reviewer left after he read my article:

There's no error here. This is a false alarm; the analyzer has not mastered it. In the middle, there's a dynamic memory allocation for the message by the malloc function. The if (wmessage_buf != wbuf) check is used to determine whether to call std::free or not.

You're probably wondering what malloc is and where it came from. My bad. It's time to fix it. Take a look at the function's entire code. Above, I have already shown you this code fragment when describing the error. The reviewer inspected the same fragment when reading the article.

template<typename T>
static ALWAYS_INLINE void FormatLogMessageAndPrintW(
                                             const char* channelName, 
                                             const char* functionName, 
                                             LOGLEVEL level,
                                             const char* message, 
                                             bool timestamp, 
                                             bool ansi_color_code,
                                             bool newline, 
                                             const T& callback)
{
  char buf[512];
  char* message_buf = buf;
  int message_len;
  if ((message_len = FormatLogMessageForDisplay(message_buf,
                       sizeof(buf), channelName, functionName, level, 
                       message, timestamp,
                       ansi_color_code, newline)) > (sizeof(buf) - 1))
  {
    message_buf = static_cast<char*>(std::malloc(message_len + 1));
    message_len = FormatLogMessageForDisplay(message_buf, 
                 message_len + 1, channelName, functionName, 
                 level, message, timestamp, ansi_color_code, newline);
  }
  if (message_len <= 0)
    return;

  // Convert to UTF-16 first so unicode characters display correctly.
  // NT is going to do it anyway...
  wchar_t wbuf[512];
  wchar_t* wmessage_buf = wbuf;
  int wmessage_buflen = countof(wbuf) - 1;
  if (message_len >= countof(wbuf))
  {
    wmessage_buflen = message_len;
    wmessage_buf = static_cast<wchar_t*>
               (std::malloc((wmessage_buflen + 1) * sizeof(wchar_t)));
  }

  wmessage_buflen = MultiByteToWideChar(CP_UTF8, 0, message_buf,
                    message_len, wmessage_buf, wmessage_buflen);

  if (wmessage_buflen <= 0)
    return;

  wmessage_buf[wmessage_buflen] = '\0';
  callback(wmessage_buf, wmessage_buflen);

  if (wmessage_buf != wbuf)
  {
    std::free(wbuf);                        // <=
  }

  if (message_buf != buf)
  {
    std::free(message_buf);
  }
}

Indeed, if the message length is greater than or equal to countof(wbuf), a new buffer on the heap will be created for it.

You may think that this fragment looks a lot like false alarm. However, I looked at the code from the function for a minute and responded the following way:

Strongly disagree. Let's look at the code: the buffer on the stack, dynamic allocation of the new buffer on the heap, releasing the wrong buffer.

If the string doesn't fit into the local buffer on the stack, then we put it in a dynamically allocated buffer at the wmessage_buf pointer. As you see from the code, below there are 2 branches with memory release, if there was a dynamic allocation. We can check it with wmessage_buf != wbuf. However, in the first branch the wrong memory is released. That's why the warning is here. In the second branch the right buffer is released. No warnings here.

Indeed, there's an error. The developer should have cleared the wmessage_buf the same way as they did below.

My teammate's response was short:

Agree. I was wrong.

P.S. I owe you a beer.

Conclusion

Unfortunately, every static analyzer issues false positives. Because of this, developers question some warnings and take them as false positives. My advice: don't rush and be attentive when you inspect warnings.

By the way, you can read similar entertaining articles. For example:

Enjoy your reading. Come and try PVS-Studio on your projects.

Popular related articles
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept