Команда PVS-Studio вместе с Сергеем Кушниренко подготовила квиз на основе его публикаций. Вам предстоит попробовать найти ошибки в C++ коде и проверить свою внимательность и знание языка. В этой статье сделан подробный разбор всех предлагаемых вопросов.
Если вы ещё не проходили квиз, а открыли эту статью, то предлагаем не спешить её читать. Интерес в том, чтобы самим попробовать понять, где находится ошибка в коде.
Пройдите квиз и возвращайтесь.
Теперь, после прохождения квиза, у вас, возможно, появились вопросы или непонимание по некоторым из предложенных заданий и ответов. Заваривайте чай/кофе, и мы неспешно разберём задачки. Но в начале несколько слов благодарности.
Квиз появился благодаря публикациям Сергея Кушниренко на сайте Habr. Команда PVS-Studio предложила оформить рассматриваемые в статьях фрагменты кода в виде развлекательного задачника.
Сергей Кушниренко - AI Gameplay Engineer at 4A Games.
Занимается классическим игровым ИИ, любит разбирать алгоритмы и старые автомобили. Ошибки и приведённые примеры он взял из проектов, над которыми работал. В них отключены исключения и есть особенности компиляторов от вендора, так что некоторые очевидные вещи могут не ловиться, а программа не падать и вести себя странно.
Профиль на сайте LinkedIn: dalerank.
Спасибо Сергею, что он согласился поучаствовать в этом проекте.
Этот код использовался в движке Unity 2014 для управления поворотом объектов с помощью гизмо (такой элемент управления обычно в форме трех окружностей, который позволяет крутить объект на сцене). В данном случае функция setDeltaPitch применяется для изменения угла наклона объекта относительно его вертикальной оси (pitch). При углах, близких к 0 (зависело от настроек редактора), функция просто переворачивала объект вверх ногами, что очень бесило дизайнеров уровней.
void UObject::setDeltaPitch(const UMatrix &gizmo) {
....
if (_fpzero(amount, eps))
return
rotateAccum.setAnglesXYZ(axis);
....
}Варианты ответа:
Ответ:
После return отсутствует точка с запятой, что при определённых условиях приводило к вызову функции setAnglesXYZ в управляемом объекте и переворачивало его на произвольный угол.
Примечание команды PVS-Studio
Кажется, что ошибки с забытой точкой с запятой весьма распространены. Они часто упоминаются в разных статьях. Но на самом деле они встречаются очень редко. Более чем за 10 лет проверки открытых проектов мы нашли всего 3 таких ошибки. Сказывается, что все знают про эти ошибки и внимательны к ним. Плюс анализаторы и компиляторы, видимо, тоже хорошо помогают обнаруживать их.
Тут повеселился компилятор. Эта функция использовалась для вычисления хеш-суммы при проверке целостности файлов. При создании контента вычислялся хеш файлов и сохранялся вместе с файлами. Позже при загрузке того же файла плеер Unity снова вычислял хеш файла и сравнивал его с сохранённым, чтобы убедиться, что файл не был изменён или повреждён. Шифрование использовалось на финальном этапе упаковки. По замыслу автора этого кода, ключ не должен был утечь за пределы этой функции, но что-то пошло не так. Наличие части утёкших исходников Unity и этого бага в движке помогло SKIDROW в 2017 взломать квест Syberia 3 (Сибирь 3) и еще несколько больших игр на этом движке, которые использовали нативные средства шифрования контента. Там, конечно, ещё был Denuvo, но его SKIDROW научились обходить ещё до этого.
void UContentPackage::createPackage(dsaFile *m, const void *v, int size) {
unsigned char secretDsaKey[3096], L;
const unsigned char *p = v;
int i, j, t;
....
UContent::generateDsaKey(secretDsaKey, sizeof(salt));
....
// тут был какой-то код для шифрования и подписи файла
....
memset (secretDsaKey, 0, sizeof(secretDsaKey));
}Варианты ответа:
Ответ
Функция memset не вызывалась из-за агрессивной оптимизации компилятора. Действия с secretDsaKey не проводились после memset, поэтому компилятор её просто выкидывал. Всё содержимое ключа оставалось на стеке.
Примечание команды PVS-Studio
Здесь ситуация по сравнению с предыдущим паттерном ошибки полностью противоположная. Ошибка с исчезновением memset кажется чем-то экзотическим и редким. Но это очень распространённая потенциальная уязвимость. На момент написания этой заметки наша коллекция уже насчитывает 330 таких ошибок в разнообразнейших открытых проектах и продолжает пополняться. Хотя, казалось бы, на эту тему тоже уже много написано, например:
Также приватные данные могут быть отправлены наружу, например по сети. Как? Рассматриваем в заметке "Перезаписывать память - зачем?".
Здесь могут быть проблемы при работе из двух и более потоков с любой из переменных a/b. Эта ошибка присутствовала в движке CryTek при синхронизации состояний автомобилей по сети, из-за чего наблюдались рывки и телепорты во время движения на авто в мультиплеере FarCry 1. Чем больше игроков было на карте, тем больше была вероятность телепорта у последнего игрока. При 16 игроках на карте последнего стабильно телепортировало при использовании авто.
struct X {
int a : 2
int b : 2
} x;
Thread 1:
void foo() { x.a = 1 }
Thread 2:
void boo() { x.b = 1 }Варианты ответа:
Ответ
Нарушение атомарности операции записи. Поля a и b не являются атомарными, что означает, что они могут выполняться частично и прерываться другими операциями. В данном коде присутствует общий доступ к комплексной переменной AB, которая состоит из двух двубитовых частей. Но компилятор не может выполнить такое присвоение атомарно, захватывая сразу байт ab и битовыми операциями выставляя нужное значение. Это может привести к гонкам данных и неопределённому поведению в многопоточной среде.
foo(): # @foo()
mov AL, BYTE PTR [RIP + x] ; разорваное присвоение 1
and AL, -4 ; разорваное присвоение 2
or AL, 1 ; разорваное присвоение 3
mov BYTE PTR [RIP + x], AL ; закончили
ret
boo(): # @boo()
mov AL, BYTE PTR [RIP + x]
and AL, -13
or AL, 4
mov BYTE PTR [RIP + x], AL
retЭтот код содержит гонку данных даже при наличии "вроде бы живого" мьютекса. Ошибка была замечена в прошивке Nintendo Switch 4.5.1 и выше. Наткнулись на неё совершенно случайно, когда пытались ускорить создание атласов UI текстур на старте игры. Если пробовали загрузить в атлас больше 100 текстур, то ломали её. А если меньше, то атлас собирался нормально. Такие мьютексы-зомби на Nintendo Switch до сих пор не пофикшены. А ещё на Nintendo Switch можно было сделать не больше 256 мьютексов на приложение, вот такая вот весёлая система.
const size_t maxThreads = 10;
void fill_texture_mt(int thread_id, std::mutex *pm) {
std::lock_guard<std::mutex> lk(*pm);
// Access data protected by the lock.
}
void prepare_texture() {
std::thread threads[maxThreads];
std::mutex m;
for (size_t i = 0; i < maxThreads; ++i) {
threads[i] = std::thread(fill_texture_mt, i, &m);
}
}Варианты ответа:
Ответ
Мьютекс удаляется сразу по завершении функции prepare_texture. Операционная система никак не реагирует на неактивный мьютекс, потому что, как объект ядра, он продолжает еще некоторое время жить. Адрес является валидным и содержит корректные данные, но не предоставляет реальной блокировки потоков.
Примечание. Судя по документации, при выходе из функции позовутся деструкторы активных std::thread, что должно приводить к аварийному завершению работы (вызову terminate). Однако, по словам автора, на этом проекте такого не происходило. Непонятный момент. Впрочем, это не так важно, ведь код в любом случае некорректен.
Функции могут быть определены так, чтобы принимать на месте вызова больше аргументов, чем указано в объявлении. Такие функции называются вариативными (variadic). C++ предоставляет два механизма, с помощью которых можно определить вариативную функцию: шаблон с переменным числом параметров и использование многоточия в стиле C в качестве окончательного объявления параметра. Очень неприятное поведение было встречено в популярной библиотеке для работы со звуком FMOD Engine. Код приводится в том виде, какой он был в исходниках: похоже, ребята хотели сэкономить на шаблонах. Поиграть с кодом на сайте OnlineGDB.
int var_add(int first, int second, ...) {
int r = first + second;
va_list va;
va_start(va, second);
while (int v = va_arg(va, int)) {
r += v;
}
va_end(va);
return r;
}Варианты ответа:
Ответ
Это вариативная функции в стиле C для сложения ряда целых чисел. Аргументы будут считываться до тех пор, пока не будет найдено значение 0. Вызов этой функции без передачи значения 0 в качестве завершающего аргумента (после первых двух аргументов) приводит к неопределённому поведению. Более того, передача любого типа кроме int также приводит к неопределённому поведению.
А вот тут мы опять ничего не залочили, хотя на первый взгляд залочили. Этот код был обнаружен в движке Metro: Exodus в некоторых местах при работе с ресурсами, что приводило к странным крашам. Нашли благодаря баг-репортам и одному умному французу.
static std::mutex m;
static int shared_resource = 0;
void increment_by_42() {
std::unique_lock<std::mutex>(m);
shared_resource += 42;
}Варианты ответа:
Ответ
Автор кода ожидает, что анонимная локальная переменная типа std::unique_lock будет блокировать и разблокировать мьютекс m посредством RAII. На самом деле создаётся новая переменная с именем m, инициализированная по умолчанию. Соответственно, мьютекс захвачен не будет. См. также "Most vexing parse".
Примечание команды PVS-Studio
Очень редкая ошибка. Соответствующая диагностика V1025 в PVS-Studio есть, но пока при исследовании открытых проектов мы так и не нашли ошибку такого типа. Интересно было её встретить, как говорится, в естественной среде обитания :).
А вот такой код использовался для подсчёта контрольных сумм областей в PathEngine и случайно сбоил в релизной сборке на разных компиляторах. При сборке для PlayStation использовался определённый флаг, который маскировал проблему, а на Xbox его не было. Ошибку обнаружили, когда попробовали собрать библиотеку на ПК с помощью Сlang.
struct AreaCrc32 {
unsigned char buffType = 0;
int crc = 0;
};
AreaCrc32 s1 {};
AreaCrc32 s2 {};
void similarArea(const AreaCrc32 &s1, const AreaCrc32 &s2) {
if (!std::memcmp(&s1, &s2, sizeof(AreaCrc32))) {
// ....
}
}Варианты ответа:
Ответ
Поля в структуре будут выравнены по границе в 4 байта. Между buffType и сrc образуются байты выравнивания, которые могут быть заполнены нулями, а могут мусором.
struct S {
unsigned char buffType = 0;
char[3] _padding = { compiler don't care about content in release };
int сrc = 0;
};Функция memcmp сравнивает память побайтово, захватывая и мусорные байты тоже, поэтому получается неизвестный результат этой операции при сравнении s1 с s2. Настройки компилятора для плойки явно указывали компилятору забивать байты выравнивания нулями. Правила C++ для структур говорят, что нам нужно использовать в этом месте оператор ==, и memcmp не предназначен для данной задачи.
Заодно приведём ещё один вариант, когда может сравниваться лишнее. Как-то раз на ревью было вот такое. Вовремя заметили.
class C {
int i;
public:
virtual void f();
// ...
};
void similar(C &c1, C &c2) {
if (!std::memcmp(&c1, &c2, sizeof(C))) {
// ...
}
}Сравнивать плюсовые структуры и классы через memcmp — плохая затея, особенно если в них есть указатель на таблицу виртуальных функций.
Пусть от C унаследованы два разных класса. Тогда если c1 и c2 — ссылки на экземпляры этих двух разных классов, то они не будут равны, даже если все данные совпадают (кроме указателя на таблицу виртуальных функций).
Но это ещё не всё. Из-за наличия неявного указателя в классе могут начаться проблемы с выравниванием (наличие байт выравнивания).
На всякий случай напоминаем, что этот текст подготовлен на основе публикаций Сергея Кушниренко и его личного опыта. Хочется избежать вопросов, почему команда PVS-Studio задаёт именно такие вопросы или пишет именно такой код. Мы это не делаем :). Сергей не является частью команды PVS-Studio. Решили оставить этот комментарий, чтобы не повторялась история странных обсуждений, как это вышло после публикации перевода сторонней заметки про "Google-программистов".
Тут вроде просто должно быть, но что-то часто на таких вещах претенденты спотыкаются на собеседованиях.
struct S { S(const S *) noexcept; /* ... */ };
class T {
int n;
S *s1;
public:
T(const T &rhs) : n(rhs.n),
s1(rhs.s1 ? new S(rhs.s1) : nullptr) {}
~T() { delete s1; }
// ...
T& operator=(const T &rhs) {
n = rhs.n;
delete s1;
s1 = new S(rhs.s1);
return *this;
}
};Варианты ответа:
Ответ
В operator = нет проверки, что объект присваивается сам себе. В итоге удалим данные и загрузим какой-то мусор.
В operator = есть ещё одно слабое место. Если исключения в проекте не отключены, то потенциально new может бросить std::bad_alloc. В этом случае в this->s1 остаётся старое и, возможно, ненулевое значение. Которое затем будет передано в delete в деструкторе. И получим double free.
Примечание команды PVS-Studio
Переставляя поля в структуре/классе, действительно иногда можно уменьшить их размер. Но к данному классу это не относится. В 32-битной программе размер класса всегда будет 8 байт, а в 64-битной — всегда 16 байт из-за необходимости выравнивания. По крайней мере это так для наиболее распространённых моделей данных и архитектур. Подробнее про уменьшение размера классов и структур: V802.
Даже в четырёх строках кода можно словить неочевидный баг. Особенно если это Unity Engine, который любит включать общие заголовки в разных DLL, а потом сравнивать их вот так super_secret() == super_secret(). Казалось бы, что может пойти не так, но секрет != секрету, и игра не грузится на Windows Phone.
// header a.h
constexpr inline const char* super_secret(void) {
constexpr const char *STRING = "string";
return STRING;
}
// где-то в a.dll
if (super_secret() == saved_string_ptr)
// где-то в b.dll
if (super_secret() != saved_string_ptr)Варианты ответа:
Ответ
А ошибка в том, что общий хедер a.h использовался в разных библиотеках. Все работало, пока движок собирался как единая DLL. Собрав две разные DLL, мы получим два разных адреса для STRING. А вообще ошибка в том, что строки сравнивают как указатели, а не функцией strcmp.
Примечание команды PVS-Studio
Большинство описанных в статье багов так или иначе связано с индустрией разработки игр. А раз так, возможно эту статью читает немало GameDev разработчиков. У команды PVS-Studio есть просьба. Мы начали реализовывать в анализаторе диагностики для выявления багов, специфичных для проектов, основанных на Unity и Unreal Engine. Просим присылать нам идеи для новых диагностик.
У этой функции может быть UB с переполнением буфера. Часто это происходит для Clang с оптимизацией -O3, и не происходит с -O0. На clang 12.10 и выше это уже исправлено для всех режимов оптимизации. Код не мой, всплыл на каком-то из собесов, когда просто беседовали по душам.
char destBuffer[16];
void serialize(bool x) {
const char* whichString = x ? "true" : "false";
const size_t len = strlen(whichString);
memcpy(destBuffer, whichString, len);
}
int main() {
bool x;
serialize(x);
printf("%s", destBuffer);
return 0;
}Варианты ответа:
Ответ
Здесь компилятор перехитрил сам себя. Итак, что же происходит? Для начала стоит напомнить, что bool — это не просто 0 или 1, он зависит от компилятора. Clang определяет, что 0 — ложь, другое — истина. Агрессивная оптимизация компилятора преобразует вычисление len в 'len = 5 – x', а переменная x не инициализирована, поэтому в некоторых случаях у нас получается 'len = 5–7'. В таком случае программа падает при вызове memcpy из-за переполнения буфера.
Примечание команды PVS-Studio
Программисты достаточно часто ошибаются, пытаясь предсказать, как проявит себя неопределённое поведение. Это невозможно и не нужно делать. Хорошая заметка на эту тему: "Ложные представления программистов о неопределённом поведении".
Что будет выведено в результате работы программы?
void sayHello() {
std::cout << "Hello, World!\n";
}
void sayHello() {
std::cout << "Goodbye, World!\n";
}
int main() {
sayHello();
return 0;
}Варианты ответа:
Ответ
При запуске кода в консоль будет выведено "Goodbye, World!". Потому что имя второй функции sayHello написано с использованием Unicode, который, к сожалению, не всегда визуально различим. Она же и вызывается (Clang, latest).
sayHello(): # @sayHello()
push rbp
mov rbp, rsp
mov rdi, qword ptr [rip + std::cout@GOTPCREL]
lea rsi, [rip + .L.str]
call std::basic_ostream<char, std::char_traits<char> >&
std::operator<< <std::char_traits<char> >(std::basic_ostream<char,
std::char_traits<char> >&, char const*)@PLT
pop rbp
ret
"_Z9sayHellov": # @"_Z9say\D0\9Dellov"
push rbp
mov rbp, rsp
mov rdi, qword ptr [rip + std::cout@GOTPCREL]
lea rsi, [rip + .L.str.1]
call std::basic_ostream<char, std::char_traits<char> >&
std::operator<< <std::char_traits<char> >(std::basic_ostream<char,
std::char_traits<char> >&, char const*)@PLT
pop rbp
retПримечание команды PVS-Studio
Всех, кому интересно узнать про эти нюансы, связанные с Unicode, приглашаем сюда: "Атака Trojan Source для внедрения в код изменений, незаметных для разработчика".
Функция обладает удивительной способностью изменять реальность в зависимости от того, сколько ног у ее вызывающего.
int abs_legs(int my_legs) {
if (my_legs < 0) {
return -my_legs;
}
}Варианты ответа:
Ответ
Функция, возвращающая значение, должна возвращать значение (хм!) из всех возможных веток, потому что всегда найдётся такое условие, которое приведёт к неопределённому поведению.
Правильный код:
int abs_legs(int my_legs) {
if (my_legs < 0) {
return -my_legs;
}
return my_legs;
}Примечание команды PVS-Studio
Многие думают, что неопределённое поведение здесь сводится к тому, что функция может вернуть случайное значение. Ещё раз скажем, что это не так. С уверенностью можно говорить только что этот код содержит ошибку. Как именно она проявит себя, предсказать невозможно. Например, компилятор может вообще не сгенерировать инструкцию для выхода из функции. Не верите? Тогда читайте "Пример проявления неопределённого поведения из-за отсутствия return".
Эта функция не раскрывается полностью, оставляя свою магию за покровом компилятора.
int get_money(int index, const int *pockets) {
int a = index + pockets[++index];
// ....
return a;
}Варианты ответа:
Ответ
Компилятор может переставить порядок операции для index + pockets[++index], и это приведёт к неоднозначному поведению с разными настройками оптимизации. Неупорядоченная или неопределённая последовательность операций приведёт к сайд эффекту при работе с переменной index.
Аналогичный код, приведённый в "Order of evaluation", считается приводящим именно к неопределённыму поведению:
n = ++i + i; // undefined behaviorТак что следует переписать код так:
int a = index + pockets[index + 1];
++index;Или так, в зависимости от того, что хочется получить:
++index;
int a = index + pockets[index];Примечание команды PVS-Studio
А вы знаете, как поведёт себя следующий код?
printf("%d, %d\n", i++, i++);Уверены? Приглашаем почитать "Глубина кроличьей норы или собеседование по C++ в компании PVS-Studio".
С виду корректный код, но ведь не просто так он попал сюда? :)
void umcp_read_buffer_from_pipe() {
char bufKernel[12];
char bufMatrix[12];
std::cin.width(12);
std::cin >> bufKernel;
std::cin >> bufMatrix;
}Варианты ответа:
Ответ
В этом примере первое чтение не приведёт к переполнению и заполнит bufKernel усечённой строкой. Но второе чтение может переполнить bufMatrix. Чтобы этого не произошло, надо также вызвать std::cin.width(12); перед получением bufMatrix. Или воспользоваться безопасной работой через строки.
void umcp_read_buffer_from_pipe() {
std::string bufKernel, bufMatrix;
std::cin >> bufKernel >> bufMatrix;
}Если вы используете C++20, то будет выбрана следующая перегрузка функции, и всё пройдёт безопасно. Даже если вы специально об этом не заботились...
template< class Traits, std::size_t N >
basic_istream<char, Traits>&
operator>>( basic_istream<char, Traits>& st, signed char (&s)[N] );Кажется, в этом коде "нильзя-программер" просто смешивает случайности с абсурдом и называет это "настройкой отображения".
std::string str_func();
void display_string(const char *);
void set_display_options() {
const char *str = str_func().c_str();
display_string(str);
}Варианты ответа:
Ответ
Здесь std::string::c_str вызывается для временного объекта std::string. Полученный указатель будет указывать на освобожденную, но, возможно, еще валидную память после уничтожения объекта std::string в конце выражения присваивания. Однако надеяться на это не стоит – при разыменовании этого указателя получите неопределённое поведение.
Правильно:
std::string str = str_func();
const char *cstr = str.c_str();
display_string(cstr);Или так:
display_string(str_func().c_str())Почему утром опасно ходить в bar?
void morning(const std::string &owner) {
std::fstream bar(owner);
if (!bar.is_open()) {
// Handle error
return;
}
bar << "customer";
std::string str;
bar >> str;
}Варианты ответа:
Ответ
Потому что можно не выйти. Данные добавляются в конец файла, а затем считываются из того же файла. Однако поскольку указатель в файле остался на прежнем месте (в конце), то попытка прочитать данные (из конца) файла ни к чему не приведёт. В бар надо ходить вечером, тогда всё "замечательно выходит" (с).
void evening(const std::string &owner) {
std::fstream bar(owner);
if (!bar.is_open()) {
// Handle error
return;
}
bar << "customer";
std::string str;
bar.seekg(0, std::ios::beg);
bar >> str;
}Станет ли бард пьяницей?
class Bard {
int _beer;
int _meal;
public:
Bard(int meal) : _meal(meal), _beer(_meal - 1) {}
};Варианты ответа:
Ответ
Члены класса инициализируются в порядке их объявления, а не в том, как они перечислены в списке инициализации конструктора.
Порядок инициализации членов класса нарушен. Из-за этого сначала будет инициализирована переменная _beer, а уже затем _meal. Попытка прочитать значение _meal до его инициализации приводит к неопределённому поведению. Скорее всего, бард сопьётся без закуси.
Исправить можно так:
Bard(int meal) : _beer(meal - 1), _meal(meal) {}Другой вариант исправления заключается в использовании везде для инициализации переменной meal:
Bard(int meal) : _meal(meal), _beer(meal - 1) {}Ещё можно поменять поля класса местами:
class Bard {
int _meal;
int _beer;
public:
Bard(int meal) : _meal(meal), _beer(_meal - 1) {}
};В каком случае выстрел по ногам будет особенно результативным?
std::vector<....> formats;
....
for (auto format = begin(formats), __end = end(formats);
format != __end; ++format)
{
if (snd::CodecNamesEq(....)) {
format->is_stereo = true;
formats.push_back(stereo_format);
}
}Варианты ответа:
Ответ
Если в процессе push_back произойдет реаллокация внутри контейнера formats, то итератор format будет указывать уже на удалённую память, работа с которой вызовет кучу проблем.
И переменная с именем __end — это плохо. Переменные, содержащие два знака подчёркивания подряд, зарезервированы для стандартной библиотеки и внутреннего использования.
Примечание. Хоть это и не указано в коде, но подразумевается, что используемый контейнер может инвалидировать итераторы при вставке элементов в конец. Это важно, т.к. тот же push_back у std::list так не делает.
Спасибо Сергею Кушниренко за предоставленные материалы для создания квиза и этой публикации. Спасибо Андрею Карпову, Михаилу Гельвиху, Филиппу Хандельянцу и другим членам команды PVS-Studio. Почему бы, собственно, самих себя не похвалить :). Спасибо читателям, надеемся, это было интересно. Если хочется ещё, приглашаем сюда.
Оставляйте комментарии про свои забавные случаи или странный код, который вы когда-то повстречали.