# Приключения капитана Блада: потонет ли Арабелла? Недавно в сети появилась новость о том, что был открыт исходный код игры "Приключения капитана Блада"\. Мы не смогли пройти мимо и проверили его качество с помощью PVS\-Studio\. Потонет ли легендарный корабль капитана Блада от найденных багов? Давайте узнаем\! ![1033_cap_blood_ru/image1.png](https://import.viva64.com/docx/blog/1033_cap_blood_ru/image1.png) ## О проекте Игра "Приключения капитана Блада" создана студией "1C Морской волк" по мотивам книг Рафаэля Сабатини\. Она повествует о приключениях главного героя этих произведений, капитана Питера Блада\. Действие игры разворачивается в средневековой новой Англии\. Главный герой, военный врач по образованию, был несправедливо осуждён и сослан на остров Барбадос\. С острова ему удаётся бежать, после чего, захватив испанский корабль, он становится самым знаменитым пиратом того времени\. Проект был завершён в 2010 году, но так и не увидел свет\. Всё потому, что компании 1С и Playlogic не смогли решить, кому принадлежат права на игру\. Далее ходили новости, что финальная сборка игры безвозвратно утеряна\. Однако не так давно появилось несколько новостей о том, что это не так\. Появилось видео с прохождением на YouTube и работающий билд игры на одном известном трекере\. Позже на GitHub выложили [исходники](https://github.com/storm-devs/captain-blood.git) под лицензией GPLv3\. Итак, после того как мы познакомились с проектом, предлагаю посмотреть какие интересные ошибки в нём удалось обнаружить\. ## Чертова дюжина **Предупреждение N1** [V1075](https://pvs-studio.ru/ru/docs/warnings/v1075/) The function expects the file to be opened for writing, but it was opened for reading\. Debugger\.cpp 172 Можем поздравить моего коллегу Сергея Ларина с первым трофейным срабатыванием его диагностического правила, вышедшего в релизе PVS\-Studio 7\.15\. Данное диагностическое правило ругается на запись в файлы, открытые для чтения, и наоборот\. Честно, мы даже не думали, что оно когда\-нибудь сработает\. Приведу проблемную функцию полностью: ```cpp 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_\. Поправить код можно таким образом: ```cpp fwrite(buf, s, 1, to); ``` **Предупреждение N2** [V1065](https://pvs-studio.ru/ru/docs/warnings/v1065/) \[CWE\-682\] Expression can be simplified, check 'bi' and similar operands\. PreviewAnimation\.cpp 146 ```cpp 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_: ```cpp bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + bw + 7 + 1; ``` Возможно, на месте одного из _bi_ должна быть какая\-то другая переменная\. Или же выражение _\(bw \+ bi\)_ забыли на что\-то умножить\. Интересно, а как бы поменялись анимации в игре после этого? Также, отмечу, что давать подобные имена константам: _bw, bh, bi, cn_ – плохая практика\. Это запутывает код для читающего его человека\. Куда уж лучше было бы назвать переменные осмысленнее или хотя бы сопроводить их определения поясняющими комментариями\. **Предупреждение N3** [V570](https://pvs-studio.ru/ru/docs/warnings/v570/) \[CWE\-480\] The 'ldMaxChars' variable is assigned to itself\. StringAttr\.cpp 198 ```cpp void StringAttribute::LoadFromFile (....) { int ldMinChars, ldMaxChars; // .... minChars = ldMinChars; ldMaxChars = ldMaxChars; // .... } class StringAttribute : public BaseAttribute { int minChars; int maxChars; } ``` Тут мы видим классическое присваивание переменной самой себе\. Чтобы посмотреть, что должно быть на месте переменной, в которую происходит присваивание, заглянем в определение класса _StringAttribute_\. В нём видно похожее поле _maxChars_\. Очевидно, что программист опечатался и на самом деле должен был присвоить переменной значение _ldMaxChars_\. Поправленный фрагмент кода: ```cpp void StringAttribute::LoadFromFile (....) { int ldMinChars, ldMaxChars; // .... minChars = ldMinChars; maxChars = ldMaxChars; // .... } ``` **Предупреждение N4** [V781](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp __forceinline void GetBootParams(const char * bootIni) { long start = 0; // .... if (bootIni[start] && start > 0) { // .... } // .... } ``` В данном фрагменте кода программист сначала использовал значение _start_ для индексации по массиву _bootIni_, а только затем проверил его на больше_ 0_\. Проверка может быть бессмысленна, тогда стоит убрать её\. Также она может иметь смысл, тогда её стоит поменять местами с операцией взятия значения по индексу\. ```cpp if (start > 0 && bootIni[start]) { // .... } ``` В противном случае, программист рискует уронить игру\. Кстати, таких срабатываний в программе было довольно много\. Вот несколько, на которые стоит обратить внимание в первую очередь: * [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'i' index is checked after it was used\. Perhaps there is a mistake in program logic\. tinyxml\.h 352 * [V781](https://pvs-studio.ru/ru/docs/warnings/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 * [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'pathLen' index is checked after it was used\. Perhaps there is a mistake in program logic\. winworkpath\.h 50 * \.\.\.\. **Предупреждение N5** [V700](https://pvs-studio.ru/ru/docs/warnings/v700/) \[CWE\-480\] Consider inspecting the 'mView \= mView \= chr\.Render\(\)\.GetView\(\)' expression\. It is odd that variable is initialized through itself\. CharacterItems\.cpp 705 ```cpp void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld) { // .... Matrix mView = mView = chr.Render().GetView(); // .... } ``` Странный код с присваиванием значения переменной самого себе\. Скорее всего произошла опечатка\. В любом случае, избыточное присваивание можно убрать: ```cpp void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld) { // .... Matrix mView = chr.Render().GetView(); // .... } ``` **Предупреждение N6** [V654](https://pvs-studio.ru/ru/docs/warnings/v654/) \[CWE\-834\] The condition 'n \>\= 0' of loop is always true\. resourceselect\.cpp 42 ```cpp 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](https://pvs-studio.ru/ru/blog/terms/0063/)\. Тут программист написал алгоритм, который ищет первое вхождение символа _\\_ с конца и удаляет подстроку от начала до этого символа включительно\. Самым корректным исправлением было бы использование _rbegin_ и _rend_, но есть нюанс – это самописная строка \(аналог _std::string_ с дополнительным функционалом\), в которой, к сожалению, нет итераторов\. Тогда, чтобы поправить код мы можем взять знаковую переменную с размерностью в 2 раза больше \(_int64\_t_\) и пустить цикл по ней\. ```cpp for (int64_t n = PakPath.Size() - 1; n >= 0; n--) { if (PakPath[n] == '\\') { PakPath.Delete(0, n+1); break; } } ``` Но такой способ уже не будет работать при портировании приложения на 64\-битную систему, в которой функция _Size_ уже будет должна возвращать значение типа _uint64\_t_\. И тогда итераторы были бы хорошим способом написать корректный код\. Например, так бы он выглядел на С\+\+14, если в самописной строке по\-прежнему не было бы итераторов, но хотя бы были функции доступа к нижележащему буферу: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp void ColorAttribute::LoadFromFile (....) { DWORD slen = 0; // .... char* ldName = NEW char[slen+1]; // .... delete ldName; } ``` Тут разработчик очистил динамически выделенную память, однако, сделал это неправильно\. Вместо того, чтобы позвать оператор _delete\[\]_ для освобождения аллоцированной ранее памяти при помощи оператора _new\[\]_, он позвал простой _delete_\. Правильный вариант: ```cpp void ColorAttribute::LoadFromFile (....) { DWORD slen = 0; // .... char* ldName = NEW char[slen+1]; // .... delete ldName[]; } ``` Подробнее проблема подобных ошибок освещена в статье моего коллеги "[Почему в С\+\+ массивы нужно удалять через delete\[\]](https://pvs-studio.ru/ru/blog/posts/cpp/0973/)"\. **Предупреждение N8** [V607](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'isSetPause ? 1 : 0'\. SoundsEngine\.cpp 741 ```cpp void SoundsEngine::SetPause(bool isSetPause) { //.... isSetPause ? 1 : 0; //.... } ``` Анализатор указывает на мёртвый код\. Как я заметил, в классе _SoundsEngine_ содержится похожее поле _isPause_, однако, оно имеет тип _long_\. Вполне вероятно, что ранее код делал преобразование параметра _isSetPause_ из типа _bool_ в _long_, но затем его отрефакторили\. Строка безобидна, и её можно вполне удалить\. **Предупреждение N9** [V583](https://pvs-studio.ru/ru/docs/warnings/v583/) \[CWE\-783\] The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0xffff0000\. NoFatality\.cpp 63 ```cpp void _cdecl NoFatality::Draw(float dltTime, long level) { //.... for (....) { Render().DrawBox(...., IsActive() ? 0xffff0000 : 0xffff0000); } //.... } ``` Здесь мы видим, что второй и третий операнды тернарного оператора равны\. Получается, что вторым аргументом функции _DrawBox_ всегда будет константа _0xffff0000_\. Наиболее вероятно, что в одной из веток следовало передавать другое значение, иначе код можно упростить\. **Предупреждение N10** [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) \[CWE\-691\] The 'then' statement is equivalent to the 'else' statement\. TaskViewer\.cpp 298 ```cpp void TaskViewer::SetText(const char * rawText) { // .... if (rawText[0] == '#') { // Подменим идентификатор на реальную строку // потом str = rawText; } else { str = rawText; } // .... } ``` В данном фрагменте кода анализатор отметил, что у оператора _if_ одинаковые ветки _then_ и _else_\. Получается, что вне зависимости от условия будет выполняться один и тот же фрагмент кода\. Возможно, это не то поведение, на которое рассчитывал программист\. **Предупреждения N11** [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) \[CWE\-457, CERT\-EXP53\-CPP\] Uninitialized variable 'color\.c' used\. Color\.h 1268 ```cpp 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_: ```cpp 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_ при помощи побитовых операций: ```cpp class DColor { public: ///Синий uint8_t b; ///Зелённый uint8_t g; ///Красный uint8_t r; ///Прозрачность uint8_t a; dword getAsDword() const noexcept { #ifndef _XBOX return (static_cast(b) << 0) | (static_cast(g) << 8) | (static_cast(r) << 16) | (static_cast(a) << 24); #else return (static_cast(a) << 0) | (static_cast(r) << 8) | (static_cast(g) << 16) | (static_cast(b) << 24); #endif } }; ``` Другим способом может быть использование массива из 4 байт, а для его преобразования в _dword_ можно воспользоваться функцией _memcpy_ \(или _std::bit\_cast_, начиная с C\+\+20\): ```cpp 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(m_components); #else dword result; memcpy(&result, m_components, sizeof(m_components)); return result; #endif } }; ``` **Комментарий коллеги Андрея Карпова\.** Подобное использование директив условной компиляции крайне усложняет чтение кода\. Нарушается вложенность, и приходится с усилием пытаться понять, что написано, включая и исключая в голове разные фрагменты кода\. Следует писать код не так, чтобы он был максимально короткий, а чтобы его было легко читать и понимать\. Я бы сделал здесь две разные реализации функции: ```cpp #if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast) dword getAsDword() const noexcept { return std::bit_cast(m_components); } #else dword getAsDword() const noexcept { dword result; memcpy(&result, m_components, sizeof(m_components)); return result; } #endif ``` Суть длиннее, но читать код намного приятнее\. В случае больших функций, с кусочками условной компиляции так, конечно, не получится\. В этом случае есть смысл эти кусочки выносить в отдельные функции, по\-разному реализованными для разных случаев\. Т\.е\. нужно максимально отделять общий код от зависящего от условий компиляции\. **Предупреждения N12** [V579](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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_ можно было так: ```cpp memset (&ct, 0, sizeof(ct)); ``` P\.S\. Вообще странно, почему программисты так упорно используют этот паттерн инициализации структур с помощью _memset_\. Можно ведь написать: ```cpp CONTEXT ct {}; ``` **Предупреждения N13** Ну и закончить перечень ошибок хочется парой прикольных опечаток: * [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) \[CWE\-571\] There are identical sub\-expressions 'lleg\.to \>\= 0' to the left and to the right of the '&&' operator\. CharacterLegs\.cpp 51 * [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) \[CWE\-571\] There are identical sub\-expressions 'lleg\.fo \>\= 0' to the left and to the right of the '&&' operator\. CharacterLegs\.cpp 51 ```cpp 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_\. Видно, что разработчик хотел проверить все поля и опечатался\. На месте двух одинаковых подвыражений должны быть проверки других переменных: ```cpp 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) { // .... } ``` ## Заключение В результате проверки анализатор нашёл в проекте ошибки разного рода\. Чёртову дюжину из них я описал в статье\. Оставшиеся – стимул для вас скачать анализатор и [попробовать](https://pvs-studio.ru/ru/pvs-studio/try-free/) его \(если вы ещё, конечно, этого не сделали\)\. А ещё интереснее будет посмотреть, что найдётся в вашем собственном коде или коде коллег :\)\. Больше спасибо людям, благодаря которым игра всё\-таки смогла увидеть свет, а также развивающим и поддерживающим её обзорами, статьями и обсуждениями\. Ну а потонет ли "Арабелла", пускай каждый читатель решит сам\!