It's freezing outside, everyone has already decorated the Christmas tree and bought tangerines. New Year is coming! So, it's time to meet the Top 10 interesting bugs found by the PVS-Studio C++ analyzer in 2021.

10th: pretty typo in a loop

V533 [CWE-691] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. gskvulkanimage.c 721

void
gsk_vulkan_image_upload_regions (GskVulkanImage    *self,
                                 GskVulkanUploader *uploader,
                                 guint              num_regions,
                                 GskImageRegion    *regions)
{
  ....
  for (int i = 0; i < num_regions; i++)
  {
    m = mem + offset;
    if (regions[i].stride == regions[i].width * 4)
    {
      memcpy (m, regions[i].data, regions[i].stride * regions[i].height);
    }
    else
    {
      for (gsize r = 0; r < regions[i].height; i++)          // <=
        memcpy (m + r * regions[i].width * 4,
                regions[i].data + r * regions[i].stride, regions[i].width * 4);
    }
    ....
  }
  ....
}

Note that in a nested loop the i variable is incremented instead of the r variable. No need to comment. It's a golden classic!

My colleague described this error in the "Finding typos in the GTK 4 project by PVS-Studio" article.

9th: suddenly, let's talk about... HTML!

V735 Possibly an incorrect HTML. The "</body>" closing tag was encountered, while the "</ul>" tag was expected. qpixeltool.cpp 707

QString QPixelTool::aboutText() const
{
  const QList<QScreen *> screens = QGuiApplication::screens();
  const QScreen *windowScreen = windowHandle()->screen();

  QString result;
  QTextStream str(&result);
  str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR
    << "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";
  for (const QScreen *screen : screens)
    str << "<li>" << (screen == windowScreen ? "* " : "  ")
        << screen << "</li>";
  str << "<ul></body></html>";
  return result;
}

PVS-Studio provides diagnostics that don't just check code – they also look for abnormalities in string literals. The code above triggered one of these diagnostics. Such cases are quite rare. That's why, this one is so intriguing.

Someone intended to create one list but added two tags that open this list instead of one. This is clearly a typo. The first tag must open the list, and the second one must close it. Here is the correct code:

str << "</ul></body></html>";

This error was described in the article: "Date processing attracts bugs or 77 defects in Qt 6".

8th: an unsafe macro

V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. bug34427.c 160

#define PM_EXP2(A) 1 << A

int process_val(const u_int8_t *data, u_int32_t data_len,
                               u_int32_t *retvalue, ....) 
{
  *retvalue = 0;
  ....
  /* Now find the actual value */
  for (; i < data_len; i++) {
    *retvalue += data[i] * PM_EXP2(8 * (data_len - i - 1));
  }
  return(0);
}

The analyzer warns that after the macro expands, it may produce an incorrect expression. The function will first multiply the variable by one, and then conduct the bitwise shift to the expression in parentheses. It was a lucky coincidence that in this line the x * 1 << y expression is equal to x * (1 << y). If to its left or right the macro has /, %, +, -, or other operations with a priority greater than <<, or if the macro contains an operation that has a lesser priority than <<, the expression will not be evaluated correctly. Always wrap the macro and its arguments in parentheses to avoid problems in the future. The following is correct:

Define PM_EXP2(A) (1 << (A))

You can find this error in the "PVS-Studio analyzer scans Snort, network traffic scanner" article.

7th: mixed-up divisor and dividend

V1064 The 'gPEClockFrequencyInfo.bus_clock_rate_hz' operand of integer division is less than the 'gPEClockFrequencyInfo.dec_clock_rate_hz' one. The result will always be zero. pe_identify_machine.c 72

void
pe_identify_machine(__unused boot_args *args)
{
  ....
  // Start with default values.
  gPEClockFrequencyInfo.timebase_frequency_hz = 1000000000;
  gPEClockFrequencyInfo.bus_frequency_hz      =  100000000;
  ....
  gPEClockFrequencyInfo.dec_clock_rate_hz = 
    gPEClockFrequencyInfo.timebase_frequency_hz;
  gPEClockFrequencyInfo.bus_clock_rate_hz =
   gPEClockFrequencyInfo.bus_frequency_hz;
  .... 
  gPEClockFrequencyInfo.bus_to_dec_rate_den =
    gPEClockFrequencyInfo.bus_clock_rate_hz /
    gPEClockFrequencyInfo.dec_clock_rate_hz;
}

All fields used here have integer types:

extern clock_frequency_info_t gPEClockFrequencyInfo;

