Думаю, многих из нас охватывает тёплое чувство ностальгии, когда речь идёт о PSP. Эта портативная консоль, выпущенная в 2004 году, стала настоящим прорывом для своего времени. Она подарила нам возможность наслаждаться полноценными игровыми проектами в дороге, что было невероятно новаторским для того периода. У вас словно была маленькая частичка домашней консоли в кармане.
Предисловие
Игры для PSP были разнообразными и захватывающими: от культовых серий, таких как "God of War" и "Final Fantasy", до уникальных проектов вроде "Patapon" и "LocoRoco". Каждая игра открывала новый мир, который можно было исследовать где угодно — в автобусе, на перемене в школе или просто сидя в парке. PSP также была первой консолью Sony с возможностью воспроизведения мультимедиа, что делало её универсальным устройством для развлечений.
С появлением эмуляторов для PSP эта ностальгия обрела вторую жизнь. Эмуляторы позволяют современным игрокам вновь окунуться в те незабываемые времена или открыть для себя игры, которые они пропустили. Они дают возможность запускать классические проекты на современных устройствах с улучшенной графикой и производительностью. Это особенно важно в эпоху цифровой дистрибуции, когда физические копии игр становятся редкостью.
Эмуляторы также играют важную роль в сохранении игровой истории. Они помогают не потерять доступ к играм, которые могут быть недоступны из-за устаревшего оборудования или ограниченного тиража. Благодаря сообществу энтузиастов и разработчиков эмуляторов мы можем быть уверены, что наследие PSP будет жить ещё долго.
PPSSPP — это эмулятор, который позволяет запускать игры от портативной консоли PlayStation Portable (PSP) на различных устройствах, включая ПК, смартфоны и планшеты. Он был создан Хенриком Ридгардом и впервые выпущен в 2012 году. С тех пор PPSSPP стал одним из самых популярных эмуляторов для PSP благодаря своей высокой совместимости и производительности.
Основные особенности PPSSPP:
Кроссплатформенность: эмулятор доступен для Windows, macOS, Linux, Android и iOS. Это делает его универсальным решением для геймеров на разных платформах.
Высокая производительность: PPSSPP оптимизирован для работы даже на устройствах с ограниченными ресурсами. Он поддерживает различные настройки графики, что позволяет пользователям улучшать качество изображения или снижать нагрузку на систему.
Поддержка HD-разрешений: В отличие от оригинальной PSP, которая имеет разрешение экрана 480x272 пикселя, PPSSPP позволяет увеличивать разрешение до HD и выше, что значительно улучшает визуальное восприятие игр.
Сохранение состояния: пользователи могут сохранять прогресс в любой момент игры благодаря функции сохранения состояния. Это особенно полезно в сложных играх или при необходимости прервать игровой процесс.
Мультиплатформенные сохранения: Вы можете переносить сохраненные игры между различными устройствами, что удобно для продолжения игры на ходу.
Поддержка текстур высокого разрешения: некоторые энтузиасты создают пакеты текстур высокого разрешения для популярных игр, которые можно использовать с PPSSPP для улучшения графики.
Широкая библиотека совместимых игр: эмулятор поддерживает большинство игр для PSP, включая такие популярные тайтлы как "God of War", "Final Fantasy", "Monster Hunter" и многие другие.
Активное сообщество: благодаря открытому исходному коду у PPSSPP есть активное сообщество разработчиков и пользователей, которые постоянно работают над улучшением эмулятора и добавлением новых функций.
Поддержка контроллеров: эмулятор поддерживает использование внешних контроллеров через Bluetooth или USB, что делает игровой процесс более комфортным.
Настройки управления: пользователи могут настроить управление под свои предпочтения, используя сенсорные экраны или физические кнопки на контроллерах.
В целом, PPSSPP — это мощный инструмент для всех любителей классических игр от PSP, который позволяет насладиться ими с улучшенной графикой и удобством современных устройств. Поскольку PPSSPP имеет открытый исходный код, мы с радостью решили проверить его нашим инструментом.
Результаты проверки
Перед тем, как мы перейдём непосредственно к разбору, стоит упомянуть, что код весьма качественный. Местами разработчики даже перестраховывались лишний раз. Однако и в такой код закралось немало ошибок.
Я проверял код PPSSPP версии 1.17.1 при помощи анализатора PVS-Studio на Linux. Версия анализатора — 7.32.
Баги минувшего релиза
Со времён последнего релиза (4 февраля 2024 года) прошло немало времени. Код уже успел порядком измениться. Поэтому, когда я сидел и вставлял ссылки на GitHub в описываемых фрагментах, я заметил, что некоторые из ошибок в мастер-ветке уже поправлены. Но от этого ценность таких фрагментов для статьи не уменьшилась. Это, наоборот, подтверждает, что:
это точно были ошибки;
разработчики потратили время на их поиск и исправление.
Выскажусь чуть подробнее по каждому пункту.
Это точно были ошибки. Маловероятно, что кто-то полезет править код просто так. Как говорит наш архитектор: "Работает – не трожь". Скорее всего, кто-то сообщил о реальном баге, поэтому его взяли и поправили.
Разработчики потратили время на их поиск и исправление. Идеально, если ваш проект — Open Source, в котором заметили баг, воспроизвели его, нашли проблемное место, пофиксили и отправили PR. Но даже для Open Source-проекта вам чаще всего приходит Issue и приходится вместо разработки новых фичей сидеть под отладчиком. Иногда отладка и на 50 часов растягивается.
Самый плохой вариант, если ваш проект — коммерческий. Вы продаёте его, и тут клиент отказывается от него потому, что именно ему этот баг как-то вставил палки в колёса, испортив впечатление от продукта. В таком случае, время — ваш злейший враг. Чем дольше баг у вас в коде, тем больше вреда он может принести бизнесу.
Все эти доводы являются отличным доказательством преимущества проектов, использующих статический анализ: обнаружение ошибок на раннем этапе, когда код только пишется.
Конкретно в случае PPSSPP я не могу утверждать, правили ли все эти ошибки авторы проекта или добрые самаритяне, и как долго исправлялись эти ошибки после обнаружения. Но факт — некоторые из них прожили в проекте годами, чтобы потом наступил их звёздный час :)
Ну и напоследок перед самым интересным. Несмотря на то, что разработчики пофиксили ошибки во фрагментах ниже, анализатор нашёл похожие паттерны в других местах (в статье они будут упомянуты).
Фрагмент N1
Рассмотрим первый такой фрагмент, который разработчики уже поправили.
template<class T>
class FastVec
{
....
public:
FastVec &operator=(FastVec &&other)
{
if (this != &other)
{
delete[] data_; // <=
data_ = other.data_;
size_ = other.size_;
capacity_ = other.capacity_;
other.data_ = nullptr;
other.size_ = 0;
other.capacity_ = 0;
}
return *this;
}
....
private:
void IncreaseCapacityTo(size_t newCapacity)
{
....
T *oldData = data_;
data_ = (T *)malloc(sizeof(T) * newCapacity); // <=
_assert_msg_(data_ != nullptr, "%d", (int)newCapacity);
if (capacity_ != 0)
{
memcpy(data_, oldData, sizeof(T) * size_);
free(oldData);
}
capacity_ = newCapacity;
}
....
}Предупреждение PVS-Studio: V611 The memory was allocated using 'malloc/realloc' function but was released using the 'delete' operator. Consider inspecting operation logics behind the 'data_' variable. Check lines: 51, 158. FastVec.h 51
В проекте имеется самописный быстрый (судя по названию) вектор. Он имеет перегруженный оператор перемещения, который выполняет освобождение буфера через оператор delete[].
Далее мы видим функцию IncreaseCapacityTo, которая увеличивает ёмкость вектора до необходимого значения. Делает она это при помощи функций malloc и free — новый кусок памяти выделяется, из старого куска памяти копируются данные, и затем он освобождается.
А теперь сюжетный твист. Если кто-то захочет переместить один вектор в другой, то память, которая была выделена через malloc, будет удаляться через оператор delete[]. При таком несогласованном использовании функций для работы с памятью поведение программы не определено.
Авторы уже поправили код следующим образом:
FastVec &operator=(FastVec &&other)
{
if (this != &other)
{
free(data_); // <=
....
}
return *this;
}Код был заложен в коммите 956d784 (23 мая 2023 года), а исправлен в 36b7875 (2 апреля 2024 года). Баг ждал своего часа чуть больше года.
Фрагмент N2
VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference,
size_t *size)
{
ZipFileReaderFileReference *reference =
(ZipFileReaderFileReference *)vfsReference;
ZipFileReaderOpenFile *openFile = new ZipFileReaderOpenFile();
....
if (zip_stat_index(zip_file_, reference->zi, 0, &zstat) != 0)
{
lock_.unlock();
delete openFile;
return nullptr;
}
openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0);
if (!openFile->zf)
{
WARN_LOG(G3D, "File with index %d not found in zip", reference->zi);
lock_.unlock();
return nullptr; // <=
}
*size = zstat.size;
// Intentionally leaving the mutex locked, will be closed in CloseFile.
return openFile;
}Предупреждение PVS-Studio V773 The function was exited without releasing the 'openFile' pointer. A memory leak is possible. ZipFileReader.cpp 284
Анализатор указывает на второй return данной функции. И действительно можно заметить, что в теле второго условия при выходе из функции забывают подчистить выделенный кусок памяти (в отличие от тела первого условия).
Самый простой вариант исправления, который также сделали авторы проекта:
....
openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0);
if (!openFile->zf)
{
WARN_LOG(G3D, "File with index %d not found in zip", reference->zi);
lock_.unlock();
delete openFile;
return nullptr;
}
....Однако такой подход плох в том, что нужно постоянно думать о ранних возвратах из функции. Поэтому я бы рекомендовал использование умных указателей (std::unique_ptr):
VFSOpenFile *ZipFileReader::OpenFileForRead
(VFSFileReference *vfsReference, size_t *size)
{
ZipFileReaderFileReference *reference =
(ZipFileReaderFileReference *)vfsReference;
auto openFile = std::make_unique<ZipFileReaderOpenFile>();
....
// Intentionally leaving the mutex locked, will be closed in CloseFile.
return openFile.release();
}Применение std::unique_ptr позволяет не задумываться о ручной очистке памяти, а также делает код читабельнее и лаконичнее. При этом платить за него не придётся.
Код был заложен в коммите 97cf5f8 (7 марта 2023 года), а исправлен в 9c3c23d (2 апреля 2024 года). Ещё один баг ждал своего часа чуть больше года :)
Фрагмент N3
bool ARM64FloatEmitter::TryAnyMOVI(u8 size,
ARM64Reg Rd,
uint64_t elementValue)
{
// Try the original size first in case that's more optimal.
if (TryMOVI(size, Rd, elementValue))
return true;
if (size != 64)
{
uint64_t masked = elementValue & ((1 << size) - 1);
for (int i = size; i < 64; ++i)
{
value |= masked << i;
}
}
....
}Предупреждение PVS-Studio: V629 Consider inspecting the '1 << size' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. Arm64Emitter.cpp 4298
Функция ARM64FloatEmitter::TryMOVI ограничивает переменную size следующими значениями: 16, 32 или 64. Также у нас есть проверка, что size не равен 64. Отлично, остаётся только 16 или 32.
Теперь обратим внимание на декларацию переменной masked. В этой строке есть аж две проблемы.
Начнём с побитового сдвига. При size == 16 ничего криминального не произойдёт, а вот при size == 32 может. Мы сдвигаем литерал '1', который имеет тип int. Размер int определяется реализацией, однако на многих платформах он равен 32 битам. Получаем выражение 1 << 32. К сожалению, поведение в таком случае не определено.
Для исправления достаточно изменить тип литерала при сдвиге, что сделали и авторы проекта:
uint64_t masked = elementValue & ((1ULL << size) – 1ULL);
Но это ещё не всё. Давайте внимательнее посмотрим на операцию побитового "И". Допустим, что сдвиг единицы влево работает корректно, и все 32 бита заполнены правильно. Вот только мы делаем побитовое "И" с 64 битной переменной. По стандарту правый операнд должен будет расшириться до 64 бит. Если в результате сдвига получается положительное число, то при расширении старшие биты будут нулевыми. Соответственно, побитовое "И" занулит старшую часть переменной elementValue. Вряд ли это то, чего хотел разработчик.
Код был заложен в коммите 00e691d (11 сентября 2023 года), а исправлен в 81ef166 (4 апреля 2024 года). В данном случае прошло чуть больше полугода.
Фрагмент N4
void Buffer::Printf(const char *fmt, ...)
{
....
size_t retval = vsnprintf(buffer, sizeof(buffer), fmt, vl);
if ((int)retval >= (int)sizeof(buffer))
{
// Output was truncated. TODO: Do something.
ERROR_LOG(IO, "Buffer::Printf truncated output");
}
if (retval < 0) // <=
{
ERROR_LOG(IO, "Buffer::Printf failed");
}
....
}Предупреждение PVS-Studio: V547 Expression 'retval < 0' is always false. Unsigned type value is never < 0. Buffer.cpp 114
Функция vsnprintf возвращает (тип int):
число символов, которое было бы записано, если бы не было ограничения, переданного вторым параметром;
отрицательное значение в случае неудачи.
Результат выполнения сохраняется в беззнаковую переменную retval. В первом условии if решили проверить, что результирующая строка была записана, и сделали это верно, преобразовав операнды к int. А вот во второй проверке это сделать забыли, из-за чего никогда не будет выполнен ERROR_LOG.
Код был заложен в коммите 1e22966 (1 мая 2021 года), а исправлен в 8e58004 (11 апреля 2024 года). Рекордсмен — прятался почти 3 года :)
Возможные баги будущего релиза
А теперь мы рассмотрим баги, которые остались в проекте до сих пор. Мы обязательно сообщим разработчикам о них и надеемся, что их успеют поправить до следующего релиза.
Неправильная работа с памятью
Начать хотелось бы с ошибок, связанных с одной из главных особенностей языка, которая является одновременно его плюсом и минусом — свободной работе с памятью.
Фрагменты N5
Потенциальное разыменование нулевого указателя.
void FramebufferManagerCommon::ReadFramebufferToMemory
(VirtualFramebuffer *vfb, int x, int y, int w, int h,
RasterChannel channel, Draw::ReadbackMode mode)
{
// Clamp to bufferWidth. Sometimes block transfers can cause this to hit.
if (x + w >= vfb->bufferWidth)
{
w = vfb->bufferWidth - x;
}
if (vfb && vfb->fbo)
{
....
}
....
}Предупреждение PVS-Studio: V595 The 'vfb' pointer was utilized before it was verified against nullptr. Check lines: 3155, 3157. FramebufferManagerCommon.cpp 3155
Сначала в условной конструкции мы разыменовываем указатель vfb, а затем несколькими строками ниже проверяем, не является ли он нулевым.
Думаю, даже если у разработчиков возник вопрос: "А не является ли указатель нулевым?", то возможна ситуация, когда он действительно может быть нулевым. Поэтому имеет смысл поднять проверку до разыменования указателя. Или другой вариант — убрать проверку, если указатель никогда не может быть нулевым.
Ещё одно подобное срабатывание:
V595 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 1331, 1346. VulkanRenderManager.cpp 1331
Фрагмент N6
Интересный кейс ошибки при использовании функции для работы с памятью.
int sceNetAdhocMatchingSetHelloOpt(int matchingId,
int optLenAddr, u32 optDataAddr)
{
....
if (optLenAddr > context->hellolen)
{
hello = realloc(hello, optLenAddr);
}
....
}С первого взгляда в этом коде всё в порядке, и в 99% случаев программа будет работать нормально. До тех пор, пока realloc не сможет удовлетворить запрос программиста. Обратимся к документации функции realloc:
> If there is not enough memory, the old memory block is not freed and null pointer is returned.
То есть, если при реалокации не хватит памяти, мы запишем в указатель hello нулевой указатель, и произойдёт утечка. Об этом и сообщает анализатор:
Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'hello' is lost. Consider assigning realloc() to a temporary pointer. sceNetAdhoc.cpp 5407
Фрагмент N7
Этот фрагмент мог бы попасть в раздел с необязательными проверками условий, однако здесь присутствует неправильная работа с функцией выделения памяти. Так что я отнёс его сюда.
u32 __MicInput(u32 maxSamples, u32 sampleRate, u32 bufAddr,
MICTYPE type, bool block)
{
....
if (!audioBuf)
{
audioBuf = new QueueBuf(size);
}
else
{
audioBuf->resize(size);
}
if (!audioBuf)
return 0;
....
}Предупреждение PVS-Studio: V668 There is no sense in testing the 'audioBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sceUsbMic.cpp 432
Думаю, сообщение диагностического правила V668 весьма красноречиво, но всё же: оператор new не может вернуть нулевой указатель, поэтому проверять его не имеет смысла. Вероятнее всего, раньше здесь был malloc, и после замены на new проверку просто не убрали.
Побитовые сдвиги
В данном разделе посмотрим на ошибки, связанные с побитовыми сдвигами.
Фрагмент N8
Фрагмент с потенциальным UB.
void ARMXEmitter::VMOVN(u32 Size, ARMReg Vd, ARMReg Vm)
{
....
_dbg_assert_msg_((Size & I_8) == 0, "%s cannot narrow from I_8",
__FUNCTION__);
// For consistency with assembler syntax and VMOVL - encode one size down.
int halfSize = encodedSize(Size) - 1;
Write32( (0xF3B << 20) | (halfSize << 18) | (1 << 17)
| EncodeVd(Vd) | (1 << 9) | EncodeVm(Vm));
}Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('halfSize' = [-1..2]). ArmEmitter.cpp 2870
Для начала разберёмся с возможными значениями переменной halfSize, на которую указывает анализатор. Мы видим, что её значение — это результат работы функции encodedSize минус единица.
А вот и функция encodedSize:
u32 encodedSize(u32 value)
{
if (value & I_8)
return 0;
else if (value & I_16)
return 1;
else if ((value & I_32) || (value & F_32))
return 2;
else if (value & I_64)
return 3;
else
_dbg_assert_msg_(false, "Passed invalid size to integer NEON instruction");
return 0;
}
Как мы видим, возможное значение *halfSize* действительно имеет диапазон [-1..2]. А значит, возможна ситуация со сдвигом влево на значение -1, что приведёт к неопределённому поведению. И, к сожалению, проверка в _dbg_assert_msg_ не поможет, т.к. этот макрос разворачивается в пустоту на релизе.
Подобные срабатывания:
V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('halfSize' = [-1..2]). ArmEmitter.cpp 2884
V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('halfSize' = [-1..2]). ArmEmitter.cpp 2897
V610 Undefined behavior. Check the shift operator '<<'. The right operand is negative ('(n - 1)' = [-1..3]). MIPSIntVFPU.cpp 2149
Необязательные проверки условий
Перейдём к ситуациям, где разработчики совершили избыточные проверки либо в результате неправильного вычисления логических выражений, либо в качестве подстраховки.
Фрагменты N9-10
Я обещал, что будут показаны баги такого же паттерна, который уже исправляли (фрагмент N4). Вот первый из них:
void WebSocketInputState::ButtonsPress(DebuggerRequest &req)
{
....
PressInfo press;
press.duration = 1;
if (!req.ParamU32("duration", &press.duration, false,
DebuggerParamType::OPTIONAL))
return;
if (press.duration < 0)
return req.Fail("Parameter 'duration' must not be negative");
....
}Предупреждение PVS-Studio: V547 Expression 'press.duration < 0' is always false. Unsigned type value is never < 0. InputSubscriber.cpp 179
Давайте посмотрим, что из себя представляет press.duration:
struct WebSocketInputState : public DebuggerSubscriber
{
....
protected:
struct PressInfo
{
std::string ticket;
uint32_t button;
uint32_t duration; // <=
std::string Event();
};
....
};Суть примерно та же, что и раньше. Перед нами беззнаковая целочисленная переменная. В неё записывается некоторое значение, которое не должно быть отрицательным. Возможно, что раньше переменная была знакового типа, и проверку забыли поправить.
А вот еще один: V547 Expression 'press.duration < 0' is always false. Unsigned type value is never < 0. InputSubscriber.cpp 208
Фрагмент N11
Данный фрагмент отлично демонстрирует ещё один из плюсов статических анализаторов. Вот смотришь на код ниже и понимаешь, что дольше нескольких секунд на него смотреть невозможно. Глазу даже зацепиться не за что, а уж найти в нём ошибку... Однако для анализаторов это пустяковая работа.
void XEmitter::ABI_CallFunctionRR(const void *func, X64Reg reg1, X64Reg reg2)
{
if (reg2 != ABI_PARAM1)
{
if (reg1 != ABI_PARAM1)
MOV(64, R(ABI_PARAM1), R(reg1));
if (reg2 != ABI_PARAM2)
MOV(64, R(ABI_PARAM2), R(reg2));
}
else
{
if (reg2 != ABI_PARAM2)
MOV(64, R(ABI_PARAM2), R(reg2));
if (reg1 != ABI_PARAM1)
MOV(64, R(ABI_PARAM1), R(reg1));
}
}Предупреждение PVS-Studio: V547 Expression 'reg2 != RSI' is always true. ABI.cpp 465
Отмечу, что RSI — это значение, прячущееся под макросом ABI_PARAM2. В ветвь else мы попадаем, если reg2 == ABI_PARAM1. А раз так, то первая проверка в ветке else всегда истинная, и код можно упростить:
else
{
MOV(64, R(ABI_PARAM2), R(reg2));
if (reg1 != ABI_PARAM1)
MOV(64, R(ABI_PARAM1), R(reg1));
}Фрагмент N12
Немного другой случай избыточной проверки.
void netValidateLoopMemory()
{
// Allocate Memory if it wasn't valid/allocated
// after loaded from old SaveState
if ( !apctlThreadHackAddr
|| ( apctlThreadHackAddr
&& strcmp("apctlThreadHack",
kernelMemory.GetBlockTag(apctlThreadHackAddr)) != 0))
{
....
}
}Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!apctlThreadHackAddr' and 'apctlThreadHackAddr'. sceNet.cpp 257
Вспомним немного алгебру логики. Представим, что у нас есть выражение:
¬A V (A ∧ B)Воспользуемся таким свойством как дистрибутивность. Тогда из нашего выражения мы получим следующее:
(¬A V A) ∧ (¬A V B)Выражение (¬A V A) всегда истинно. Таким образом остается только:
¬A V BА значит вторая проверка apctlThreadHackAddr действительно лишняя, и код можно упростить:
void netValidateLoopMemory()
{
// Allocate Memory if it wasn't valid/allocated
// after loaded from old SaveState
if ( !apctlThreadHackAddr
|| strcmp("apctlThreadHack",
kernelMemory.GetBlockTag(apctlThreadHackAddr)) != 0)
{
....
}
}Фрагмент N13
Ещё один пример интересного избыточного усложнения проверки.
bool ApplyMemoryValidation(const IRWriter &in, IRWriter &out,
const IROptions &opts)
{
....
for (IRInst inst : in.GetInstructions())
{
IRMemoryOpInfo info = IROpMemoryAccessSize(inst.op);
// Note: we only combine word aligned accesses.
if (info.size != 0 && inst.src1 == MIPS_REG_SP && info.size == 4) // <=
{
....
}
....
}
....
}Предупреждение PVS-Studio: V590 Consider inspecting this expression. The expression is excessive or contains a misprint. IRPassSimplify.cpp 1792
В проверке происходит явно что-то странное. Если info.size == 4, то проверка info.size != 0 явно лишняя. Вполне возможно, что надо было проверить что-то другое. Ну или можно упростить проверку.
Снижение производительности
Понятно, что и срабатывания из предыдущего раздела можно было отнести сюда. Однако здесь находятся срабатывания из оптимизационной группы диагностических правил.
Рассмотрим следующий фрагмент:
Фрагмент N14
void JsonWriter::pushArray()
{
str_ << arrayComma() << arrayIndent() << "[";
stack_.back().first = false;
stack_.push_back(StackEntry(ARRAY));
}Предупреждение PVS-Studio: V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 101
Анализатор предлагает заменить вызов функции *push_back* на emplace_back у контейнера stack_. Что ж, давайте посмотрим, что представляют из себя переменная *stack_* и тип StackEntry.
class JsonWriter
{
private:
....
struct StackEntry
{
StackEntry(BlockType t) : type(t), first(true) {}
BlockType type;
bool first;
};
std::vector<StackEntry> stack_;
....Как мы видим, переменная stack_ — это обычный std::vector, а StackEntry — это структура, которая имеет конвертирующий конструктор. Соответственно, код можно сделать немного эффективнее, если заменить push_back на emplace_back.
Что происходит в случае push_back:
Вызывается конструктор StackEntry, формируя временный объект.
Временный объект передаётся по rvalue-ссылке в push_back.
Вычисляется адрес, по которому надо сконструировать объект в векторе с помощью placement new.
В placement new перемещается временный объект, переданный по rvalue-ссылке.
Вызывается неявный конструктор перемещения, который копирует 2 поля.
Что произойдёт в случае замены на emplace_back:
Параметры конструктора StackEntry идеально передаются в emplace_back. Никакого временного объекта не создаётся.
Вычисляется адрес, по которому надо сконструировать объект в векторе с помощью placement new.
В placement new идеально передаются аргументы emplace_back, объект сразу конструируется в векторе.
Как видно, emplace_back в этом случае позволяет сэкономить вызов конструктора перемещения. Кто-то может сказать, что это капля в море, но мы же с вами C++ разработчики, не так ли? Битва за производительность не заканчивается никогда :)
Аналогичные срабатывания:
V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 22
V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 27
V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 32
V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 87
V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 108
V823 Decreased performance. Object may be created in-place in the 'callbacks_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. VulkanContext.h 123
V823 Decreased performance. Object may be created in-place in the 'compileQueue_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. VulkanRenderManager.cpp 876
V823 Decreased performance. Object may be created in-place in the 'threads_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. HTTPServer.cpp 48
V823 Decreased performance. Object may be created in-place in the 'ip_ranges' container. Consider replacing methods: 'push_back' -> 'emplace_back'. proAdhoc.cpp 1896
Фрагмент N15
bool LoadRemoteFileList(const Path &url, const std::string &userAgent,
bool *cancel, std::vector<File::FileInfo> &files)
{
....
std::string contentType;
for (const std::string &header : responseHeaders)
{
if (startsWithNoCase(header, "Content-Type:"))
{
contentType = header.substr(strlen("Content-Type:"));
// Strip any whitespace (TODO: maybe move this to stringutil?)
contentType.erase(0, contentType.find_first_not_of(" \t\r\n"));
contentType.erase(contentType.find_last_not_of(" \t\r\n") + 1);
}
}
....
}Предупреждение PVS-Studio: V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. PathBrowser.cpp 58
Увидев предупреждение, читатель может сказать: "Компиляторы уже прекрасно умеют оптимизировать множественные вызовы strlen в цикле". И да, и нет. На самом деле, я хотел бы заострить внимание на другом моменте.
Благодаря этому предупреждению был найден достаточно странный код. Что же он делает:
Итерируемся по всем строкам в векторе responseHeaders.
В каждой строке проверяем, что она начинается с заголовка "Content-Type:".
Если это так, то сохраняем во внешнюю для цикла переменную contentType подстроку без заголовка, а также убираем пробельные символы.
И вот это компилятор уже вряд ли сможет оптимизировать, убрав цикл :) Возможно, что цикл надо было остановить, как только первый заголовок был встречен.
Ну и по поводу strlen: давайте не будем его забывать. Можно вынести этот строковый литерал в std::string_view, и на этапе компиляции гарантированно будет подставляться константное значение:
....
constexpr std::string_view ContentTypeHeader = "Content-Type:";
std::string contentType;
for (const std::string &header : responseHeaders)
{
if (startsWithNoCase(header, ContentTypeHeader))
{
contentType = header.substr(ContentTypeHeader.size());
....
}
}
....Фрагмент N16
А в этом фрагменте всё гораздо хуже, чем в предыдущих.
bool ZipFileReader::GetFileListing
(const char *orig_path, std::vector<File::FileInfo> *listing,
const char *filter = 0)
{
....
for (auto fiter = files.begin(); fiter != files.end(); ++fiter)
{
std::string fpath = path;
File::FileInfo info;
info.name = *fiter;
std::string relativePath = std::string(path).substr(inZipPath_.size());
info.fullName = Path(relativePath + *fiter);
info.exists = true;
info.isWritable = false;
info.isDirectory = false;
std::string ext = info.fullName.GetFileExtension();
if (filter)
{
if (filters.find(ext) == filters.end())
continue;
}
listing->push_back(info);
}
....
}Предупреждение PVS-Studio: V808 'fpath' object of 'basic_string' type was created but was not utilized. ZipFileReader.cpp 128
Посмотрев на текст предупреждения, я начал искать, а где же должна была использоваться переменная fpath. То, что я увидел, мягко говоря, "удивило" меня.
Начнём с того, что мы действительно на каждой итерации цикла создаём объект типа std::string, который не используем (переменная fpath).
Дальше обратим внимание на предполагаемое место использования fpath:
std::string relativePath = std::string(path).substr(inZipPath_.size());Скорее всего, функция substr должна была зваться именно на fpath. А вместо этого мы создаём ещё один временный объект типа std::string.
Потом *substr* создаёт ещё одну копию строки. И лишь затем всё это помещается в переменную relativePath.
В ещё одном цикле чуть выше по коду происходит то же самое.
И как вишенка на торте: все вычисления можно было сделать один раз, ведь переменные, от которых мы всё вычисляли, никак не меняются в цикле.
Разное
В данном разделе представлены все остальные срабатывания анализатора, которые сложно отнести к категориям выше.
Фрагмент N17
После прошлого фрагмента вас будет сложно удивить, но я попробую.
MockPSP::MockPSP(UI::LayoutParams *layoutParams) : AnchorLayout(layoutParams)
{
....
AddButton(CTRL_RTRIGGER, ImageID("I_R"), ImageID("I_SHOULDER_LINE"), 0.0f,
LayoutSize(50.0f, 16.0f, 397.0f, 0.0f))->SetFlipHBG(true);
....
}Предупреждение PVS-Studio: V601 The bool type is implicitly cast to the float type. Inspect the first argument. ControlMappingScreen.cpp 810
Перед нами вызов функции AddButton. Но она нам не интересна. Обратите внимание на вызов функции SetFlipHBG(true) внутри. Всё дело в том, что на самом деле она принимает параметр типа float:
MockButton *SetFlipHBG(float f);Как в неё попал флаг true, остаётся загадкой.
Фрагмент N18
А закончить хочется на серьезной ноте с ошибкой в функции, связанной с безопасностью. "А что можно защищать в эмуляторе игровой приставки?" — подумаете вы? И вопрос скорее всего правильный. Но всё же перед нами функция с названием sha1_hmac.
Для тех, кто не очень хорошо знаком с криптографическими алгоритмами
HMAC (сокращение от англ. hash-based message authentication code) — это механизм проверки целостности информации, позволяющий гарантировать то, что данные не были изменены кем-то посторонним.
Реализация HMAC является обязательной для протокола IPsec. Также HMAC используется в других протоколах интернета, например, TLS.
void sha1_hmac( unsigned char *key, int keylen,
unsigned char *input, int ilen,
unsigned char output[20] )
{
sha1_context ctx;
sha1_hmac_starts( &ctx, key, keylen );
sha1_hmac_update( &ctx, input, ilen );
sha1_hmac_finish( &ctx, output );
memset( &ctx, 0, sizeof( sha1_context ) );
}Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s() function should be used to erase the private data. md5.cp p 290
Проблема, на которую указывает анализатор, известна уже очень давно. Если коротко, то компиляторы во время оптимизации могут удалить записи в память без последующего чтения (Dead Store Elimination). В нашем случае такой записью будет считаться вызов memset, который должен очистить приватную информацию в объекте ctx. Подробнее о проблеме можно почитать тут.
На самом деле заставить компилятор не удалять очистку приватных данных — задача нетривиальная. В этом проползале отражены все доступные на данный момент способы явного зануления памяти (секция "The problem").
Скоро мы будем решать её с помощью функции memset_explicit, которую добавили в C23. В C++26 также есть все шансы увидеть реализацию этой функции, но в виде шаблона функции. Исправленный код будет выглядеть следующим способом:
void sha1_hmac( unsigned char *key, int keylen,
unsigned char *input, int ilen,
unsigned char output[20] )
{
sha1_context ctx;
sha1_hmac_starts( &ctx, key, keylen );
sha1_hmac_update( &ctx, input, ilen );
sha1_hmac_finish( &ctx, output );
// won't be optimized out
#ifdef __cplusplus
memset_explicit( ctx );
#else
memset_explicit( &ctx, 0, sizeof( sha1_context ) );
#endif
}Аналогичные предупреждения:
V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The memset_s() function should be used to erase the private data. sha1.cpp 379
V597 The compiler could delete the 'memset' function call, which is used to flush 'sum' buffer. The memset_s() function should be used to erase the private data. sha1.cpp 355
V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s() function should be used to erase the private data. sha1.cpp 325
V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s() function should be used to erase the private data. md5.cpp 359
V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The memset_s() function should be used to erase the private data. md5.cpp 344
V597 The compiler could delete the 'memset' function call, which is used to flush 'sum' buffer. The memset_s() function should be used to erase the private data. md5.cpp 320
V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s() function should be used to erase the private data. md5.cpp 290
Заключение
В этой статье приведён далеко не весь список обнаруженных ошибок. Поэтому про оставшуюся их часть выйдет отдельная статья.
Но даже из того, что мы посмотрели, можно сделать следующие выводы:
Ошибки, попадая в проект, могут находиться там несколько лет (мы увидели даже ошибку 3-х летней давности).
Повезёт, если кто-то сообщит о них и их сразу удастся исправить. И то — это не гарантирует, что вы исправите все места с такой же ошибкой.
PVS-Studio выдал действительно полезные предупреждения, ведь то, что о некоторых из них уже сообщили и исправили, де-факто подтверждает это.
Ну и напоследок хочется высказать такую мысль: статические анализаторы позволяют выявлять баги на ранних этапах разработки, до того, как код попадёт в релиз. Это особенно важно для крупных проектов, таких как PPSSPP, где участие большого количества разработчиков может привести к возникновению сложных и трудно обнаруживаемых проблем.
Но нахождение ошибок на раннем этапе — далеко не единственное преимущество статических анализаторов. Сюда также можно отнести:
Сокращение времени и стоимости исправления ошибок;
Высвобождение ресурсов для решения бизнес-задач;
Контроль качества;
Поддержку различных стандартов кодирования;
И многое другое.
У PVS-Studio существуют различные сценарии бесплатного использования. А для коммерческих компаний получить пробную версию анализатора PVS-Studio можно здесь.
Спасибо за внимание и до новых встреч!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Smolskas. PPSSPP or PSP?.
