# 30 лет DOOM: новый код — новые баги Сегодня первой игре из серии DOOM исполняется ровно 30 лет\! Мы не могли обойти стороной это событие и в честь этого решили посмотреть, как же выглядит код этой легендарной игры спустя годы\. ![1087_gzdoom_ru/image1.png](https://import.viva64.com/docx/blog/1087_gzdoom_ru/image1.png) ## Предисловие DOOM навсегда останется в истории как одна из величайших классических игр, которая оказала огромное влияние на игровую индустрию\. Игра стала революционной для своего времени, ввела новые стандарты в геймплее и технических возможностях для шутеров от первого лица\. Её быстрая, напряжённая игровая механика, мрачная атмосфера и впечатляющий арсенал оружия навсегда запали в сердца игроков\. Что уж говорить о потрясающем музыкальном сопровождении\! Как говорится: "Хеви\-металл играет не потому, что ты сражаешься с демонами, он играет, потому что демоны сражаются с тобой\!"\. Понятно, что посмотреть все тысячи строк кода в рамках одной статьи невозможно, да и скучно это\. Поэтому сегодня мы буквально побудем Думгаем и последуем цитате из последней DOOM Eternal: > Против всех исчадий из глубин преисподней, против всех нечестивцев рода людского, против легионов мы выставим тебя одного\. Рви и кромсай, пока не иссякнут\. Мы будем рвать и кромсать всё то зло, что может произвести ад\. А какое наихудшее зло может быть для программиста? Конечно же, баги в коде\! Мы будем находить и ~~убивать~~ исправлять их\. В качестве плацдарма для высадки послужит [GZDoom v4\.11\.3](https://github.com/ZDoom/gzdoom/tree/g4.11.3) — один из самых популярных портов оригинальной игры DOOM\. В качестве помощника — статический анализатор [PVS\-Studio](https://pvs-studio.ru/ru/)\. Что ж, приступим\! > Примечание автора: названия разделов соответствуют названиям глав из игры\\\. Почему? Можно предположить, что каждое название так или иначе соответствует типу багов\\\. Например, в разделе "По колено в трупах" будут баги, связанные с "мёртвыми" \\\(висячими\\\) указателями или ссылками, мертвым кодом\\\. В разделе "Побережье ада"\\\.\\\.\\\. ладно, стоп\\\. На самом деле, просто я так захотел\\\. Звучит прикольно\\\. > > > > Кстати, несколько лет назад мы \[проверяли\]\(https://pvs\-studio\.ru/ru/blog/posts/cpp/0662/\) исходный код порта DOOM Engine на Linux\\\. Также рекомендуем её к ознакомлению всем любителям серии :\\\) ## По колено в трупах ![1087_gzdoom_ru/image2.png](https://import.viva64.com/docx/blog/1087_gzdoom_ru/image2.png) Итак, высадка в космическом комплексе прошла успешно\. Мы сразу видим демонов, заполнивших подстанцию, в которой мы находимся, а также путь дальше по комплексу\. Наша задача — очистить комплекс от всех демонов\. **Фрагмент N1** Первым встреченным противником становится зомби\-человек, который зачем\-то бессмысленно бьётся головой об стену\. Дам вам время самим разобраться что к чему, я лишь пока навёл прицел на нужное место\. ```cpp void SWPalDrawers::DrawUnscaledFuzzColumn(const SpriteDrawerArgs& args) { .... int fuzzstep = 1; int fuzz = _fuzzpos % FUZZTABLE; #ifndef ORIGINAL_FUZZ while (count > 0) { int available = (FUZZTABLE - fuzz); int next_wrap = available / fuzzstep; if (available % fuzzstep != 0) // <= next_wrap++; .... // Здесь fuzzstep не меняется. Клянусь BFG. } .... } ``` Предупреждение анализатора: * [V1063](https://pvs-studio.ru/ru/docs/warnings/v1063/) The modulo by 1 operation is meaningless\. The result will always be zero\. [r\_draw\_rgba\.cpp 328](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/rendering/swrenderer/drawers/r_draw_rgba.cpp#L328) * [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'available % fuzzstep \!\= 0' is always false\. [r\_draw\_rgba\.cpp 328](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/rendering/swrenderer/drawers/r_draw_rgba.cpp#L328) Как мы видим, переменная _fuzzstep _объявляется и сразу инициализируется значением _1_ во фрагменте кода\. Далее её значение нигде не меняется\. В цикле _while_ происходит проверка условия _if \(available % fuzzstep \!\= 0\) _раз за разом, в надежде на изменения\.\.\. \(чёрт, кажется, это не та игра\), но _fuzzstep _нигде не меняется, и мы каждый раз делим _available_ по модулю на _1_, и каждый раз результат _0_, и мы проверяем его на неравенство с _0_\.\.\. Поскорее покончим с ним и пройдём дальше\. **Фрагмент N2** На нашем пути появляется ловушка, оставленная кем\-то до нас\. Потрясающая работа других морпехов: возможный путь для демонов нашли, установили мину\. И когда демоны решат здесь пройти, то\.\.\. ничего не произойдёт\. Мину не активировали\.\.\. ```cpp StringPool::Block *StringPool::AddBlock(size_t size) { .... auto mem = (Block *)malloc(size); if (mem == nullptr) { } mem->Limit = (uint8_t *)mem + size; mem->Avail = &mem[1]; mem->NextBlock = TopBlock; TopBlock = mem; return mem; } ``` Предупреждение анализатора: [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'mem'\. Check lines: 100, 95\. [fs\_stringpool\.cpp 100](https://github.com/ZDoom/gzdoom/blob/6ce809efe2902e43ceaa7031b875225d3a0367de/src/common/filesystem/source/fs_stringpool.cpp#L100) Давайте посмотрим внимательнее\. Здесь объявляется переменная _mem_ и сразу же инициализируется результатом функции _malloc_\. Как известно, _malloc_ может вернуть _NULL_, и об этом разработчики прекрасно знали и даже сделали необходимую проверку в виде _if \(mem \=\= nullptr\)_, но забыли написать, что делать, если условие истинно\. Кстати, если вы не проверяете результат функции _malloc_, предлагаю прочитать эту [статью](https://pvs-studio.ru/ru/blog/posts/cpp/0938/)\. Что именно забыли написать, остаётся на совести разработчиков\. Возможно, здесь должен быть вызов [_std::exit_](https://en.cppreference.com/w/cpp/utility/program/exit), или возвращаться какое\-либо значение, или что\-то еще\. Не став рисковать и пытаться активировать мину, мы идём дальше\. **Фрагмент N3** На пути встречается имп, который даже не замечает нас и не пытается атаковать\. Он вообще ничего не делает\. ```cpp void PClassActor::InitializeDefaults() { .... if (MetaSize > 0) memcpy(Meta, ParentClass->Meta, ParentClass->MetaSize); else memset(Meta, 0, MetaSize); .... } ``` Предупреждение анализатора: [V575](https://pvs-studio.ru/ru/docs/warnings/v575/) The 'memset' function processes '0' elements\. Inspect the third argument\. [info\.cpp 518](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/gamedata/info.cpp#L518) Как мы видим, память, на которую указывает _Meta, _хотят перезаписать нулями при помощи [_memset_](https://en.cppreference.com/w/cpp/string/byte/memset)\. Проблема в том, что в ветку _else_ мы попадаем только если _MetaSize_ равен 0\. Для _memset_ такой вызов означает: "заполни по такому\-то адресу таким\-то значением область памяти размером 0 байт" \=\= "ничего не делай"\. Проходим дальше\. **Фрагмент N4** Мы натыкаемся на импа, который, в отличие от предыдущего, тут же нападает\. ```cpp void FDecalLib::ParseDecal (FScanner &sc) { FDecalTemplate newdecal; .... memset ((void *)&newdecal, 0, sizeof(newdecal)); .... } ``` Предупреждение анализатора: [V598](https://pvs-studio.ru/ru/docs/warnings/v598/) The 'memset' function is used to nullify the fields of 'FDecalTemplate' class\. Virtual table pointer will be damaged by this\. [decallib\.cpp 367](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/gamedata/decallib.cpp#L367) Здесь вызывается уже рассмотренная выше функция _memset_ для объекта _newdecal_ типа _FDecalTemplate_\. Таким образом хотят занулить обьект\. Проблема в том, что тип _FDecalTemplate_ содержит в себе указатель на виртуальную таблицу: ```cpp class FDecalTemplate : public FDecalBase { .... } class FDecalBase { .... virtual const FDecalTemplate *GetDecal () const; virtual void ReplaceDecalRef (FDecalBase *from, FDecalBase *to) = 0; .... }; ``` Оператор _sizeof_ же вернёт размер объекта с учётом размера этого указателя\. Обнуляя поля таким образом, занулится также и указатель на таблицу виртуальных функций\. Хороший способ ~~прострелить себе ногу~~ получить дамаг от удара демона\. **Фрагмент N5** Пройдя чуть дальше, мы встречаем одиноко сидящего морпеха: ```cpp class PaletteContainer { public: PalEntry BaseColors[256]; // non-gamma corrected palette .... }; static void DrawPaletteTester(int paletteno) { .... for (int i = 0; i < 16; ++i) { for (int j = 0; j < 16; ++j) { PalEntry pe; if (t > 1) { auto palette = GPalette.GetTranslation(TRANSLATION_Standard, t - 2)->Palette; pe = palette[k]; } else GPalette.BaseColors[k]; // <= .... } .... } .... } ``` Предупреждение анализатора: [V607](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'GPalette\.BaseColors\[k\]'\. [d\_main\.cpp 762](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/d_main.cpp#L762) К сожалению, какая именно миссия была у него, узнать не удаётся\. Он слишком долго пробыл здесь в одиночестве и уже забыл\. **Фрагмент N6** Оставив морпеха и завернув за угол, мы встречаем первого босса — барона ада, который может доставить немало проблем: ```cpp uint8_t work[8 + // signature 12+2*4+5 + // IHDR 12+4 + // gAMA 12+256*3]; // PLTE uint32_t *const sig = (uint32_t *)&work[0]; ``` Предупреждение анализатора: [V641](https://pvs-studio.ru/ru/docs/warnings/v641/) The size of the '&work\[0\]' buffer is not a multiple of the element size of the type 'uint32\_t'\. [m\_png\.cpp 143](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/m_png.cpp#L143) Массив _work_ объявлен как массив из 829 элементов типа _uint8\_t_\. Затем указатель _sig_ инициализируется при помощи приведения указателя на массив _work_ к типу _uint32\_t\*_\. Такой код может привести к нарушению правил strict aliasing\. Массив _work_ выровнен по границе 1, а указатель _sig_ требует выравнивание по границе 4 байтов\. Если адрес начала массива не будет кратен 4, то можно получить непредсказуемый результат\. Например, процессор может отказываться работать с невыровненными данными \(ARM\)\. Проблему можно решить при помощи спецификатора _alignas_: ```cpp uint8_t alignas(uint32_t) work[....]; ``` Теперь адрес массива будет выровнен по границе 4 байтов\. Однако всё равно этот код продолжает плохо использовать память\. Число 829 как\-то не очень делится на 4, и с этим могут быть связаны ещё какие\-то проблемы\. Первая глава пройдена\. Переходим к следующей\. ## Побережье ада ![1087_gzdoom_ru/image3.png](https://import.viva64.com/docx/blog/1087_gzdoom_ru/image3.png) В коде могут встречаться различные демоны, — даже те, которые на первый взгляд кажутся хилыми, но на самом деле оказываются гораздо сильнее\. Например, вот один из таких на 1730 строке файла _vectors\.h_\. **Фрагмент N7** ```cpp constexpr DAngle nullAngle = DAngle::fromDeg(0.); ``` Казалось бы, безобидное объявление\. Что может пойти не так? Однако рядом мы замечаем лежащего раненого морпеха\. Что, если я скажу вам, что во всех юнитах трансляции, которые включили в этот заголовочный файл, будет создана своя копия этой константы? А теперь представьте, что каждая такая переменная занимает в памяти 100 байт\. И их таких 100\. И включены в 100 других файлов\. Представили? Забудьте, это не самая страшная проблема\. Гораздо хуже дела обстоят, если это объявление переменных вашего кастомного типа, который имеет нетривиальный конструктор, выполняющий тяжеловесную логику\. В таком случае, на старте приложения вам придётся немного \(вру, много\) подождать\. Предупреждение анализатора: [V1043](https://pvs-studio.ru/ru/docs/warnings/v1043/) A global object variable 'nullAngle' is declared in the header\. Multiple copies of it will be created in all translation units that include this header file\. [vectors\.h 1730](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/utility/vectors.h#L1730) Таких же объявлений ещё 11 в этом файле\. Давайте разберёмся с этим демоном, а заодно и вылечим раненого морпеха\. Если используется стандарт C\+\+17, то достаточно добавить в объявление спецификатор _inline_: ```cpp inline constexpr DAngle nullAngle = DAngle::fromDeg(0.); ``` Если же используется более старый стандарт, то придётся разделить объявление и определение: ```cpp // vectors.h extern const DAngle nullAngle; // some.cpp const DAngle nullAngle = DAngle::fromDeg(0.); ``` Всё, теперь морпех снова в строю, можно продолжать похождение\. **Фрагмент N8** Очередной противник на нашем пути\. ```cpp PalettedPixels FVoxelTexture::CreatePalettedPixels(int conversion, int frame) { uint8_t *pp = SourceVox->Palette.Data(); if (pp != nullptr) { .... } else { for (int i = 0; i < 256; i++, pp+=3) { bitmap[i] = (uint8_t)i; pe[i] = GPalette.BaseColors[i]; pe[i].a = 255; } } } ``` Предупреждение анализатора: [V769](https://pvs-studio.ru/ru/docs/warnings/v769/) The 'pp' pointer in the 'pp \+\= 3' expression equals nullptr\. The resulting value is senseless and it should not be used\. [models\_voxel\.cpp 145](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/models/models_voxel.cpp#L145) Здесь мы видим, что в ветку _else_ мы попадаем, если указатель _pp \=\= nullptr_\. Соответственно, после итерации по циклу мы получаем указатель не пойми на что, который небезопасно использовать\. Вряд ли его собирались использовать после этого\. Однако там, где есть возможность выстрелить себе в ногу, скорее всего, будет и выстрел\. Или в нашем случае, если морпех даёт возможность демону укусить себя, то демон, скорее всего, это сделает\. **Фрагмент N9** Зайдя в помещение, мы встречаем демона, который ведёт себя необычно\. Воспользуемся этим и изучим его ~~внутренности~~ подробнее\. ```cpp void DLL InitLUTs() { .... for (i=0; i<32; i++) for (j=0; j<64; j++) for (k=0; k<32; k++) { r = i << 3; g = j << 2; b = k << 3; Y = (r + g + b) >> 2; u = 128 + ((r - b) >> 2); v = 128 + ((-r + 2*g -b)>>3); } } ``` Предупреждение анализатора: * [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Unspecified behavior\. Check the shift operator '\>\>'\. The left operand is negative \('\(\- r \+ 2 \* g \- b\)' \= \[\-496\.\.504\]\)\. [hq4x\_asm\.cpp 5391](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/hires/hqnx_asm/hq4x_asm.cpp#L5391) * [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Unspecified behavior\. Check the shift operator '\>\>'\. The left operand is negative \('\(r \- b\)' \= \[\-248\.\.248\]\)\. [hq4x\_asm\.cpp 5390](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/hires/hqnx_asm/hq4x_asm.cpp#L5390) Небольшая разминка для мозга\. Данный код содержит неуточнённое поведение, если он компилируется с версией стандарта ниже C23 или C\+\+20\. Давайте разбираться, где оно спрятано\. В выражениях, значения которых присваиваются переменным _u_ и _v_, используются операторы побитового сдвига\. Проблема в том, что промежуточные значения, для которых происходит сдвиг, могут быть отрицательные\. Диапазоны для переменных _r_, _g_, _b_ и промежуточных значений приведены в комментариях справа от интересующих строк\. ```cpp void DLL InitLUTs() { .... for (i=0; i<32; i++) for (j=0; j<64; j++) for (k=0; k<32; k++) { r = i << 3; // [0 .. 248] g = j << 2; // [0 .. 252] b = k << 3; // [0 .. 248] Y = (r + g + b) >> 2; u = 128 + ((r - b) >> 2); // ([0..248] - [0..248]) >> 3 v = 128 + ((-r + 2*g -b)>>3); // (-[0..248]+[0..504]-[0..248])>>3 } } ``` Соответственно, в циклах происходит побитовый сдвиг вправо отрицательных значений, что ведёт к неуточнённому поведению\. Скорее всего, на основных платформах, где запускается DOOM, всё будет хорошо\. Но надо не забывать, что DOOM, где только не [запускают](https://www.ign.com/articles/weirdest-devices-that-run-doom-1993) :\) **Фрагмент N10** Дальше мы встречаем какого\-то непонятного демона\. Непонятного потому, что как только мы на него посмотрим, он тут же меняет облик\. ```cpp int FPCXTexture::CopyPixels(FBitmap *bmp, int conversion, int frame) { .... uint8_t c = lump.ReadUInt8(); c = 0x0c; // Apparently there's many non-compliant PCXs out there... if (c != 0x0c) { for(int i=0;i<256;i++) pe[i]=PalEntry(255,i,i,i);// default to a gray map } .... } ``` Предупреждение анализатора: [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'c' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 475, 476\. [pcxtexture\.cpp 476](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/formats/pcxtexture.cpp#L476) Мы видим, что переменную _c_ сначала инициализируют, считывая в неё значение из буфера, а затем тут же присваивают ей значение _0x0c_\. Таких демонов тут можно встретить часто: * V519 The 'dg\.mIndexIndex' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 820, 829\. v\_2ddrawer\.cpp 829 * V519 The 'dg\.mTexture' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 885, 887\. v\_2ddrawer\.cpp 887 * V519 The 'LastChar' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 226, 228\. singlelumpfont\.cpp 228 * V519 The 'flavour\.fogEquationRadial' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 164, 167\. gles\_renderstate\.cpp 167 * V519 The 'flavour\.twoDFog' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 162, 169\. gles\_renderstate\.cpp 169 * V519 The 'flavour\.fogEnabled' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 163, 170\. gles\_renderstate\.cpp 170 * V519 The 'flavour\.colouredFog' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 165, 171\. gles\_renderstate\.cpp 171 * \.\.\. и т\.д\. Но на этом странности данного фрагмента не заканчиваются: после присваивания сразу же следует проверка _if \(c \!\=0x0c\)_\. Очевидно, _then_\-ветка будет недостижима\. На это анализатор также выдаёт предупреждение: * [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'c \!\= 0x0c' is always false\. [pcxtexture\.cpp 477](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/formats/pcxtexture.cpp#L477) Что ж, пожалуй, раньше здесь было просто считывание из буфера с проверкой, но затем решили, что ветка кода должна стать недостижимой\. А может и нет, кто же знает этих демонов :\) **Фрагмент N11** В данном фрагменте два какодемона\-близнеца\. ```cpp int32_t ANIM_LoadAnim(anim_t *anim, const uint8_t *buffer, size_t length) { .... length -= sizeof(lpfileheader)+128+768; if (length < 0) return -1; .... length -= lpheader.nLps * sizeof(lp_descriptor); if (length < 0) return -2; .... } ``` Предупреждения анализатора: * [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'length < 0' is always false\. Unsigned type value is never < 0\. [animlib\.cpp 225](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/animlib.cpp#L225) * [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'length < 0' is always false\. Unsigned type value is never < 0\. [animlib\.cpp 247](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/textures/animlib.cpp#L247) Или это один и тот же, появившийся сразу в двух местах? Как мы видим, переменная _length_ имеет тип _size\_t_, который является беззнаковым\. Соответственно, проверки _length < 0_ совершенно бессмысленны\. P\.S\. Кстати, подобные ошибки могут стать причиной уязвимости\. Для игры это, пожалуй, не страшно\. А вот в приложениях, критичных с точки зрения информационной безопасности, это очень серьёзная [потенциальная уязвимость](https://pvs-studio.ru/ru/blog/terms/6441/)\. Раз некий размер неправильно вычисляется, можно на этом сыграть и попробовать переполнить какой\-то буфер\. **Фрагмент N12** А вот мы и добрались до босса этой главы — Кибердемона\. Уверен, что мало кто из читателей сталкивался с таким\. Просматривая отчет анализатора, я случайно забрёл в файл [hudmessages\.cpp](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/g_statusbar/hudmessages.cpp#L858)\. И хочу представить вашему вниманию вызов функции аж с 25 АРГУМЕНТАМИ\! ```cpp void DHUDMessageTypeOnFadeOut::DoDraw(int linenum, int x, int y, bool clean, int hudheight) { DrawText(twod, Font, TextColor, x, y, Lines[linenum].Text, DTA_VirtualWidth, HUDWidth, DTA_VirtualHeight, hudheight, DTA_ClipLeft, ClipLeft, DTA_ClipRight, ClipRight, DTA_ClipTop, ClipTop, DTA_ClipBottom, ClipBot, DTA_Alpha, Alpha, DTA_TextLen, LineVisible, DTA_RenderStyle, Style, TAG_DONE); } ``` Мне стало интересно, что же за морпех это написал\. Моему удивлению не было предела, однако, как оказалось, [объявления](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/2d/v_draw.h#L279) функций _DrawText_ не такие уж демонические: ```cpp void DrawText(F2DDrawer* drawer, FFont* font, int normalcolor, double x, double y, const char* string, int tag_first, ...); void DrawText(F2DDrawer* drawer, FFont* font, int normalcolor, double x, double y, const char32_t* string, int tag_first, ...); ``` Всего лишь 7 обязательных аргументов :\) Но вызов этой [variadic функции](https://pvs-studio.ru/ru/blog/terms/0069/) перерос аж в 25 аргументов\.\.\. Возможно, это какой\-то демонический паттерн, но как минимум существует ряд причин, почему так делать не стоит: 1. Ухудшается читаемость кода: понять, что делает такая функция, гораздо сложнее\. В данном примере из названия понятно, что функция вроде бы отрисовывает текст\. Но чтобы разобраться, что делает каждый из передаваемых аргументов, нужно потратить значительное количество времени\. А если бы функция имела другое, менее понятное название, то оставалось бы лишь заплакать\. 1. Увеличивается вероятность ошибки: шанс передать аргументы не в том порядке или не с тем значением гораздо выше, а если вам потребуется переписать её, то потребуется переписать и все места, где она вызывается, а это также увеличивает шанс ошибки\. 1. Сложнее поддерживать код: представьте, что потребуется изменить эту функцию\. Она уже принимает кучу параметров, и вам потребуется время, чтобы изменить её тело\. Но на этом всё не заканчивается, вам нужно будет ещё найти все места, где она вызывается, и поддержать нужным образом код там\. 1. Нарушается принцип единственной ответственности: скорее всего такая функция выполняет сразу несколько дел: и рисует, и жонглирует\.\.\. На практике признаком этого является именно избыточное количество аргументов\. Это приводит к усложнению и избыточности кода\. Из решений я вижу: объединить часть аргументов в какую\-либо структуру и передавать её, либо разбить эту функцию на более мелкие функции, принимающие необходимые только им аргументы\. Переходим к следующей главе\. ## Инферно ![1087_gzdoom_ru/image4.png](https://import.viva64.com/docx/blog/1087_gzdoom_ru/image4.png) **Фрагмент N13** Как и в первой главе, сразу же видим весьма странного демона: ```cpp ExpEmit FxVMFunctionCall::Emit(VMFunctionBuilder *build) { int count = 0; if (count == 1) { ExpEmit reg; if (CheckEmitCast(build, false, reg)) { ArgList.DeleteAndClear(); ArgList.ShrinkToFit(); return reg; } } .... } ``` Предупреждение анализатора: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'count \=\= 1' is always false\. [codegen\.cpp 9405](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/scripting/backend/codegen.cpp#L9405) Вероятнее всего, такой код появился в результате рефакторинга, однако всё равно выглядит страшно, когда часть логики программы просто игнорируется таким образом\. Этот демон вряд ли тянет на босса, скорее имп\. А вот следующий\.\.\. **Фрагмент N14** \.\.\. оказался призраком, который очень умело спрятался\. Попробуйте обнаружить его сами во фрагменте ниже: ```cpp static void CreateIndexedFlatVertices(FFlatVertexBuffer* fvb, TArray& sectors) { .... for (auto& sec : sectors) { for (auto ff : sec.e->XFloor.ffloors) { if (ff->top.model == &sec) { ff->top.vindex = sec.iboindex[ff->top.isceiling]; } if (ff->bottom.model == &sec) { ff->bottom.vindex = sec.iboindex[ff->top.isceiling]; } } } } ``` Тяжело, не правда ли? А вот с помощью автонаводки нашего оружия его легко заметить\. Предупреждение анализатора: [V778](https://pvs-studio.ru/ru/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'bottom' variable should be used instead of 'top'\. [hw\_vertexbuilder\.cpp 407](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/rendering/hwrenderer/hw_vertexbuilder.cpp#L407) Обратите внимание на строки, где для полей _ff\-\>top_ и _ff\-\>bottom_ выставляется _vindex_\. Они очень похожи, я бы даже сказал сильнее, чем нужно\. Дело в том, что скорее всего они появились в результате copy\-paste\. В строке с _ff\-\>bottom\.vindex_, где в качестве индекса для _sec\.iboindex_ должно выступать поле _ff\-\>bottom\.isceiling_, просто забыли поменять _top_ на _bottom_\. **Фрагмент N15** Теперь нам попадаются 3 барона ада, разберёмся с первым: ```cpp void TParseContextBase::rValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) { TIntermBinary* binaryNode = node->getAsBinaryNode(); const TIntermSymbol* symNode = node->getAsSymbolNode(); if (!node) return; .... } ``` Предупреждение анализатора: [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'node' pointer was utilized before it was verified against nullptr\. Check lines: 231, 234\. [ParseContextBase\.cpp 231](https://github.com/ZDoom/gzdoom/blob/g4.11.3/libraries/ZVulkan/src/glslang/glslang/MachineIndependent/ParseContextBase.cpp#L231) Мы видим, что здесь предусмотрели возможность появления нулевого указателя \(и даже не забыли обработать\!\), но разыменовали его до проверки\. Бах\! И неопределённое поведение\. Другие два князя ада: * V595 The 'linker' pointer was utilized before it was verified against nullptr\. Check lines: 1550, 1552\. ShaderLang\.cpp 1550 * V595 The 'mo' pointer was utilized before it was verified against nullptr\. Check lines: 6358, 6359\. p\_mobj\.cpp 6358 **Фрагмент N16** Босс этой главы — паук\-предводитель, взглянем на него: ```cpp PClassPointer::PClassPointer(PClass *restrict) : PPointer(restrict->VMType), ClassRestriction(restrict) { if (restrict) mDescriptiveName.Format("ClassPointer<%s>", restrict->TypeName.GetChars()); else mDescriptiveName = "ClassPointer"; loadOp = OP_LP; storeOp = OP_SP; Flags |= TYPE_ClassPointer; mVersion = restrict->VMType->mVersion; } ``` Предупреждение анализатора: [V664](https://pvs-studio.ru/ru/docs/warnings/v664/) The 'restrict' pointer is being dereferenced on the initialization list before it is verified against null inside the body of the constructor function\. Check lines: 1605, 1607\. [types\.cpp 1605](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/scripting/core/types.cpp#L1605) Красивый пример того, когда в конструкторе вроде бы предусмотрели проверку на разыменование нулевого указателя, но вот незадача, само разыменование происходит раньше — ещё в списке инициализации\. ## Твоя плоть съедена ![1087_gzdoom_ru/image5.png](https://import.viva64.com/docx/blog/1087_gzdoom_ru/image5.png) Финальная глава\. Думгай изнеможён и весь в крови, но до победы осталось немного\. **Фрагмент N17** ```cpp bool FScanner::GetFloat (bool evaluate) { .... if(sym && sym->tokenType == TK_IntConst && sym->tokenType != TK_FloatConst) { BigNumber = sym->Number; Number = (int)sym->Number; Float = sym->Float; // String will retain the actual symbol name. return true; } .... } ``` Здесь всё сделали правильно: перестраховались от нулевого указателя, а также проверили, что тип токена — это _TK\_IntConst_\. А затем\.\.\. ещё раз проверили, что тип токена не _TK\_FloatConst_\. Предосторожность — это хорошо, однако всё должно быть в меру, в данном случае код лишь раздулся и сделался менее читабельным\. Анализатор выдал два предупреждения: * Предупреждение анализатора: [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. [sc\_man\.cpp 829](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/engine/sc_man.cpp#L829) * Предупреждение анализатора: [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: sym\-\>tokenType \!\= TK\_FloatConst\. [sc\_man\.cpp 829](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/engine/sc_man.cpp#L829) По отчёту я нашёл ещё одно похожее место: * V590 Consider inspecting this expression\. The expression is excessive or contains a misprint\. [sc\_man\.cpp 787](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/engine/sc_man.cpp#L787) После предыдущих противников этот показался несущественным\. Но всё ещё впереди\. **Фрагмент N18** Один из сильнейших демонов, которых можно встретить в этом комплексе\. ```cpp static ASMJIT_INLINE bool X86RAPass_mustConvertSArg(X86RAPass* self, uint32_t dstTypeId, uint32_t srcTypeId) noexcept { bool dstFloatSize = dstTypeId == TypeId::kF32 ? 4 : dstTypeId == TypeId::kF64 ? 8 : 0; bool srcFloatSize = srcTypeId == TypeId::kF32 ? 4 : srcTypeId == TypeId::kF32x1 ? 4 : srcTypeId == TypeId::kF64 ? 8 : srcTypeId == TypeId::kF64x1 ? 8 : 0; if (dstFloatSize && srcFloatSize) return dstFloatSize != srcFloatSize; else return false; } ``` Предупреждение анализатора: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'dstFloatSize \!\= srcFloatSize' is always false\. [x86regalloc\.cpp 1115](https://github.com/ZDoom/gzdoom/blob/g4.11.3/libraries/asmjit/asmjit/x86/x86regalloc.cpp#L1115) Давайте разберёмся, что же здесь происходит\. В функции проверяется размер _dstTypeId_ и _srcTypeId_\. Размер может быть 0, 4, либо 8 байт\. Если размер 4 или 8 байт, то в соответствующие переменные типа _bool_ выставляется _true_\. Если размер 0 байт, то _false_\. Дальше, если размер обоих типов не 0 байт, то мы хотим узнать, различается их размер или нет\. Но тут, вместо того чтобы сравнить на неравенство размеры типов \(_dstTypeId_ и _srcTypeId_\), сравниваются сами флаги, причём после того, как мы убедились, что они оба _true_\. Соответственно, результат и поведение функции совсем не те, которые ожидались\. Функция всегда будет считать, что размеры исходного и конечного типа не совпадают, причём компиляторы [оптимизируют код](https://godbolt.org/z/Eoeje14TE) так, что останется только 2\-ой _return_\.
Занятный факт Внимательный читатель может заметить, что это код из third\-party библиотеки\. И сначала мы хотели не включать его в статью\. Однако натолкнулись на занимательный факт\. В 2017 году в проекте asmjit был открыт [тикет](https://github.com/asmjit/asmjit/issues/178) по поводу того, что компилятор GCC 7\.2 сгенерировал предупреждение на этот код\. И авторы проекта его [поправили](https://github.com/asmjit/asmjit/commit/771d66b301e60ebc3ffa69b11765622c547df6ab): ```cpp static ASMJIT_INLINE bool X86RAPass_mustConvertSArg(X86RAPass* self, uint32_t dstTypeId, uint32_t srcTypeId) noexcept { uint32_t dstFloatSize = dstTypeId == TypeId::kF32 ? 4 : // <= dstTypeId == TypeId::kF64 ? 8 : 0; uint32_t srcFloatSize = srcTypeId == TypeId::kF32 ? 4 : // <= srcTypeId == TypeId::kF32x1 ? 4 : srcTypeId == TypeId::kF64 ? 8 : srcTypeId == TypeId::kF64x1 ? 8 : 0; if (dstFloatSize && srcFloatSize) return dstFloatSize != srcFloatSize; else return false; } ``` Возможно, бравые морпехи обратят на это внимание, т\.к\. [попытки](https://github.com/ZDoom/gzdoom/commits/g4.11.3/libraries/asmjit) обновления библиотеки уже были, но пришлось откатывать изменения\.
**Фрагмент N19** Перед комнатой с финальным боссом мы встречаем ещё одного противника: ```cpp FString SuggestNewName(const ReverbContainer *env) { char text[32]; size_t len; strncpy(text, env->Name, 31); text[31] = 0; len = strlen(text); .... if (text[len - 1] != ' ' && len < 31) // <= { text[len++] = ' '; } } ``` Предупреждение анализатора: [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) The value of the 'len' index is checked after it was used\. Perhaps there is a mistake in program logic\. [s\_reverbedit\.cpp 193](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/audio/sound/s_reverbedit.cpp#L193) Фрагмент очень похожий на примеры с разыменованием указателей до проверки, но я бы назвал его улучшенной версией из\-за большей скрытности\. Тут получают доступ к элементу массива для того, чтобы проверить, что в ней не содержится символ пробела, и только потом проверяют корректность индекса, по которому происходит доступ\. Логичнее было бы сначала проверить индекс, а уже потом делать проверку элемента по этому индексу\. Ведь тогда, если индекс некорректный, то при наличии логического оператора _&&_, за счёт [вычислений по короткой схеме](https://ru.wikipedia.org/wiki/%D0%92%D1%8B%D1%87%D0%B8%D1%81%D0%BB%D0%B5%D0%BD%D0%B8%D1%8F_%D0%BF%D0%BE_%D0%BA%D0%BE%D1%80%D0%BE%D1%82%D0%BA%D0%BE%D0%B9_%D1%81%D1%85%D0%B5%D0%BC%D0%B5), разыменования по указанному индексу даже не произойдет\. Поскольку здесь максимальное значение _len_ равно 32, то выхода за границу не будет, но я бы сказал, что это снова какой\-то демонический паттерн xD\. **Фрагмент N20** Итак, встречайте\! Самый финальный босс всех финалов\. Любителей многопоточности прошу к столу\. Нелюбителей — тоже\. ```cpp void OpenALSoundRenderer::BackgroundProc() { std::unique_lock lock(StreamLock); while (!QuitThread.load()) { if (Streams.Size() == 0) { // If there's nothing to play, wait indefinitely. StreamWake.wait(lock); } else { // Else, process all active streams and sleep for 100ms for (size_t i = 0; i < Streams.Size(); i++) Streams[i]->Process(); StreamWake.wait_for(lock, std::chrono::milliseconds(100)); } } } ``` Предупреждение анализатора: [V1089](https://pvs-studio.ru/ru/docs/warnings/v1089/) Waiting on condition variable without predicate\. A thread can wait indefinitely or experience a spurious wakeup\. Consider passing a predicate as the second argument\. [oalsound\.cpp 927](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/audio/sound/oalsound.cpp#L927) В коде в одном из потоков исполнения происходит обработка контейнера _Streams_ \(consumer\)\. Если другой поток не отправил данные \(producer\), то consumer отправляют в ожидание, пока producer не пробудит его через условную переменную\. В сон поток отправляют с помощью перегрузок функций [_std::condition\_variable::wait_](https://en.cppreference.com/w/cpp/thread/condition_variable/wait) и [_std::condition\_variable::wait\_for_](https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for), не принимающих предикат в качестве второго/третьего аргумента\. Однако у условных переменных существует такая штука, как [спонтанное пробуждение](https://en.wikipedia.org/wiki/Spurious_wakeup)\. Оно означает, что producer ещё не оповещал consumer'ов пробудиться, однако consumer'ы это сделали\. При этом, судя по коду, пробуждение должно произойти в следующих ситуациях: * Контейнер _Streams_ не пуст\. Поток оповестят об этом через условную переменную _StreamWake_\. * Выполнение функции _BackgroundProc_ надо остановить\. Оповестят об этом через атомарную переменную _QuitThread_\. Из\-за спонтанного пробуждения поток может выйти из сна, и при этом _Streams_ будет пуст\. Тогда произойдёт ещё одно чтение атомарной переменной и при этом под полным барьером памяти \(перегрузка [_std::atomic::load_](https://en.cppreference.com/w/cpp/atomic/atomic/load) делает это при аргументе по умолчанию\)\. Бравые морпехи не совершили ошибки как таковой\. Но код можно сделать чуточку лучше: ```cpp void OpenALSoundRenderer::BackgroundProc() { std::unique_lock lock { StreamLock }; bool repeat = !QuitThread.load(std::memory_order_relaxed); const auto pred = [this, &repeat] { repeat = !QuitThread.load(std::memory_order_relaxed); return !repeat || Streams.Size() != 0; }; while (repeat) { // If there's nothing to play, wait indefinitely. auto cond_met = StreamWake.wait_for(lock, 100ms, pred); if (!cond_met || !repeat) { continue; } // Else, process all active streams and sleep for 100ms for (size_t i = 0; i < Streams.Size(); i++) { Streams[i]->Process(); } } } ```
Если интересно, что здесь происходит 1. Цикл теперь выполняется относительно локальной переменной _repeat_\. Она отражает состояние атомарной переменной _QuitThread_ \(нужно ли остановить consumer\)\. 1. Ослабили барьер памяти, под которым происходит чтение атомарной переменной _QuitThread_\. Судя по кодовой базе, её модификация и чтение всегда происходит под мьютексом _StreamLock_\. Мьютекс сам по себе является полным барьером памяти \(_std::memory\_order\_seq\_cst_\), следовательно, чтение и запись можно производить в режиме _std::memory\_order\_relaxed_\. Если хочется, чтобы компилятор/процессор не переупорядочивал инструкции, можно производить чтение в режиме _std::memory\_order\_acquire_, а запись — в режиме _std::memory\_order\_release_\. 1. Добавили предикат, который будет проверять готовность общих данных и исключать спонтанные пробуждения\. Предикат внутри перечитывает значение атомарной переменной _QuitThread_\. Согласно стандарту, предикат исполняется под блокировкой, поэтому чтение _QuitThread_ можно производить в режиме _std::memory\_order\_relaxed_\. 1. Оставили один вызов _std::condition\_variable::wait\_for_, который будет ставить consumer в ожидание, пока не готовы общие данные\. Для того, чтобы избежать возможного вечного зависания, каждые 100 миллисекунд будем пробуждать consumer\. Например, если кто\-то забудет вызвать _std::condition\_variable::notify\_\*_ при выставлении _QuitThread_ в _true_\. 1. Если _wait\_for_ возвращает _false_, то это означает, что за указанное время данных не поступило\. Иначе надо перепроверить, не выставил ли producer _QuitThread_ в _true_ и остановить выполнение цикла\.
## Заключение Фух\.\.\. Ну и квест нам удалось пережить\. Каких только демонов мы не встретили на своём пути\. Все читатели получают \+опыт за каждого из них\. Закончить наше приключение хочется цитатой из кодекса Некрополиса: > Теперь ты можешь вернуться к работе защитника, ибо теперь ты знаешь, зачем мы это делаем\. Теперь и вы можете вернуться к работе защитника своего кода от багов, ибо теперь знаете, что даже в коде легендарных игр, в которые играл каждый, могут быть самые разнообразные ошибки\. Всем читателям спасибо\! Надеюсь, вы приятно провели время\. С радостью прочитаю все ваши комментарии и приму участие в любом обсуждении\!