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

01 Nov 2021

Author: Vladislav Stolyarov

Introduction
Example of an error
Conclusion

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.

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.

#Cpp