За окном зима, год стремится к завершению, а значит, пришло время рассмотреть самые интересные ошибки, обнаруженные анализатором PVS-Studio в 2020 году.
Стоит отметить, что прошедший год ознаменовался большим количеством новых диагностических правил, срабатывания которых позволили им попасть в данный топ. Также мы продолжаем улучшать ядро анализатора и добавлять новые сценарии его использования, обо всём этом можно почитать в нашем блоге. Если вам интересны другие поддерживаемые нашим анализатором языки (C, C# и Java), обратите внимание на статьи моих коллег. Теперь же перейдём непосредственно к самым запомнившимся мне багам, найденным PVS-Studio за прошедший год.
Десятое место: Деление по модулю на единицу
V1063 The modulo by 1 operation is meaningless. The result will always be zero. llvm-stress.cpp 631
void Act() override {
....
// If the value type is a vector, and we allow vector select,
// then in 50% of the cases generate a vector select.
if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {
unsigned NumElem =
cast<FixedVectorType>(Val0->getType())->getNumElements();
CondTy = FixedVectorType::get(CondTy, NumElem);
}
....
}Разработчик хотел получить случайное значение в диапазоне от 0 до 1, использовав деление по модулю. Однако операция вида X%1 всегда вернёт 0. В данном случае правильно было бы переписать условие следующим образом:
if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))Эта ошибка вошла в топ из статьи: "Проверка Clang 11 с помощью PVS-Studio".
Девятое место: Четыре проверки
На следующий участок кода PVS-Studio выдал четыре предупреждения:
- V560 A part of conditional expression is always true: x >= 0. editor.cpp 1137
- V560 A part of conditional expression is always true: y >= 0. editor.cpp 1137
- V560 A part of conditional expression is always true: x < 40. editor.cpp 1137
- V560 A part of conditional expression is always true: y < 30. editor.cpp 1137
int editorclass::at( int x, int y )
{
if(x<0) return at(0,y);
if(y<0) return at(x,0);
if(x>=40) return at(39,y);
if(y>=30) return at(x,29);
if(x>=0 && y>=0 && x<40 && y<30)
{
return contents[x+(levx*40)+vmult[y+(levy*30)]];
}
return 0;
}Все предупреждения относятся к последнему if-выражению. Проблема в том, что все четыре проверки, которые в нём выполняются, всегда будут возвращать true. Не сказал бы, что это серьезная ошибка, но получилось довольно забавно. В общем, данные проверки являются избыточными и их можно убрать.
Эта ошибка вошла в топ из статьи: "VVVVVV??? VVVVVV!!!".
Восьмое место: delete вместо delete[]
V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 410
BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
....
char *poke_data = new char [length + 2*sizeof(int)]; // <=
....
if(DDE_Class->Poke_Server( .... ) == FALSE) {
CCDebugString("C&C95 - POKE failed!\n");
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (FALSE);
}
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (TRUE);
}Анализатор обнаружил ошибку, связанную с тем, что память выделена и освобождена несовместимыми между собой способами. Для освобождения памяти, выделенной под массив, следует использовать оператор delete[], а не delete.
Эта ошибка вошла в топ из статьи: "Код игры Command & Conquer: баги из 90-х. Том второй".
Седьмое место: Выход за границу буфера
Рассмотрим функцию net_hostname_get, которая будет использоваться дальше.
#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
return "zephyr";
}
#endifВ данном случае при препроцессировании выбирался вариант, относящийся к ветке #else. То есть, в препроцессированном файле функция реализуется так:
static inline const char *net_hostname_get(void)
{
return "zephyr";
}Функция возвращает указатель на массив из 7 байт (учитываем терминальный ноль в конце строки).
Теперь рассмотрим код, приводящий к выходу за границу массива.
static int do_net_init(void)
{
....
(void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
....
}Предупреждение PVS-Studio: V512 [CWE-119] A call of the 'memcpy' function will lead to the 'net_hostname_get()' buffer becoming out of range. log_backend_net.c 114
После препроцессирования MAX_HOSTNAME_LEN раскрывается следующим образом:
(void)memcpy(hostname, net_hostname_get(),
sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));Соответственно, при копировании данных возникает выход за границу строкового литерала. Как это скажется на выполнении программы — предсказать сложно, так как это приводит к неопределённому поведению.
Эта ошибка вошла в топ из статьи: "Исследуем качество кода операционной системы Zephyr".
Шестое место: Что-то очень странное
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
memcpy(cpy_mntpt, mntpt, strlen(mntpt));
}
return cpy_mntpt;
}Предупреждение PVS-Studio: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. shell.c 427
Кто-то пытался сделать аналог функции strdup, но у него это не получилось.
Начнём с предупреждения анализатора. Он сообщает, что функция memcpy копирует строчку, но не скопирует терминальный ноль, и это очень подозрительно.
Кажется, что этот терминальный 0 копируется здесь:
((u8_t *)mntpt)[strlen(mntpt)] = '\0';Но нет! Здесь опечатка, из-за которой терминальный ноль копируется сам в себя! Обратите внимание, что запись происходит в массив mntpt, а не в cpy_mntpt. В итоге функция mntpt_prepare возвращает строку, незавершенную терминальным нулём.
На самом деле программист хотел написать так:
((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';Однако всё равно не понятно, зачем сделано так сложно! Этот код можно упростить до следующего варианта:
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
strcpy(cpy_mntpt, mntpt);
}
return cpy_mntpt;
}Эта ошибка вошла в топ из вышеупомянутой статьи: "Исследуем качество кода операционной системы Zephyr".
Пятое место: Неправильная защита от переполнения
V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359
static DWORD
get_rel_wait(const struct timespec *abstime)
{
struct __timeb64 t;
_ftime64_s(&t);
time_t now_ms = t.time * 1000 + t.millitm;
time_t ms = (time_t)(abstime->tv_sec * 1000 +
abstime->tv_nsec / 1000000);
DWORD rel_wait = (DWORD)(ms - now_ms);
return rel_wait < 0 ? 0 : rel_wait;
}В данном случае переменная rel_wait имеет беззнаковый тип DWORD. А значит, сравнение rel_wait < 0 не имеет смысла, так как результатом всегда является ложь.
Сама по себе ошибка не очень интересная. Зато интересно вышло с тем, как её попробовали исправить. Получилось, что изменения не исправили, а лишь упростили код. Подробнее про эту историю можно прочитать в статье моего коллеги: "Почему PVS-Studio не предлагает автоматические правки кода".
Ошибка же вошла в топ из статьи: "Статический анализ кода коллекции библиотек PMDK от Intel и ошибки, которые не ошибки".
Четвёртое место: Не пиши в std, брат
V1061 Extending the 'std' namespace may result in undefined behavior. sized_iterator.hh 210
// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdВ статье, из которой взято срабатывание: "Анализ кода проекта DeepSpeech или почему не стоит писать в namespace std" подробно описано, почему не стоит поступать подобным образом.
Третье место: Скроллбар, который не смог
V501. There are identical sub-expressions to the left and to the right of the '-' operator: bufferHeight — bufferHeight TermControl.cpp 592
bool TermControl::_InitializeTerminal()
{
....
auto bottom = _terminal->GetViewport().BottomExclusive();
auto bufferHeight = bottom;
ScrollBar().Maximum(bufferHeight - bufferHeight);
ScrollBar().Minimum(0);
ScrollBar().Value(0);
ScrollBar().ViewportSize(bufferHeight);
....
}Это, что называется, "срабатывание с историей". В данном случае из-за ошибки не работал скроллбар в Windows Terminal. По мотивам данного бага написана целая статья, в которой мой коллега провёл исследование и разобрался почему так случилось. Заинтересовались? Вот она: "Скроллбар, который не смог".
Второе место: перепутали радиус и высоту
И опять речь пойдёт о нескольких предупреждениях анализатора:
- V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 791
- V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 833
- V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 884
Привожу вызовы функции:
NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);А так выглядит её объявление:
static NewtonBody* CreateWheel (DemoEntityManager* const scene,
const dVector& location, dFloat radius, dFloat height)При вызовах функций аргументы были перепутаны местами.
Эта ошибка вошла в топ из статьи: "Повторная проверка Newton Game Dynamics статическим анализатором PVS-Studio".
Первое место: Затирание результата
V519 The 'color_name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627
static bool parseNamedColorString(const std::string &value,
video::SColor &color)
{
std::string color_name;
std::string alpha_string;
size_t alpha_pos = value.find('#');
if (alpha_pos != std::string::npos) {
color_name = value.substr(0, alpha_pos);
alpha_string = value.substr(alpha_pos + 1);
} else {
color_name = value;
}
color_name = lowercase(value); // <=
std::map<const std::string, unsigned>::const_iterator it;
it = named_colors.colors.find(color_name);
if (it == named_colors.colors.end())
return false;
....
}Данная функция должна производить разбор названия цвета с параметром прозрачности и вернуть его шестнадцатеричный код. В зависимости от результата проверки условия в переменную color_name передается либо результат разбиения строки, либо копия аргумента функции.
Однако затем в функции lowercase() в нижний регистр переводится не сама полученная строка, а исходный аргумент функции. В результате мы просто потеряем цвет, который должна была вернуть parseNamedColorString().
color_name = lowercase(color_name);Эта ошибка вошла в топ из статьи: "PVS-Studio: Анализ pull request-ов в Azure DevOps при помощи self-hosted агентов".
Заключение
За прошедший год мы нашли много ошибок в open source проектах. Это были привычные ошибки copy-paste, ошибки в константах, утечки памяти и множество других проблем. Наш анализатор не стоит на месте и в топе присутствует несколько срабатываний новых диагностик, написанных в этом году.
Надеюсь, вам понравились собранные ошибки. Лично мне они показались достаточно интересными. Но, конечно, ваше видение может отличаться от моего, поэтому вы можете составить свой "Tоп 10...", почитав статьи из нашего блога или посмотрев список ошибок, найденных PVS-Studio в open source проектах.
Также предлагаю вашему вниманию статьи с топ 10 C++ ошибок прошлых лет: 2016, 2017, 2018, 2019.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. Top 10 Bugs Found in C++ Projects in 2020.
