Недавно в сети появилась новость о том, что был открыт исходный код игры "Приключения капитана Блада". Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Потонет ли легендарный корабль капитана Блада от найденных багов? Давайте узнаем!
Игра "Приключения капитана Блада" создана студией "1C Морской волк" по мотивам книг Рафаэля Сабатини. Она повествует о приключениях главного героя этих произведений, капитана Питера Блада. Действие игры разворачивается в средневековой новой Англии. Главный герой, военный врач по образованию, был несправедливо осуждён и сослан на остров Барбадос. С острова ему удаётся бежать, после чего, захватив испанский корабль, он становится самым знаменитым пиратом того времени.
Проект был завершён в 2010 году, но так и не увидел свет. Всё потому, что компании 1С и Playlogic не смогли решить, кому принадлежат права на игру. Далее ходили новости, что финальная сборка игры безвозвратно утеряна. Однако не так давно появилось несколько новостей о том, что это не так. Появилось видео с прохождением на YouTube и работающий билд игры на одном известном трекере. Позже на GitHub выложили исходники под лицензией GPLv3.
Итак, после того как мы познакомились с проектом, предлагаю посмотреть какие интересные ошибки в нём удалось обнаружить.
Предупреждение N1
V1075 The function expects the file to be opened for writing, but it was opened for reading. Debugger.cpp 172
Можем поздравить моего коллегу Сергея Ларина с первым трофейным срабатыванием его диагностического правила, вышедшего в релизе PVS-Studio 7.15. Данное диагностическое правило ругается на запись в файлы, открытые для чтения, и наоборот. Честно, мы даже не думали, что оно когда-нибудь сработает. Приведу проблемную функцию полностью:
void appDebuger::CopyDump(const char * src, DWORD from, const char * dst)
{
FILE * file = fopen(src, "rb");
if(file)
{
fseek(file, 0, SEEK_END);
DWORD size = ftell(file);
fseek(file, from, SEEK_SET);
if(size > from)
{
FILE * to = fopen(dst, "a+b");
if(to)
{
char buf[128];
while(from < size)
{
DWORD s = size - from;
if(s > sizeof(buf)) s = sizeof(buf);
memset(buf, ' ', sizeof(buf));
fread(buf, s, 1, file);
fwrite(buf, s, 1, file); // <=
from += s;
}
fclose(to);
}
}
fclose(file);
}
}Итак, функция должна прочитать данные из одного файла по пути src, начиная с позиции from, и записать их в другой файл по пути dst. За исходный файл отвечает переменная file, за результирующий файл переменная to.
К сожалению, в коде происходит чтение из переменной file и запись в переменную file. Поправить код можно таким образом:
fwrite(buf, s, 1, to);Предупреждение N2
V1065 [CWE-682] Expression can be simplified, check 'bi' and similar operands. PreviewAnimation.cpp 146
PreviewAnimation::PreviewAnimation(....)
{
// ....
const int bw = 30;
const int bh = 30;
const int bi = 3;
const int cn = 9;
bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + (bw + bi) + 7 - bi + 1;
// ....
}А это одно из моих самых любимых диагностических правил! Оно говорит нам о том, что в представленном выражении можно сократить переменную bi:
bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + bw + 7 + 1;Возможно, на месте одного из bi должна быть какая-то другая переменная. Или же выражение (bw + bi) забыли на что-то умножить. Интересно, а как бы поменялись анимации в игре после этого?
Также, отмечу, что давать подобные имена константам: bw, bh, bi, cn – плохая практика. Это запутывает код для читающего его человека. Куда уж лучше было бы назвать переменные осмысленнее или хотя бы сопроводить их определения поясняющими комментариями.
Предупреждение N3
V570 [CWE-480] The 'ldMaxChars' variable is assigned to itself. StringAttr.cpp 198
void StringAttribute::LoadFromFile (....)
{
int ldMinChars, ldMaxChars;
// ....
minChars = ldMinChars;
ldMaxChars = ldMaxChars;
// ....
}
class StringAttribute : public BaseAttribute
{
int minChars;
int maxChars;
}Тут мы видим классическое присваивание переменной самой себе. Чтобы посмотреть, что должно быть на месте переменной, в которую происходит присваивание, заглянем в определение класса StringAttribute. В нём видно похожее поле maxChars. Очевидно, что программист опечатался и на самом деле должен был присвоить переменной значение ldMaxChars. Поправленный фрагмент кода:
void StringAttribute::LoadFromFile (....)
{
int ldMinChars, ldMaxChars;
// ....
minChars = ldMinChars;
maxChars = ldMaxChars;
// ....
}Предупреждение N4
V781 [CWE-20, CERT-API00-C] The value of the 'start' index is checked after it was used. Perhaps there is a mistake in program logic. coreloader.h 136
__forceinline void GetBootParams(const char * bootIni)
{
long start = 0;
// ....
if (bootIni[start] && start > 0)
{
// ....
}
// ....
}В данном фрагменте кода программист сначала использовал значение start для индексации по массиву bootIni, а только затем проверил его на больше 0. Проверка может быть бессмысленна, тогда стоит убрать её. Также она может иметь смысл, тогда её стоит поменять местами с операцией взятия значения по индексу.
if (start > 0 && bootIni[start])
{
// ....
}В противном случае, программист рискует уронить игру. Кстати, таких срабатываний в программе было довольно много. Вот несколько, на которые стоит обратить внимание в первую очередь:
Предупреждение N5
V700 [CWE-480] Consider inspecting the 'mView = mView = chr.Render().GetView()' expression. It is odd that variable is initialized through itself. CharacterItems.cpp 705
void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld)
{
// ....
Matrix mView = mView = chr.Render().GetView();
// ....
}Странный код с присваиванием значения переменной самого себе. Скорее всего произошла опечатка. В любом случае, избыточное присваивание можно убрать:
void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld)
{
// ....
Matrix mView = chr.Render().GetView();
// ....
}Предупреждение N6
V654 [CWE-834] The condition 'n >= 0' of loop is always true. resourceselect.cpp 42
typedef unsigned int dword;
TResourceSelectorWindow::TResourceSelectorWindow ()
{
string PakPath;
// ....
miss->EditorGetPackPath(PakPath);
//....
for (dword n = PakPath.Size()-1; n >= 0; n--)
{
if (PakPath[n] == '\\')
{
PakPath.Delete(0, n+1);
break;
}
}
// ....
}Здесь мы видим потенциально бесконечный цикл, если в строчке нет символа \. Переменная n беззнакового типа, а значит, она всегда будет больше либо равна 0.
Но на самом деле никакого вечного цикла не будет, так как когда возникнет переполнение переменной n произойдёт сбой. Переполнение беззнаковой переменной не приводит к неопределённому поведению, но зато к нему приведёт доступ за пределами массива. На практике скорее всего возникнет Access Violation.
Тут программист написал алгоритм, который ищет первое вхождение символа \ с конца и удаляет подстроку от начала до этого символа включительно. Самым корректным исправлением было бы использование rbegin и rend, но есть нюанс – это самописная строка (аналог std::string с дополнительным функционалом), в которой, к сожалению, нет итераторов. Тогда, чтобы поправить код мы можем взять знаковую переменную с размерностью в 2 раза больше (int64_t) и пустить цикл по ней.
for (int64_t n = PakPath.Size() - 1; n >= 0; n--)
{
if (PakPath[n] == '\\')
{
PakPath.Delete(0, n+1);
break;
}
}Но такой способ уже не будет работать при портировании приложения на 64-битную систему, в которой функция Size уже будет должна возвращать значение типа uint64_t. И тогда итераторы были бы хорошим способом написать корректный код. Например, так бы он выглядел на С++14, если в самописной строке по-прежнему не было бы итераторов, но хотя бы были функции доступа к нижележащему буферу:
auto r_begin = std::make_reverse_iterator(PakPath.GetBuffer() + PakPath.Size());
auto r_end = std::make_reverse_iterator(PakPath.GetBuffer());
while (r_begin != r_end )
{
if (*r_begin == '\\')
{
PakPath.Delete(0, std::distance(r_begin, r_end));
break;
}
++r_begin;
}Предупреждение N7
V611 [CWE-762, CERT-MEM51-CPP] 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 [] ldName;'. ColorAttr.cpp 198
void ColorAttribute::LoadFromFile (....)
{
DWORD slen = 0;
// ....
char* ldName = NEW char[slen+1];
// ....
delete ldName;
}Тут разработчик очистил динамически выделенную память, однако, сделал это неправильно. Вместо того, чтобы позвать оператор delete[] для освобождения аллоцированной ранее памяти при помощи оператора new[], он позвал простой delete. Правильный вариант:
void ColorAttribute::LoadFromFile (....)
{
DWORD slen = 0;
// ....
char* ldName = NEW char[slen+1];
// ....
delete ldName[];
}Подробнее проблема подобных ошибок освещена в статье моего коллеги "Почему в С++ массивы нужно удалять через delete[]".
Предупреждение N8
V607 Ownerless expression 'isSetPause ? 1 : 0'. SoundsEngine.cpp 741
void SoundsEngine::SetPause(bool isSetPause)
{
//....
isSetPause ? 1 : 0;
//....
}Анализатор указывает на мёртвый код. Как я заметил, в классе SoundsEngine содержится похожее поле isPause, однако, оно имеет тип long. Вполне вероятно, что ранее код делал преобразование параметра isSetPause из типа bool в long, но затем его отрефакторили. Строка безобидна, и её можно вполне удалить.
Предупреждение N9
V583 [CWE-783] The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0xffff0000. NoFatality.cpp 63
void _cdecl NoFatality::Draw(float dltTime, long level)
{
//....
for (....)
{
Render().DrawBox(...., IsActive() ? 0xffff0000 : 0xffff0000);
}
//....
}Здесь мы видим, что второй и третий операнды тернарного оператора равны. Получается, что вторым аргументом функции DrawBox всегда будет константа 0xffff0000. Наиболее вероятно, что в одной из веток следовало передавать другое значение, иначе код можно упростить.
Предупреждение N10
V523 [CWE-691] The 'then' statement is equivalent to the 'else' statement. TaskViewer.cpp 298
void TaskViewer::SetText(const char * rawText)
{
// ....
if (rawText[0] == '#')
{
// Подменим идентификатор на реальную строку
// потом
str = rawText;
}
else
{
str = rawText;
}
// ....
}В данном фрагменте кода анализатор отметил, что у оператора if одинаковые ветки then и else. Получается, что вне зависимости от условия будет выполняться один и тот же фрагмент кода. Возможно, это не то поведение, на которое рассчитывал программист.
Предупреждения N11
V614 [CWE-457, CERT-EXP53-CPP] Uninitialized variable 'color.c' used. Color.h 1268
mathinline dword mathcall Color::GetDword() const
{
DColor color;
color.r = (byte)(r * 255.0f);
color.g = (byte)(g * 255.0f);
color.b = (byte)(b * 255.0f);
color.a = (byte)(a * 255.0f);
return color.c;
}Посмотрим, что такое DColor:
class DColor
{
public:
union
{
#ifndef _XBOX
struct
{
///Синий
unsigned char b;
///Зелённый
unsigned char g;
///Красный
unsigned char r;
///Прозрачность
unsigned char a;
};
#else
struct
{
///Прозрачность
unsigned char a;
///Красный
unsigned char r;
///Зелённый
unsigned char g;
///Синий
unsigned char b;
};
#endif
union
{
///Упакованный цвет
dword c;
///Упакованный цвет
dword color;
};
};В коде четыре компоненты RGBA записывают побайтово в нужные поля, а затем возвращают из функции в виде значения типа dword (4-байтовый беззнаковый тип). В языке C подобный код безобиден и программист получил бы то, чего хотел. В C++ чтение из неактивного поля union ведёт к неопределённому поведению. Причина тому – C++ оперирует объектами, у которых должно стартовать время жизни. В любой момент времени может быть активно лишь одно поле. У неактивного же поля (последний union) оно не началось.
Некоторые компиляторы в качестве нестандартного расширения позволяют писать такой код, и большую часть времени он работает так, как вы этого ожидаете. Но стоит учитывать, что он непереносим. На текущем компиляторе и платформе может всё и верно работает, но стоит сменить что-то одно – и могут начаться причуды.
Как написать код на C++, который будет работать везде и всегда одинаково? Подходов может быть несколько. Можно по-прежнему оставить четыре компоненты RGBA в виде четырех полей, а при необходимости получать их комбинацию в виде dword при помощи побитовых операций:
class DColor
{
public:
///Синий
uint8_t b;
///Зелённый
uint8_t g;
///Красный
uint8_t r;
///Прозрачность
uint8_t a;
dword getAsDword() const noexcept
{
#ifndef _XBOX
return (static_cast<uint32_t>(b) << 0)
| (static_cast<uint32_t>(g) << 8)
| (static_cast<uint32_t>(r) << 16)
| (static_cast<uint32_t>(a) << 24);
#else
return (static_cast<uint32_t>(a) << 0)
| (static_cast<uint32_t>(r) << 8)
| (static_cast<uint32_t>(g) << 16)
| (static_cast<uint32_t>(b) << 24);
#endif
}
};Другим способом может быть использование массива из 4 байт, а для его преобразования в dword можно воспользоваться функцией memcpy (или std::bit_cast, начиная с C++20):
class DColor
{
private:
uint8_t m_components[4];
public:
DColor(uint8_t r, uint8_t g, uint8_t b, uint8_t a) noexcept;
DColor(dword comp) noexcept; // To construct from dword
DColor& operator=(dword) noexcept; // To be assigned to dword
uint8_t& R() noexcept;
uint8_t& G() noexcept;
uint8_t& B() noexcept;
uint8_t& A() noexcept;
dword getAsDword() const noexcept
{
#if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast)
return std::bit_cast<dword>(m_components);
#else
dword result;
memcpy(&result, m_components, sizeof(m_components));
return result;
#endif
}
};Комментарий коллеги Андрея Карпова. Подобное использование директив условной компиляции крайне усложняет чтение кода. Нарушается вложенность, и приходится с усилием пытаться понять, что написано, включая и исключая в голове разные фрагменты кода.
Следует писать код не так, чтобы он был максимально короткий, а чтобы его было легко читать и понимать. Я бы сделал здесь две разные реализации функции:
#if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast)
dword getAsDword() const noexcept
{
return std::bit_cast<dword>(m_components);
}
#else
dword getAsDword() const noexcept
{
dword result;
memcpy(&result, m_components, sizeof(m_components));
return result;
}
#endifСуть длиннее, но читать код намного приятнее.
В случае больших функций, с кусочками условной компиляции так, конечно, не получится. В этом случае есть смысл эти кусочки выносить в отдельные функции, по-разному реализованными для разных случаев. Т.е. нужно максимально отделять общий код от зависящего от условий компиляции.
Предупреждения N12
V579 [CWE-687, CERT-ARR01-C] The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. Debugger.cpp 282
void appDebuger::debuggerMakeThreadDump(....)
{
CONTEXT ct;
memset (&ct, 0, sizeof(&ct));
// ....
}
typedef struct DECLSPEC_NOINITALL _CONTEXT
{
DWORD ContextFlags;
// ....
DWORD SegCs; // MUST BE SANITIZED
DWORD EFlags; // MUST BE SANITIZED
DWORD Esp;
DWORD SegSs;
BYTE ExtendedRegisters[MAXIMUM_SUPPORTED_EXTENSION];
} CONTEXT;Программист хотел инициализировать все поля структуры нулями. Размер объекта ct – 716 байт, а передали в неё размер указателя – 4 байта. Поправить вызов memset можно было так:
memset (&ct, 0, sizeof(ct));P.S. Вообще странно, почему программисты так упорно используют этот паттерн инициализации структур с помощью memset. Можно ведь написать:
CONTEXT ct {};Предупреждения N13
Ну и закончить перечень ошибок хочется парой прикольных опечаток:
void CharacterLegs::Invalidate()
{
lleg.th = ani->FindBone("Bedro_left_joint" ,true);
lleg.kn = ani->FindBone("Golen_left_joint" ,true);
lleg.fo = ani->FindBone("Foolt_left_joint" ,true);
lleg.to = ani->FindBone("Foolt_left_joint_2",true);
rleg.th = ani->FindBone("Bedro_right_joint" ,true);
rleg.kn = ani->FindBone("Golen_right_joint" ,true);
rleg.fo = ani->FindBone( "Foot_right_joint" ,true);
rleg.to = ani->FindBone( "Foot_right_joint_2",true);
if ( lleg.th >= 0 && lleg.kn >= 0
&& lleg.fo >= 0 && lleg.to >= 0
&& rleg.th >= 0 && rleg.kn >= 0
&& lleg.fo >= 0 && lleg.to >= 0 )
{
// ....
}
// ....
}Здесь, в условии оператора if анализатор заметил два одинаковых подвыражения lleg.to >= 0 и lleg.fo >= 0. Видно, что разработчик хотел проверить все поля и опечатался. На месте двух одинаковых подвыражений должны быть проверки других переменных:
if ( lleg.th >= 0 && lleg.kn >= 0
&& rleg.th >= 0 && rleg.kn >= 0
&& lleg.fo >= 0 && lleg.to >= 0
&& rleg.fo >= 0 && rleg.to >= 0)
{
// ....
}В результате проверки анализатор нашёл в проекте ошибки разного рода. Чёртову дюжину из них я описал в статье. Оставшиеся – стимул для вас скачать анализатор и попробовать его (если вы ещё, конечно, этого не сделали). А ещё интереснее будет посмотреть, что найдётся в вашем собственном коде или коде коллег :).
Больше спасибо людям, благодаря которым игра всё-таки смогла увидеть свет, а также развивающим и поддерживающим её обзорами, статьями и обсуждениями. Ну а потонет ли "Арабелла", пускай каждый читатель решит сам!