Думаю, многих из нас охватывает тёплое чувство ностальгии, когда речь идёт о PSP. Эта портативная консоль, выпущенная в 2004 году, стала настоящим прорывом для своего времени. Она подарила нам возможность наслаждаться полноценными игровыми проектами в дороге, что было невероятно новаторским для того периода. У вас словно была маленькая частичка домашней консоли в кармане.
Игры для PSP были разнообразными и захватывающими: от культовых серий, таких как "God of War" и "Final Fantasy", до уникальных проектов вроде "Patapon" и "LocoRoco". Каждая игра открывала новый мир, который можно было исследовать где угодно — в автобусе, на перемене в школе или просто сидя в парке. PSP также была первой консолью Sony с возможностью воспроизведения мультимедиа, что делало её универсальным устройством для развлечений.
С появлением эмуляторов для PSP эта ностальгия обрела вторую жизнь. Эмуляторы позволяют современным игрокам вновь окунуться в те незабываемые времена или открыть для себя игры, которые они пропустили. Они дают возможность запускать классические проекты на современных устройствах с улучшенной графикой и производительностью. Это особенно важно в эпоху цифровой дистрибуции, когда физические копии игр становятся редкостью.
Эмуляторы также играют важную роль в сохранении игровой истории. Они помогают не потерять доступ к играм, которые могут быть недоступны из-за устаревшего оборудования или ограниченного тиража. Благодаря сообществу энтузиастов и разработчиков эмуляторов мы можем быть уверены, что наследие PSP будет жить ещё долго.
PPSSPP — это эмулятор, который позволяет запускать игры от портативной консоли PlayStation Portable (PSP) на различных устройствах, включая ПК, смартфоны и планшеты. Он был создан Хенриком Ридгардом и впервые выпущен в 2012 году. С тех пор PPSSPP стал одним из самых популярных эмуляторов для PSP благодаря своей высокой совместимости и производительности.
Основные особенности PPSSPP:
В целом, 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, а затем несколькими строками ниже проверяем, не является ли он нулевым.
Думаю, даже если у разработчиков возник вопрос: "А не является ли указатель нулевым?", то возможна ситуация, когда он действительно может быть нулевым. Поэтому имеет смысл поднять проверку до разыменования указателя. Или другой вариант — убрать проверку, если указатель никогда не может быть нулевым.
Ещё одно подобное срабатывание:
Фрагмент 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_ не поможет, т.к. этот макрос разворачивается в пустоту на релизе.
Подобные срабатывания:
Перейдём к ситуациям, где разработчики совершили избыточные проверки либо в результате неправильного вычисления логических выражений, либо в качестве подстраховки.
Фрагменты 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:
Что произойдёт в случае замены на emplace_back:
Как видно, emplace_back в этом случае позволяет сэкономить вызов конструктора перемещения. Кто-то может сказать, что это капля в море, но мы же с вами C++ разработчики, не так ли? Битва за производительность не заканчивается никогда :)
Аналогичные срабатывания:
Фрагмент 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 в цикле". И да, и нет. На самом деле, я хотел бы заострить внимание на другом моменте.
Благодаря этому предупреждению был найден достаточно странный код. Что же он делает:
И вот это компилятор уже вряд ли сможет оптимизировать, убрав цикл :) Возможно, что цикл надо было остановить, как только первый заголовок был встречен.
Ну и по поводу 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(strlen(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
}Аналогичные предупреждения:
В этой статье приведён далеко не весь список обнаруженных ошибок. Поэтому про оставшуюся их часть выйдет отдельная статья.
Но даже из того, что мы посмотрели, можно сделать следующие выводы:
Ну и напоследок хочется высказать такую мысль: статические анализаторы позволяют выявлять баги на ранних этапах разработки, до того, как код попадёт в релиз. Это особенно важно для крупных проектов, таких как PPSSPP, где участие большого количества разработчиков может привести к возникновению сложных и трудно обнаруживаемых проблем.
Но нахождение ошибок на раннем этапе — далеко не единственное преимущество статических анализаторов. Сюда также можно отнести:
У PVS-Studio существуют различные сценарии бесплатного использования. А для коммерческих компаний получить пробную версию анализатора PVS-Studio можно здесь.
Спасибо за внимание и до новых встреч!