Как статический анализ побуждает разработчика рефакторить код. По мотивам Source SDK

Раннее утро. Туман окутал горные хребты. Просыпайся, Гордон, вставай. Нам пора отправляться в сердце тьмы и освободить этот мир от лап дремлющего зла. Да, и не забудь свою монтировку.

Дорогой читатель, если у тебя временная амнезия после последнего боя, и ты не понимаешь, почему тебя звать Гордоном, ничего страшного. У меня сохранились записи последних событий нашего странствия. Они помогут освежить память.

Напомню лишь, что во время последней встречи мы познакомились с Source SDK и разобрали некоторые ошибки, начиная небольшими аномалиями и заканчивая непредсказуемыми результатами.

В финальной части нашего небольшого путешествия, посвящённого Source SDK, мы обратим внимание на одно предупреждение, которое сподвигнет нас отрефакторить код.

class DmeFramerate_t { public: .... unsigned short abs( unsigned short i ) { return i >= 0 ? i : -i; // <= } DmeFramerate_t operator*=( int i ) { Assert( abs( m_num * i ) <= USHRT_MAX ); // <= m_num *= ( unsigned short )i; return *this; } DmeFramerate_t operator/=( int i ) { Assert( abs( m_den * i ) <= USHRT_MAX ); m_den *= ( unsigned short )i; return *this; } .... private: .... unsigned short m_num; unsigned short m_den; .... };

Предупреждение PVS-Studio: V547 Expression 'i >= 0' is always true. Unsigned type value is always >= 0. timeutils.h 46

Всё началось с одного предупреждения. Видим, что метод `abs()` имеет странное поведение. Объект функции `i` с типом `unsigned short` всегда имеет положительное значение, т.е. условие `i >= 0` всегда вычисляется как `true`. Стало даже интересно, почему оно появилось. К сожалению, в репозитории отсутствует информация по blame, поэтому посмотрим весь класс. Но, к счастью, функция `abs()` из класса `DmeFramerate_t` используется только в следующих двух операторах присваивания:

`DmeFramerate_t operator*=(int i)`;
`DmeFramerate_t operator/=(int i)`.

Рассмотрим перегруженный оператор `*=`. Как видно под дебагером, с помощью `Assert()` разработчик хотел проверить, что результат умножения по модулю не будет превышать `USHRT_MAX`. Здесь возникает несколько проблем:

Проблема 1. При умножении ` m_num * i` производится вычисление знаковых аргументов, т.к. происходит неявное преобразование `m_num` в `int`. Согласно стандарту С++ (см. стандарт С++20 пункт 7.1 параграф 4), переполнение знаковых чисел приводит к неопределённому поведению.

Переполнение знаковых чисел в С++20

Начиная со стандарта С++20, знаковые целочисленные типы теперь представляются в дополнительном коде. Это значит, что при переполнении знакового числа производится оборачивание по modulo 2n, т.е. поведение всегда предсказуемо. Но даже с этим нововведением переполнение знакового числа остаётся undefined behavior. Это правило оставляет компилятору свободу на оптимизацию кода. Более подробно о переполнении можно узнать в нашей статье.

Далее результат с типом `int` преобразуется в тип `unsigned short`, где от значения типа `int` отрезаются 2 старших байта, и результат трактуется как беззнаковое число. Здесь мы заметим неявную конверсию значения из типа большого размера в тип меньшего размера.

Проблема 2. Как уже упоминалось выше, переменная `i` с типом `unsigned short` всегда имеет положительное значение, т.е. условие `i >= 0` всегда вычисляется как `true`;

Проблема 3. В итоге результат возвращаемого значения функции `abs()` — это `unsigned short`, который всегда будет меньше или равен `USHRT_MAX`;

Проблема 4. Помимо полученных багов в функции `abs()` мы ещё имеем нерабочий `Assert()`. Идём дальше и обращаем внимание на следующую строку.

m_num *= ( unsigned short )i;

Здесь мы снова наблюдаем сценарий сужения типов, конвертируя i с типом int в тип unsigned short. Это может привести к неожиданным результатам.

Проблема 5. Не отходя от кассы, обратим внимание на возвращаемое значение оператора *=, которое передаётся по копии. Хотя при реализации присваивающего оператора можно произвольно выбирать тип возвращаемого значения, но всё же следует возвращать ссылку на левый операнд, который может быть использован как правый операнд в другом присваивании. Это избавит код от лишнего копирования и сделает перегруженный оператор максимально похожим на поведение соответствующих встроенных операторов.

Исправленный код:

DmeFramerate_t& operator*=( int i ) {....} DmeFramerate_t& operator/=( int i ) {....}

Критиковать — не мешки ворочать. Попробуем исправить. В голову приходит два варианта, как проверить переполнение целочисленных переменных при их умножении.

Попытка 1. Использовать стандартную функцию `std::abs()`. При умножении `static_cast (m_num)` на `i` произойдёт неявное преобразование к типу с более высоким приоритетом, т.е. к типу `int`. Результат вычисления может стать как положительным, так и отрицательным. Поэтому, передав значение в функцию `std::abs()`, получим модуль целого числа c типом `int`. Теперь при сравнении левый операнд может быть больше правого `USHRT_MAX`.

Интересно, что в файле `timeutils.h` подключена библиотека`#include <math.h>`, которая содержит стандартную функцию `int abs(int num)`. В таком случае эта функция импортируется в глобальную область видимости. Но поскольку в классе `DmeFramerate_t` была определена кастомная функция `unsigned short abs(unsigned short i)`, то при её вызове в операторе `*=` для вычисления модуля целого числа будет вызван метод класса `DmeFramerate_t` по правилу поиска неквалифицированных имён. Поэтому простое решение — это удалить метод `unsigned short abs(unsigned short i)` в классе `DmeFramerate_t`. Или более гуманный способ: явно указать пространство имён, в котором будет производится поиск функции.

Попытка исправления кода:

DmeFramerate_t& operator*=( int i ) { Assert( std::abs( m_num * i ) <= USHRT_MAX ); .... } DmeFramerate_t& operator/=( int i ) { Assert( std::abs( m_den * i ) <= USHRT_MAX ); .... }

Вариант 2. Создать свою функцию. Реализовать можно разными способами. Предположим, в левый аргумент всегда поступает значение типа unsigned short, а в правый аргумент может поступать любое целочисленное значение. Важным моментом является то, что функция должна проверять, что результат вычисления и правый аргумент находятся в диапазоне unsigned short.

Пример функции:

template<typename T> bool DmeFramerate_t::isMultiplyInUShort(unsigned short lhs, T rhs) { static_assert(std::is_integral<T>::value, "Second argument must be an integral type"); if (lhs == 0 || rhs == 0) return true; uintmax_t absRhs; if constexpr (std::is_signed<T>::value) { if (rhs < 0) { absRhs = static_cast<uintmax_t>(-rhs); } else { absRhs = static_cast<uintmax_t>(rhs); } } else { absRhs = static_cast<uintmax_t>(rhs); } if (absRhs > std::numeric_limits<unsigned short>::max()) return false; return lhs <= std::numeric_limits<unsigned short>::max() / absRhs; }

Используем нашу функцию вместо DmeFramerate_t::abs().

class DmeFramerate_t { public: .... DmeFramerate_t& operator*=( int i ) { Assert(isMultiplyInUShort( m_num, i )); .... } DmeFramerate_t& operator/=( int i ) { Assert(isMultiplyInUShort( m_den, i)); .... } .... };

Также стоит обратить внимание на аналогичные срабатывания в других местах кода:

V547 Expression is always true. imesh.h 814
V547 Expression 'nSpew != 0' is always false. c_sceneentity.cpp 1121
V547 Expression 'nSpew > 1' is always false. c_sceneentity.cpp 1132
V547 Expression 'nSpew != 0' is always false. c_sceneentity.cpp 1139
V547 Expression 'm_nActiveParticles >= 0' is always true. Unsigned type value is always >= 0. particlemgr.cpp 971
V547 Expression 'iClient < 0' is always false. voice_status.cpp 321
V547 Expression 'i < 2' is always true. c_citadel_effects.cpp 216
V547 Expression 'bSortNow == true' is always true. c_smokestack.cpp 489
V547 Expression 'nRetVal > 0' is always true. baseassetpicker.cpp 465
V547 Expression 'waterSurfaceTexInfoID >= 0' is always false. ivp.cpp 1215
V547 Expression 'firstedge >= 0' is always true. Unsigned type value is always >= 0. writebsp.cpp 1429
V547 Expression 'pEdge->v[0] >= 0' is always true. Unsigned type value is always >= 0. writebsp.cpp 1436

Функция isMultiplyInUShort получилась специфичной. Сделаем её обобщенной для любых целочисленных аргументов с возможностью указать результирующий тип. Разобьём функционал на четыре части:

bool MulIsSafe(A a, B b) для знаковых чисел;
uintmax_t ToUnsigned(T x);
bool MulIsSafe(A a, B b) для беззнаковых чисел;
std::optional<Target> Mul(A a, B b).

Этап 1. Функция для проверки переполнения при умножении, если тип возвращаемого значения — это целочисленное знаковое число:

template<std::signed_integral Target, std::integral A, std::integral B> bool MulIsSafe(A a, B b) { auto maxTarget = std::numeric_limits<Target>::max(); auto minTarget = std::numeric_limits<Target>::min(); if (maxTarget < a || maxTarget < b) return false; if (minTarget > a || minTarget > b) return false; if (a > 0) { if (b > 0) { if (a > maxTarget / b) return false; } else { if (b < minTarget / a) return false; } } else { if (b > 0) { if (a < minTarget / b) return false; } else { if (b < maxTarget / a) return false; } } return true; }

Этап 2. Вспомогательная функция ToUnsigned, которая преобразует знаковое число в беззнаковый с самым широким типом, если результирующий тип — это целочисленное беззнаковое число:

template<std::integral Target, std::integral T> uintmax_t ToUnsigned(T x) { if constexpr (std::signed_integral<Target>) { return x; } if (x < 0) { return static_cast<uintmax_t>(-x); } return static_cast<uintmax_t>(x); }

Этап 3. Функция для проверки переполнения при умножении, если тип возвращаемого значения — это целочисленное беззнаковое число:

template<std::unsigned_integral Target, std::integral A, std::integral B> bool MulIsSafe(A a, B b) { auto maxTarget = std::numeric_limits<Target>::max(); uintmax_t absA = ToUnsigned<Target>(a); uintmax_t absB = ToUnsigned<Target>(b); if (absA > maxTarget || absB > maxTarget) return false; if (absA > maxTarget / absB) return false; return true; }

Этап 4. Функция Mul, которая вызывает MulIsSafe для проверки переполнения целых чисел перед их умножением. Работает только для целочисленный типов данных:

template<std::integral Target, std::integral A, std::integral B> std::optional<Target> Mul(A a, B b) { if (!MulIsSafe<Target>(a, b)) return std::nullopt; uintmax_t absA = ToUnsigned<Target>(a); uintmax_t absB = ToUnsigned<Target>(b); return static_cast<Target>(absA * absB); }

Этап 5. Тестирование является немаловажной частью процесса разработки. Поэтому, прежде чем использовать реализованные функции, прогоним пару тестов. Это помогло отладить код. Например, в функции MulIsSafe была пропущена проверка на то, что входные аргументы могут быть вне диапазона результирующего типа.

template<std::signed_integral Target, std::integral A, std::integral B> bool MulIsSafe(A a, B b) { .... if (maxTarget < a || maxTarget < b) return false; if (minTarget > a || minTarget > b) return false; .... }

Этап 6. Применим нашу функцию в перегруженных операторах присваивания:

class DmeFramerate_t { public: .... DmeFramerate_t& operator*=( int i ) { auto multReslt = Mul<unsigned short>(m_num, i); if (!multReslt.has_value()) { Assert(false, "Detected possible overflow of an integer value."); return *this; } m_num = multReslt.value(); return *this; } DmeFramerate_t& operator/=( int i ) { auto multReslt = Mul<unsigned short>(m_den, i); if (!multReslt.has_value()) { Assert(false, "Detected possible overflow of an integer value."); return *this; } m_den = multReslt.value(); return *this; } ....

Теперь функция Mul может вернуть корректное произведение двух множителей с разными целочисленными типами. Если входные параметры или результат вычисления не соответствуют требованиям, то получим nullopt.

Отметим, что наша безопасная версия умножения накладывает дополнительные проверки, из-за чего делает код более медленным. Поэтому необходимо находить баланс между безопасностью и производительностью. Например, постараться минимизировать проверки во время выполнения, перенося их на этап компиляции.

Также можно присмотреться ко встроенным функциям компилятора для выполнения безопасных арифметических вычислений. Например, функция _builtin_mul_overflow, которая проверяет переполнение умножения.

Мы побывали в мире Source SDK, пересекли континент и обнаружили жуков вредителей, идя по следу, как настоящие сыщики.

Анализатор может указать на место в коде, где могут скрываться смертельные баги, ждущие напасть в самый неподходящий момент. Ведь достаточно одной улики, чтобы раскрыть великое преступление в Восточном экспрессе.

Если вы захотите попробовать анализатор PVS-Studio, то скачать триальную версию можно здесь. Также для open source проектов у нас предоставляется бесплатная лицензия.

Спасибо, Гордон, что прошёл этот опасный путь до конца. До новых встреч!

Как статический анализ побуждает разработчика рефакторить код. По мотивам Source SDK

Покажи, как глубока кроличья нора

Гордон, время чинить

Дополнительные мысли и предложения

Заключение