На улице мороз, ёлка наряжена, мандарины разложены. Дело идёт к Новому году, а значит — время рассмотреть самые интересные срабатывания, найденные C++ анализатором PVS-Studio в 2021 году.
Десятое место: красивая опечатка в цикле
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);
}
....
}
....
}
Обратите внимание, что во вложенном цикле инкрементируется не переменная r, а i. Что-то дополнительно пояснять здесь не требуется. Это золотая классика!
Эта ошибка вошла в топ из статьи: "Выявляем опечатки в проекте GTK 4 с помощью PVS-Studio".
Девятое место: неожиданно мы поговорим об 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 есть диагностики, которые не только проверяют сам код, но и выискивают аномалии в строковых константах. Здесь как раз сработала одна из таких диагностик. Это достаточно редкий случай, зато этим он и примечателен.
Дважды используется тег открытия списка. Это явная опечатка. Первый тег должен открывать список, а второй – закрывать. Правильный код:
str << "</ul></body></html>";Эта ошибка вошла в топ из статьи: "Обработка дат притягивает ошибки, или 77 дефектов в Qt 6".
Восьмое место: Опасный макрос
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);
}
Анализатор предупреждает, что после подстановки макроса результирующее выражение может вычисляться некорректно. Сначала будет выполнено умножение на 1, а уже затем побитовый сдвиг на выражение в скобках. Повезло, что в данной строке выражение x * 1 << y эквивалентно x * (1 << y). Если слева или справа от макроса будет стоять /,* %, +, -* или другая операция с приоритетом большим, чем у <<, либо внутри макроса будет стоять операция с приоритетом меньшим, чем у <<, выражение будет вычисляться неправильно. Всегда следует оборачивать макрос и его аргументы скобками, чтобы избежать проблем в будущем. Так, правильным будет:
Define PM_EXP2(A) (1 << (A))Эта ошибка вошла в топ из статьи: "Сканер сетевого трафика Snort под сканером анализатора кода PVS-Studio".
Седьмое место: перепутаны делитель и делимое
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;
....
};
Через промежуточные присвоения полю gPEClockFrequencyInfo.bus_clock_rate_hz, являющемуся делимым, присваивается значение 100000000, а полю-делителю gPEClockFrequencyInfo.dec_clock_rate_hz присваивается значение 1000000000. Делитель в этом случае в десять раз больше делимого. Так как все поля здесь являются целочисленными, поле gPEClockFrequencyInfo.bus_to_dec_rate_den окажется равным 0.
Судя по наименованию результирующего поля bus_to_dec_rate_den, делитель и делимое были перепутаны местами.
Эта ошибка также вошла в топ из статьи: "Ядро macOS, есть ли червячки в этом яблоке".
Шестое место: ошибка в выборе типов
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;
}
Обратите внимание, что макрос BITS_PER_LONG может иметь значение 64.
В коде содержится неопределенное поведение:
после проверки переменная addr может быть в диапазоне [0..127]
если формальный параметр addr >= 16, то переменная status будет сдвинута вправо на число бит больше, чем вмещает тип int (32 бита).
Вероятно, автор хотел сократить количество строк и сделал объявление переменной bitpos рядом с переменной status. Однако он не учёл, что int имеет 32-битный размер на 64-битных платформах, в отличие от типа long.
Для исправления следует объявить переменную status с типом long.
Эта ошибка вошла в топ из статьи: "30 лет ядру Linux: поздравление от PVS-Studio". Если вы её ещё не читали – советую обратить внимание, в ней есть классные исторические картинки :)
Пятое место: межмодульный анализ и потерявшийся memset
В этом году в PVS-Studio появилась одна большая и нужная фича – поддержка межмодульного анализа С++ проектов. Данное срабатывание на проекте codelite было найдено как раз с его помощью.
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);
}
Вызов memset может быть удалён при использовании LTO (Link Time Optimization). Компилятор, пользуясь правилом as-if, видит, что eFree не делает никаких полезных вычислений с данными по указателю, а лишь вызывает функцию free, которая освобождает память.
Без LTO вызов eFree выглядит как неизвестная внешняя функция, поэтому memset останется.
Эта ошибка вошла в топ из статьи: "Межмодульный анализ С++ проектов в PVS-Studio".
Четвертое место: бессмысленная проверка && Unreal Engine
Недавно PVS-Studio начал ещё лучше проверять Unreal Engine проекты. Подробно об этом рассказал мой коллега, ссылка на статью которого будет ниже. Сначала же давайте посмотрим на интересную ошибку, которую ему удалось найти.
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;
....
}
Давайте попробуем подробно разобраться, что происходит в данном фрагменте кода.
У нас есть переменная tmp, тип которой uint8_t. Диапазон её значений ограничен восьмью битами - [0, 255]. Программист, который писал этот код, предполагает, что значение tmp может быть равно 255. После присваивания m_trail = tmp + 1 он проверяет, что не случилось целочисленного переполнения. Арифметика переполнения целочисленной беззнаковой переменной циклична по модулю, и результатом операции tmp + 1 может быть 0.
Однако анализатор говорит о том, что проверка m_trail == 0 всегда false. Давайте разбираться.
Сейчас нам пригодятся правила вывода общего типа, про которые, кстати, рассказывается в нашей недавней статье.
Перейдём к рассматриваемой строчке. В ней выполняется операция сложения. При бинарных операциях между значениями разных типов компилятор применяет usual arithmetic conversions, в ходе которых на переменную tmp применится integral promotion, и её тип в этом выражении расширится до типа литерала 1, то есть до int. В итоге, даже если значением tmp будет 255, в ходе операции сложения результирующее значение будет 256. Тип int спокойно вмещает это значение. А значит, проверка m_trail == 0 и правда бессмысленна.
Эта ошибка вошла в топ из статьи: "Прокачка статического анализа проектов на Unreal Engine 4 и проверка автосимулятора Carla".
Третье место: неправильная обработка ошибки в функции обработки даты
В данном случае анализатор PVS-Studio выдал целый букет срабатываний:
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
Для начала нам следует посмотреть, как устроена функция, возвращающая номер месяца по его сокращённому названию:
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;
}
В случае успеха функция возвращает номер месяца (значение от 1 до 12). Если имя месяца некорректно, то функция возвращает отрицательное значение (-1). Обратите внимание, что функция не может вернуть значение 0.
Но там, где только что рассмотренная функция используется, программист как раз рассчитывает, что в качестве статуса ошибки ему будет возвращено нулевое значение. Фрагмент кода, где некорректно используется функция fromShortMonthName:
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);
}
}
....
}
Проверки номера месяца на равенство нулю никогда не сработают, и программа продолжит выполняться с некорректным отрицательным номером месяца.
Эта ошибка также вошла в топ из статьи: "Обработка дат притягивает ошибки, или 77 дефектов в Qt 6".
Второе место: показательный пример невнимательности
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);
}
....
}
Здесь анализатор обнаружил ошибочный код, в котором осуществляется попытка удаления массива, выделенного на стеке. Так как память не была выделена на куче, не стоит также и вызывать каких-либо специальных функций, таких как std::free для её очистки – она будет произведена автоматически при уничтожении объекта.
На мой взгляд, интереснее, чем данное срабатывание – его история. Не буду спойлерить и предлагаю прочитать первоисточник: "Как один разработчик PVS-Studio защищал баг в проверяемом проекте".
Это не единственная найденная нами ошибка в данном проекте, и, если вы хотите ознакомиться с полным перечнем интересных срабатываний, их можно найти в статье: "PVS-Studio ищет баги в проекте DuckStation".
Первое место: показательный пример ещё большей невнимательности
Так уж получилось, что в этом году мы сами наступали на те же грабли, от которых стараемся уберечь пользователей. Мы не стесняемся об этом рассказывать, так как такие случаи очень хорошо подтверждают теорию — статический анализатор гораздо внимательнее человека. Перейдём к примеру:
V645 The 'strncat' function call could lead to the 'а.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);
....
}
На первый взгляд, кажется, что фрагмент кода корректный и неопределённое поведение нам не грозит. Однако давайте внимательно посмотрим на выражение:
sizeof(a.consoleText) – strlen(a.consoleText) – 5Его результатом легко может стать отрицательное число! Такое может случиться, например, при strlen(a.consoleText) = 508. В таком случае произойдёт переполнение беззнакового числа, и результатом выражения будет максимальное значение результирующего типа, в данном случае -size_t.
Эта ошибка вошла в топ из статьи: "Один день из жизни разработчика PVS-Studio, или отладка диагностики, которая оказалась внимательнее трёх программистов".
Заключение
Этот год вышел плодотворным на статьи по проверке C++ проектов. Интересных срабатываний в них могло бы хватить на несколько обзоров. Поэтому если ваше видение отличается от моего, то вы можете составить свой альтернативный "Tоп-10...", почитав статьи из нашего блога.
Писать такие новогодние статьи уже стало традицией, и поэтому я предлагаю вашему вниманию статьи с топ-10 ошибок в C++ проектах прошлых лет: 2016, 2017, 2018, 2019, 2020.
