# Йо\-хо\-хо, и бутылка рому\! Разбираем ошибки игрового движка Storm Engine PVS\-Studio – это инструмент статического анализа, позволяющий находить ошибки в исходном коде программ\. В качестве знакомства с возможностями анализатора вашему вниманию предлагается результат проверки PVS\-Studio исходного кода игрового движка Storm Engine\. ![0838_Storm_Engine_ru/image1.png](https://import.viva64.com/docx/blog/0838_Storm_Engine_ru/image1.png) ## Storm Engine Storm Engine \- игровой движок, разрабатывавшийся с января 2000 года компанией Акелла для серии игр Корсары\. 26 марта 2021 года исходный код движка открыт под лицензией GPLv3 и доступен на [GitHub](https://github.com/storm-devs/storm-engine)\. Проект написан на C\+\+\. Всего в проекте было обнаружено 235 предупреждений уровня High и 794 предупреждения уровня Medium\. Многие из этих предупреждений указывают на ошибки, которые могут привести к неопределенному поведению программы\. Другие предупреждения выявляют логические ошибки в коде – программа будет работать стабильно, но результаты этой работы будут отличаться от ожидаемых\. Разбор всех 1029 предупреждений – это материал, достаточный для написания целой книги, а не статьи\. Кроме того, анализ почти всех этих предупреждений требует глубокого погружения в кодовую базу проекта, что долго и утомительно как описывать, так и читать\. Поэтому в своей статье разберу только те ошибки и неточности, которые понятны без погружения в структуру проекта\. ## Найденные ошибки ### Лишние проверки Предупреждение PVS\-Studio: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'nStringCode \>\= 0xffffff' is always false\. dstring\_codec\.h 84 ```cpp #define DHASH_SINGLESYM 255 .... uint32_t Convert(const char *pString, ....) { uint32_t nStringCode; .... nStringCode = ((((unsigned char)pString[0]) << 8) & 0xffffff00) | (DHASH_SINGLESYM) .... if (nStringCode >= 0xffffff) { __debugbreak(); } return nStringCode; } ``` Проведем оценку выражения, которое будет присвоено переменной _nStringCode_\. Тип _unsigned_ _char_ принимает значения _\[0, 255\]_, поэтому _\(unsigned char\)pString\[0\]_ всегда меньше _2^8_, после сдвига на _8 _получаем не более _2^16_, конъюнкция не увеличивает это значение\. После увеличиваем значение выражения не более чем на _255_\. Итого значение переменной _nStringCode_ не превышает _2^16\+256_, что всегда меньше, чем _0xffffff \= 2^24\-1,_ и проверка оказалась всегда ложной\. Таким образом, проверка на самом деле ни от чего не защищает, и её можно удалить из кода: ```cpp #define DHASH_SINGLESYM 255 .... uint32_t Convert(const char *pString, ....) { uint32_t nStringCode; .... nStringCode = ((((unsigned char)pString[0]) << 8) & 0xffffff00) | (DHASH_SINGLESYM) .... return nStringCode; } ``` Впрочем, спешить это делать не стоит\. Данная проверка явно написана для защиты от ошибок, которые могут возникнуть в процессе модификации выражения или, например, константы _DHASH\_SINGLESYM_\. Это тот случай, когда формально анализатор прав, но это не значит, что он нашёл фрагмент кода, требующий обязательного исправления\. Предупреждение PVS\-Studio: [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: 0x00 <\= c\. utf8\.h 187 ```cpp inline bool IsValidUtf8(....) { int c, i, ix, n, j; for (i = 0, ix = str.length(); i < ix; i++)s { c = (unsigned char)str[i]; if (0x00 <= c && c <= 0x7f) n = 0; ... } ... } ``` Переменной _c_ было присвоено значение беззнакового типа, поэтому проверка _0x00 <\= c_ бессмысленна, и ее можно убрать\. Сокращенный код: ```cpp inline bool IsValidUtf8(....) { int c, i, ix, n, j; for (i = 0, ix = str.length(); i < ix; i++)s { c = (unsigned char)str[i]; if (c <= 0x7f) n = 0; ... } ... } ``` ### Выход за границу массива Предупреждение PVS\-Studio: [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'TempLong2 \- TempLong1 \+ 1' index could reach 520\. internal\_functions\.cpp 1131 ```cpp DATA *COMPILER::BC_CallIntFunction(....) { if (TempLong2 - TempLong1 >= sizeof(Message_string)) { SetError("internal: buffer too small"); pV = SStack.Push(); pV->Set(""); pVResult = pV; return pV; } memcpy(Message_string, pChar + TempLong1, TempLong2 - TempLong1 + 1); Message_string[TempLong2 - TempLong1 + 1] = 0; pV = SStack.Push(); } ``` В этом примере анализатор помогает найти off\-by\-one error\. Сначала проверяется, что значение _TempLong2 \- TempLong1_ меньше размера строки _Message\_string_, а затем выполняется присваивание по индексу _TempLong2 \- TempLong1 \+ 1_\. В случае, если _TempLong2 \- TempLong1 \+ 1 \=\= sizeof\(Message\_string\)_, проверка будет пройдена, внутренняя ошибка сгенерирована не будет, а при выполнении присваивания произойдет обращение к незарезервированной памяти, что приведет к неопределенному поведению программы\. Необходимо поменять проверку: ```cpp DATA *COMPILER::BC_CallIntFunction(....) { if (TempLong2 - TempLong1 + 1 >= sizeof(Message_string)) { SetError("internal: buffer too small"); pV = SStack.Push(); pV->Set(""); pVResult = pV; return pV; } memcpy(Message_string, pChar + TempLong1, TempLong2 - TempLong1 + 1); Message_string[TempLong2 - TempLong1 + 1] = 0; pV = SStack.Push(); } ``` ### Присваивание переменной самой себе Предупреждение PVS\-Studio: [V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'Data\_num' variable is assigned to itself\. s\_stack\.cpp 36 ```cpp uint32_t Data_num; .... DATA *S_STACK::Push(....) { if (Data_num > 1000) { Data_num = Data_num; } ... } ``` Возможно, данный фрагмент кода был добавлен для отладки и впоследствии не был удален\. Переменной _Data\_num_ вместо нового значения присваивается ее собственное значение\. Сложно точно сказать, какое значение программист хотел присвоить\. Вероятно, здесь должно быть присваивание значения другой переменной с похожим названием, но по невнимательности название было перепутано\. Другой вариант – значение переменной хотели ограничить константой 1000, но опечатались\. В любом случае это ошибка, и ее нужно исправить\. ### Разыменование нулевого указателя Предупреждение PVS\-Studio: [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'rs' pointer was utilized before it was verified against nullptr\. Check lines: 163, 164\. Fader\.cpp 163 ```cpp uint64_t Fader::ProcessMessage(....) { .... textureID = rs->TextureCreate(_name); if (rs) { rs->SetProgressImage(_name); .... } ``` Здесь сначала происходит разыменование указателя _rs_, а потом этот указатель проверяется на равенство _nullptr_\. Если указатель может быть равен _nullptr_, то разыменование нулевого указателя приведет к неопределенному поведению, поэтому проверку нужно делать до первого разыменования: ```cpp uint64_t Fader::ProcessMessage(....) { .... if (rs) { textureID = rs->TextureCreate(_name); rs->SetProgressImage(_name); .... } ``` Если же гарантируется, что всегда _rs \!\= nullptr_, то проверка _if \(rs\)_ лишняя, и ее надо убрать: ```cpp uint64_t Fader::ProcessMessage(....) { .... textureID = rs->TextureCreate(_name); rs->SetProgressImage(_name); .... } ``` Есть и ещё один вариант\. На самом деле, хотели проверить переменную _textureID_\. Всего в проекте встретилось 14 предупреждений V595\. Заинтересованным читателям предлагаю провести анализ этих предупреждений самостоятельно, [скачав и запустив PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/)\. Я же ограничусь разбором еще одного примера: Предупреждение PVS\-Studio: [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'pACh' pointer was utilized before it was verified against nullptr\. Check lines: 1214, 1215\. sail\.cpp 1214 ```cpp void SAIL::SetAllSails(int groupNum) { .... SetSailTextures(groupNum, core.Event("GetSailTextureData", "l", pACh->GetAttributeAsDword("index", -1))); if (pACh != nullptr){ .... } ``` При вычислении аргументов метода _Event _происходит разыменования указателя _pACh_, который в следующей строке кода проверяется на равенство _nullptr_\. Если указатель может быть нулевым, то вызов функции _SetSailTextures_, порождающий разыменование _pACh_, должен выполняться после проверки: ```cpp void SAIL::SetAllSails(int groupNum) { .... if (pACh != nullptr){ SetSailTextures(groupNum, core.Event("GetSailTextureData", "l", pACh->GetAttributeAsDword("index", -1))); .... } ``` Если же гарантируется, что _pACh_ – ненулевой, то проверку нужно убрать: ```cpp void SAIL::SetAllSails(int groupNum) { .... SetSailTextures(groupNum, core.Event("GetSailTextureData", "l", pACh->GetAttributeAsDword("index", -1))); .... } ``` ### Ошибка new\[\] – delete Предупреждение PVS\-Studio: [V611](https://pvs-studio.ru/ru/docs/warnings/v611/) 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 \[\] pVSea;'\. Check lines: 169, 191\. SEA\.cpp 169 ```cpp struct CVECTOR { public: union { struct { float x, y, z; }; float v[3]; }; }; .... struct SeaVertex { CVECTOR vPos; CVECTOR vNormal; float tu, tv; }; .... #define STORM_DELETE (x) { delete x; x = 0; } void SEA::SFLB_CreateBuffers() { ... pVSea = new SeaVertex[NUM_VERTEXS]; } SEA::~SEA() { ... STORM_DELETE(pVSea); ... } ``` Использование макросов – практика, требующая особой внимательности\. В данном случае использование макроса привело к ошибке: выделенная оператором _new\[\]_ память освобождается неправильным оператором _delete_ вместо правильного _delete\[\]_\. Как результат, деструкторы элементов массива _pVSea_ вызваны не будут\. Иногда это может не проявляться – например, когда все деструкторы элементов массива и их полей тривиальны\. Однако если ошибка не проявляется на runtime – не значит, что ее нет\. Дело в том, что реализация оператора _new\[\]_ может быть устроена так, что в начало участка памяти, выделенного под массив, дополнительно помещается размер этого участка и количество элементов в массиве\. Тогда при вызове несовместимого оператора _delete_ информация о размере блока и числе элементов, вероятно, будет интерпретирована неправильно, и результат такой операции не определен\. Также не исключена ситуация, когда память для отдельных элементов и массивов выделяется из разных пулов памяти\. Тогда попытка вернуть выделенную для массива память в пул, предназначенный для скаляров, приведет к катастрофе\. Эта ошибка особенна опасна тем, что она долгое время может не проявлять себя и выстрелить в неожиданный момент\. Всего было выявлено 15 ошибок такого типа, вот некоторые из них: * V611 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 \[\] m\_pShowPlaces;'\. Check lines: 421, 196\. ActivePerkShower\.cpp 421 * V611 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 \[\] pTable;'\. Check lines: 371, 372\. AIFlowGraph\.h 371 * V611 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 \[\] vrt;'\. Check lines: 33, 27\. OctTree\.cpp 33 * V611 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 \[\] flist;'\. Flag\.cpp 738 * V611 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 \[\] rlist;'\. Rope\.cpp 660 Анализ кода показал, что макрос _STORM\_DELETE_ используется во многих из найденных случаев\. Однако простое изменение оператора _delete_ на оператор _delete\[\]_ приведет к новым ошибкам, потому что этот макрос используется также и для освобождения памяти, выделенной оператором _new_\. Поэтому исправить код нужно, добавив новый макрос _STORM\_DELETE\_ARRAY_, который использует правильный оператор _delete\[\]_: ```cpp struct CVECTOR .... struct SeaVertex { CVECTOR vPos; CVECTOR vNormal; float tu, tv; }; .... #define STORM_DELETE (x) { delete x; x = 0; } #define STORM_DELETE_ARRAY (x) { delete[] x; x = 0; } void SEA::SFLB_CreateBuffers() { ... pVSea = new SeaVertex[NUM_VERTEXS]; } SEA::~SEA() { ... STORM_DELETE_ARRAY(pVSea); ... } ``` ### Присваивание дважды подряд Предупреждение PVS\-Studio: [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'h' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 385, 389\. Sharks\.cpp 389 ```cpp inline void Sharks::Shark::IslandCollision(....) { if (h < 1.0f) { h -= 100.0f / 150.0f; if (h > 0.0f) { h *= 150.0f / 50.0f; } else h = 0.0f; h = 0.0f; vx -= x * (1.0f - h); vz -= z * (1.0f - h); } ``` В этом фрагменте кода в случае _h < 1\.0f_ сначала производятся вычисления над переменной _h_, а затем ей присваивается значение _0_\. Как результат – переменная _h_ всегда равна _0_, что является ошибкой, и последнее присваивание переменной _h_ нужно убрать: ```cpp inline void Sharks::Shark::IslandCollision(....) { if (h < 1.0f) { h -= 100.0f / 150.0f; if (h > 0.0f) { h *= 150.0f / 50.0f; } else h = 0.0f; vx -= x * (1.0f - h); vz -= z * (1.0f - h); } ``` ### Разыменование указателя, полученного из функции realloc или malloc Предупреждение PVS\-Studio: [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'pTable'\. Check lines: 36, 35\. s\_postevents\.h 36 ```cpp void Add(....) { .... pTable = (S_EVENTMSG **)realloc( pTable, nClassesNum * sizeof(S_EVENTMSG *)); pTable[n] = pClass; .... }; ``` Функция _realloc_ в случае, когда недостаточно памяти для расширения блока до заданного размера, возвращает _NULL_\. Если произойдет такая ситуация, то при вычислении выражения _pTable\[n\] _произойдет разыменование нулевого указателя, что приведет к неопределенному поведению программы\. Кроме того, из\-за того, что указатель _pTable_ будет перезаписан, адрес исходного блока памяти может быть утерян\. Необходимо добавить проверку и использовать дополнительный указатель: ```cpp void Add(....) { .... S_EVENTMSG ** newpTable = (S_EVENTMSG **)realloc(pTable, nClassesNum * sizeof(S_EVENTMSG *)); if(newpTable) { pTable = newpTable; pTable[n] = pClass; .... } else { // Обрабатываем ситуацию, когда realloc не смог сделать реаллокацию } }; ``` Встречаются подобные ошибки и в использовании функции _malloc_: Предупреждение PVS\-Studio: [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'label'\. Check lines: 116, 113\. geom\_static\.cpp 116 ```cpp GEOM::GEOM(....) : srv(_srv) { .... label = static_cast