Top 10 bugs found in C++ projects 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);
}
....
}
....
}

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;
}
str << "</ul></body></html>";

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 << Aint 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);
}
Define PM_EXP2(A) (1 << (A))

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

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

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.

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

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.

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

3rd: misinterpreted error in the date processing function

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

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

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

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:

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

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.

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

610 Followers

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.