struct clock_frequency_info_t {
  unsigned long bus_clock_rate_hz;
  unsigned long dec_clock_rate_hz;
  unsigned long bus_to_dec_rate_den;
  unsigned long long bus_frequency_hz;
  unsigned long timebase_frequency_hz;
  ....
};

Through intermediate assignments, the dividend data member gPEClockFrequencyInfo.bus_clock_rate_hz, is assigned the 100000000 value, and the divisor data member gPEClockFrequencyInfo.dec_clock_rate_hz is assigned the 1000000000 value. In this case, the divisor is ten times greater than the dividend. Since all the data members here are integers, the gPEClockFrequencyInfo.bus_to_dec_rate_den data member is 0.

Judging by the name of the resulting bus_to_dec_rate_den data member, the divisor and the dividend are mixed up.

My colleague described this error in the following article: "MacOS Kernel, how good is this apple?".

6th: error in selecting types

V610 Undefined behavior. Check the shift operator '>>='. The right operand ('bitpos % 64' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. master.c 354

// bitsperlong.h
#ifdef CONFIG_64BIT
#define BITS_PER_LONG 64
#else
#define BITS_PER_LONG 32
#endif /* CONFIG_64BIT */

// bits.h
/*
 * Create a contiguous bitmask starting at bit position @l and ending at
 * position @h. For example
 * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
 */
#define __GENMASK(h, l) ....

// master.h
#define I2C_MAX_ADDR      GENMASK(6, 0)

// master.c
static enum i3c_addr_slot_status
i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
{
  int status, bitpos = addr * 2;                   // <=

  if (addr > I2C_MAX_ADDR)
    return I3C_ADDR_SLOT_RSVD;

  status = bus->addrslots[bitpos / BITS_PER_LONG];
  status >>= bitpos % BITS_PER_LONG;               // <=

  return status & I3C_ADDR_SLOT_STATUS_MASK;
}

Note that the BITS_PER_LONG macro can be 64-bit.

The code contains undefined behavior:

after the check, the addr variable can be in the range [0..127]
if the formal parameter is addr >= 16, then the status variable is right-shifted by a number of bits more than the int type contains (32 bits).

Perhaps, the author wanted to reduce the number of lines and declared the bitpos variable next to the status variable. However, the programmer did not take into account that int has a 32-bit size on 64-bit platforms, unlike the long type.

To fix this, declare the status variable with the long type.

You can read about this error in the "Linux kernel turns 30: congratulations from PVS-Studio" article. Do read the article, if you haven't already. You'll find nice old-school pictures :)

5th: intermodular analysis and lost memset

This year, the PVS-Studio developers added one major and necessary feature – intermodular analysis of C++ projects. Intermodular analysis helped to find this warning in the codelite project.

V597 The compiler could delete the 'memset' function call, which is used to flush 'current' object. The memset_s() function should be used to erase the private data. args.c 269

// args.c
extern void eFree (void *const ptr);

extern void argDelete (Arguments* const current)
{
  Assert (current != NULL);
  if (current->type ==  ARG_STRING  &&  current->item != NULL)
    eFree (current->item);
  memset (current, 0, sizeof (Arguments));  // <=
  eFree (current);                          // <=
}

// routines.c
extern void eFree (void *const ptr)
{
  Assert (ptr != NULL);
  free (ptr);
}

LTO (Link Time Optimization) may optimize out the memset call. The compiler, using the as-if rule, figures out that eFree doesn't calculate any useful pointer-related data. eFree only calls the free function that frees memory.

Without LTO, the eFree call looks like an unknown external function, so memset will remain.

This error was described in the following article: "Intermodular analysis of C++ projects in PVS-Studio".

4th: senseless check && Unreal Engine

Recently, PVS-Studio enhanced the check of Unreal Engine projects. My colleague wrote an article, describing this in detail – you'll find the link below. First, let's take a look at an interesting error that the analyzer found.

V547 Expression 'm_trail == 0' is always false. unpack.hpp 699

std::size_t m_trail; 
....
inline int context::execute(const char* data, std::size_t len,
 std::size_t& off)
{
  ....
  case MSGPACK_CS_EXT_8: {
                uint8_t tmp;
                load<uint8_t>(tmp, n);
                m_trail = tmp + 1;
                if(m_trail == 0) {
                    unpack_ext(m_user, n, m_trail, obj);
                    int ret = push_proc(obj, off);
                    if (ret != 0) return ret;
                }
                else {
                    m_cs = MSGPACK_ACS_EXT_VALUE;
                    fixed_trail_again = true;
                }
            } break;
  ....
}

Let's see what's going on in this code fragment.

We have the tmp variable of the uint8_t type. Its value is limited with eight bits – [0, 255]. The programmer who wrote this code assumes that tmp can be 255. After the m_trail = tmp + 1 assignment, they check that there's no integer overflow. An unsigned integer arithmetic can cause a wrap around. So, the result of the tmp + 1 operation can be 0.

However, the analyzer says that the m_trail == 0 check is always false. Let's figure it out.

At first, we need to recall std::common_type. By the way, we discussed it in our recent article.

Let's consider the line. It contains the addition operation. For binary operations between values of different types, the compiler uses usual arithmetic conversions during which integral promotion is applied to the tmp variable. Its type in this expression is expanded to the type of the 1 literal, that is, to int. As a result, even if the tmp value is 255, the addition operation results in 256. The int type stores this value. So, the m_trail == 0 check is senseless.

My colleague described this error in the "How the Carla car simulator helped us level up the static analysis of Unreal Engine 4 projects" article.

3rd: misinterpreted error in the date processing function

In this case, the PVS-Studio analyzer issued a whole bunch of warnings:

V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4907
V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4911
V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4913
V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4921

First, let's take a look at the function that accepts a month's abbreviated name and returns its number.

static const char qt_shortMonthNames[][4] = {
    "Jan", "Feb", "Mar", "Apr", "May", "Jun",
    "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
};

static int fromShortMonthName(QStringView monthName)
{
  for (unsigned int i = 0;
       i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)
  {
    if (monthName == QLatin1String(qt_shortMonthNames[i], 3))
      return i + 1;
  }
  return -1;
}

If the operation succeeds, the function returns the month number (a value from 1 to 12). If the month's name is incorrect, the function returns a negative value (-1). Note that the function cannot return 0.

However, the function above is used where the developer expects it to return zero value in case of an error. Here's the code fragment with the incorrect fromShortMonthName function:

QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format)
{
  ....
  month = fromShortMonthName(parts.at(1));
  if (month)
    day = parts.at(2).toInt(&ok);

  // If failed, try day then month
  if (!ok || !month || !day) {
    month = fromShortMonthName(parts.at(2));
    if (month) {
      QStringView dayPart = parts.at(1);
      if (dayPart.endsWith(u'.'))
        day = dayPart.chopped(1).toInt(&ok);
    }
  }
  ....
}

The program never reaches the code that checks the month number for zero value. It continues to run with an incorrect negative month number.

This error was discussed in the "Date processing attracts bugs or 77 defects in Qt 6" article.

2nd: an example of inattention

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);
  }
  ....
}

This code fragment triggered the analyzer. An attempt to delete an array allocated on the stack provokes an error. The memory has not been allocated on the heap. So, there's no need to call any special functions like std::free to clear it. When the object is destroyed, the memory is cleared automatically.

To my mind, the origin of this warning is more thrilling than the warning itself. I don't want to spoil the story, so, I invite you to read the original article: "How a PVS-Studio developer defended a bug in a checked project".

This is not the only bug we found in this project. To see the full list of curious warnings, read the following article: "PVS-Studio searches for bugs in the DuckStation project".

1st: an example of greater inattention

We always warn our users of making errors. But this year, we made the error. We are not afraid to talk about this. Such cases prove that a static analyzer is much more attentive than a programmer. Here's an example:

V645 The 'strncat' function call could lead to the 'a.consoleText' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold.

struct A
{
  char consoleText[512];
};

void foo(A a)
{
  char inputBuffer[1024];
  ....
  strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) –
                                      strlen(a.consoleText) - 5);
  ....
}

At first glance, the code fragment seems to be correct. We are safe from undefined behavior. However, let's take a closer look at an expression:

sizeof(a.consoleText) – strlen(a.consoleText) – 5

The expression may receive a negative number! For example, such may happen if strlen(a.consoleText) = 508. In this case, an unsigned integer overflow happens. The expression results in the maximum value of the resulting type – size_t.

This error was described in the following article: "One day in the life of PVS-Studio developer, or how I debugged diagnostic that surpassed three programmers".

Conclusion

This year, we checked lots of C++ projects. We could even write several reviews about top bugs. If you have your Top 10 bugs list, feel free to share them in the comments after reading articles from our blog.

Every year we write New Year's articles about Top 10 bugs. I invite you to read articles about Top 10 bugs in C++ projects for the past years: 2016, 2017, 2018, 2019, 2020.

Top 10 bugs found in C++ projects in 2021