30 лет DOOM: новый код — новые баги

На днях первой игре из серии DOOM исполнилось ровно 30 лет! Мы не могли обойти стороной это событие и в честь этого решили посмотреть, как же выглядит код этой легендарной игры спустя годы.

Предисловие

DOOM навсегда останется в истории как одна из величайших классических игр, которая оказала огромное влияние на игровую индустрию. Игра стала революционной для своего времени, ввела новые стандарты в геймплее и технических возможностях для шутеров от первого лица. Её быстрая, напряжённая игровая механика, мрачная атмосфера и впечатляющий арсенал оружия навсегда запали в сердца игроков. Что уж говорить о потрясающем музыкальном сопровождении! Как говорится: "Хеви-металл играет не потому, что ты сражаешься с демонами, он играет, потому что демоны сражаются с тобой!".

Понятно, что посмотреть все тысячи строк кода в рамках одной статьи невозможно, да и скучно это. Поэтому сегодня мы буквально побудем Думгаем и последуем цитате из последней DOOM Eternal:

"Против всех исчадий из глубин преисподней, против всех нечестивцев рода людского, против легионов мы выставим тебя одного. Рви и кромсай, пока не иссякнут."

Мы будем рвать и кромсать всё то зло, что может произвести ад. А какое наихудшее зло может быть для программиста? Конечно же, баги в коде! Мы будем находить и исправлять их.

В качестве плацдарма для высадки послужит GZDoom v4.11.3 — один из самых популярных портов оригинальной игры DOOM. В качестве помощника — статический анализатор PVS-Studio.

Что ж, приступим!

Примечание автора: названия разделов соответствуют названиям глав из игры. Почему? Можно предположить, что каждое название так или иначе соответствует типу багов. Например, в разделе "По колено в трупах" будут баги, связанные с "мёртвыми" (висячими) указателями или ссылками, мертвым кодом. В разделе "Побережье ада"... ладно, стоп. На самом деле, просто я так захотел. Звучит прикольно.
Кстати, несколько лет назад мы проверяли исходный код порта DOOM Engine на Linux. Также рекомендуем её к ознакомлению всем любителям серии :)

Итак, высадка в космическом комплексе прошла успешно. Мы сразу видим демонов, заполнивших подстанцию, в которой мы находимся, а также путь дальше по комплексу. Наша задача — очистить комплекс от всех демонов.

Фрагмент N1

Первым встреченным противником становится зомби-человек, который зачем-то бессмысленно бьётся головой об стену. Дам вам время самим разобраться что к чему, я лишь пока навёл прицел на нужное место.

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 The modulo by 1 operation is meaningless. The result will always be zero. r_draw_rgba.cpp 328
V547 Expression 'available % fuzzstep != 0' is always false. r_draw_rgba.cpp 328

Как мы видим, переменная fuzzstep объявляется и сразу инициализируется значением 1 во фрагменте кода. Далее её значение нигде не меняется. В цикле while происходит проверка условия if (available % fuzzstep != 0) раз за разом, в надежде на изменения... (чёрт, кажется, это не та игра), но fuzzstep нигде не меняется, и мы каждый раз делим available по модулю на 1, и каждый раз результат 0, и мы проверяем его на неравенство с 0...

Поскорее покончим с ним и пройдём дальше.

Фрагмент N2

На нашем пути появляется ловушка, оставленная кем-то до нас.

Потрясающая работа других морпехов: возможный путь для демонов нашли, установили мину. И когда демоны решат здесь пройти, то... ничего не произойдёт. Мину не активировали...

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 There might be dereferencing of a potential null pointer 'mem'. Check lines: 100, 95. fs_stringpool.cpp 100

Давайте посмотрим внимательнее. Здесь объявляется переменная mem и сразу же инициализируется результатом функции malloc. Как известно, malloc может вернуть NULL, и об этом разработчики прекрасно знали и даже сделали необходимую проверку в виде if (mem == nullptr), но забыли написать, что делать, если условие истинно. Кстати, если вы не проверяете результат функции malloc, предлагаю прочитать эту статью.

Что именно забыли написать, остаётся на совести разработчиков. Возможно, здесь должен быть вызов std::exit, или возвращаться какое-либо значение, или что-то еще.

Не став рисковать и пытаться активировать мину, мы идём дальше.

Фрагмент N3

На пути встречается имп, который даже не замечает нас и не пытается атаковать. Он вообще ничего не делает.

void PClassActor::InitializeDefaults() { .... if (MetaSize > 0) memcpy(Meta, ParentClass->Meta, ParentClass->MetaSize); else memset(Meta, 0, MetaSize); .... }

Предупреждение анализатора: V575 The 'memset' function processes '0' elements. Inspect the third argument. info.cpp 518

Как мы видим, память, на которую указывает Meta, хотят перезаписать нулями при помощи memset. Проблема в том, что в ветку else мы попадаем только если MetaSize равен 0. Для memset такой вызов означает: "заполни по такому-то адресу таким-то значением область памяти размером 0 байт" == "ничего не делай".

Проходим дальше.

Фрагмент N4

Мы натыкаемся на импа, который, в отличие от предыдущего, тут же нападает.

void FDecalLib::ParseDecal (FScanner &sc) { FDecalTemplate newdecal; .... memset ((void *)&newdecal, 0, sizeof(newdecal)); .... }

Предупреждение анализатора: V598 The 'memset' function is used to nullify the fields of 'FDecalTemplate' class. Virtual table pointer will be damaged by this. decallib.cpp 367

Здесь вызывается уже рассмотренная выше функция memset для объекта newdecal типа FDecalTemplate. Таким образом хотят занулить обьект. Проблема в том, что тип FDecalTemplate содержит в себе указатель на виртуальную таблицу:

class FDecalTemplate : public FDecalBase { .... } class FDecalBase { .... virtual const FDecalTemplate *GetDecal () const; virtual void ReplaceDecalRef (FDecalBase *from, FDecalBase *to) = 0; .... };

Оператор sizeof же вернёт размер объекта с учётом размера этого указателя. Обнуляя поля таким образом, занулится также и указатель на таблицу виртуальных функций. Хороший способ получить дамаг от удара демона.

Фрагмент N5

Пройдя чуть дальше, мы встречаем одиноко сидящего морпеха:

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 Ownerless expression 'GPalette.BaseColors[k]'. d_main.cpp 762

К сожалению, какая именно миссия была у него, узнать не удаётся. Он слишком долго пробыл здесь в одиночестве и уже забыл.

Фрагмент N6

Оставив морпеха и завернув за угол, мы встречаем первого босса — барона ада, который может доставить немало проблем:

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 The size of the '&work[0]' buffer is not a multiple of the element size of the type 'uint32_t'. m_png.cpp 143

Массив work объявлен как массив из 829 элементов типа uint8_t. Затем указатель sig инициализируется при помощи приведения указателя на массив work к типу uint32_t*.

Такой код может привести к нарушению правил strict aliasing. Массив work выровнен по границе 1, а указатель sig требует выравнивание по границе 4 байтов. Если адрес начала массива не будет кратен 4, то можно получить непредсказуемый результат. Например, процессор может отказываться работать с невыровненными данными (ARM).

Проблему можно решить при помощи спецификатора alignas:

uint8_t alignas(uint32_t) work[....];

Теперь адрес массива будет выровнен по границе 4 байтов.

Однако всё равно этот код продолжает плохо использовать память. Число 829 как-то не очень делится на 4, и с этим могут быть связаны ещё какие-то проблемы.

Первая глава пройдена. Переходим к следующей.

В коде могут встречаться различные демоны, — даже те, которые на первый взгляд кажутся хилыми, но на самом деле оказываются гораздо сильнее. Например, вот один из таких на 1730 строке файла vectors.h.

Фрагмент N7

constexpr DAngle nullAngle = DAngle::fromDeg(0.);

Казалось бы, безобидное объявление. Что может пойти не так? Однако рядом мы замечаем лежащего раненого морпеха.

Что, если я скажу вам, что во всех юнитах трансляции, которые включили в этот заголовочный файл, будет создана своя копия этой константы?

А теперь представьте, что каждая такая переменная занимает в памяти 100 байт. И их таких 100. И включены в 100 других файлов. Представили? Забудьте, это не самая страшная проблема.

Гораздо хуже дела обстоят, если это объявление переменных вашего кастомного типа, который имеет нетривиальный конструктор, выполняющий тяжеловесную логику. В таком случае, на старте приложения вам придётся немного (вру, много) подождать.

Предупреждение анализатора: 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

Таких же объявлений ещё 11 в этом файле.

Давайте разберёмся с этим демоном, а заодно и вылечим раненого морпеха. Если используется стандарт C++17, то достаточно добавить в объявление спецификатор inline:

inline constexpr DAngle nullAngle = DAngle::fromDeg(0.);

Если же используется более старый стандарт, то придётся разделить объявление и определение:

// vectors.h extern const DAngle nullAngle; // some.cpp const DAngle nullAngle = DAngle::fromDeg(0.);

Всё, теперь морпех снова в строю, можно продолжать похождение.

Фрагмент N8

Очередной противник на нашем пути.

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 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

Здесь мы видим, что в ветку else мы попадаем, если указатель pp == nullptr. Соответственно, после итерации по циклу мы получаем указатель не пойми на что, который небезопасно использовать. Вряд ли его собирались использовать после этого. Однако там, где есть возможность выстрелить себе в ногу, скорее всего, будет и выстрел. Или в нашем случае, если морпех даёт возможность демону укусить себя, то демон, скорее всего, это сделает.

Фрагмент N9

Зайдя в помещение, мы встречаем демона, который ведёт себя необычно. Воспользуемся этим и изучим его подробнее.

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 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('(- r + 2 * g - b)' = [-496..504]). hq4x_asm.cpp 5391
V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('(r - b)' = [-248..248]). hq4x_asm.cpp 5390

Небольшая разминка для мозга. Данный код содержит неуточнённое поведение, если он компилируется с версией стандарта ниже C23 или C++20. Давайте разбираться, где оно спрятано.

В выражениях, значения которых присваиваются переменным u и v, используются операторы побитового сдвига. Проблема в том, что промежуточные значения, для которых происходит сдвиг, могут быть отрицательные. Диапазоны для переменных r, g, b и промежуточных значений приведены в комментариях справа от интересующих строк.

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, где только не запускают :)

Фрагмент N10

Дальше мы встречаем какого-то непонятного демона. Непонятного потому, что как только мы на него посмотрим, он тут же меняет облик.

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 The 'c' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 475, 476. pcxtexture.cpp 476

Мы видим, что переменную 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 Expression 'c != 0x0c' is always false. pcxtexture.cpp 477

Что ж, пожалуй, раньше здесь было просто считывание из буфера с проверкой, но затем решили, что ветка кода должна стать недостижимой. А может и нет, кто же знает этих демонов :)

Фрагмент N11

В данном фрагменте два какодемона-близнеца.

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 Expression 'length < 0' is always false. Unsigned type value is never < 0. animlib.cpp 225
V547 Expression 'length < 0' is always false. Unsigned type value is never < 0. animlib.cpp 247

Или это один и тот же, появившийся сразу в двух местах?

Как мы видим, переменная length имеет тип size_t, который является беззнаковым. Соответственно, проверки length < 0 совершенно бессмысленны.

P.S. Кстати, подобные ошибки могут стать причиной уязвимости. Для игры это, пожалуй, не страшно. А вот в приложениях, критичных с точки зрения информационной безопасности, это очень серьёзная потенциальная уязвимость. Раз некий размер неправильно вычисляется, можно на этом сыграть и попробовать переполнить какой-то буфер.

Фрагмент N12

А вот мы и добрались до босса этой главы — Кибердемона. Уверен, что мало кто из читателей сталкивался с таким.

Просматривая отчет анализатора, я случайно забрёл в файл hudmessages.cpp. И хочу представить вашему вниманию вызов функции аж с 25 АРГУМЕНТАМИ!

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); }

Мне стало интересно, что же за морпех это написал. Моему удивлению не было предела, однако, как оказалось, объявления функций DrawText не такие уж демонические:

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 функции перерос аж в 25 аргументов... Возможно, это какой-то демонический паттерн, но как минимум существует ряд причин, почему так делать не стоит:

Ухудшается читаемость кода: понять, что делает такая функция, гораздо сложнее. В данном примере из названия понятно, что функция вроде бы отрисовывает текст. Но чтобы разобраться, что делает каждый из передаваемых аргументов, нужно потратить значительное количество времени. А если бы функция имела другое, менее понятное название, то оставалось бы лишь заплакать.
Увеличивается вероятность ошибки: шанс передать аргументы не в том порядке или не с тем значением гораздо выше, а если вам потребуется переписать её, то потребуется переписать и все места, где она вызывается, а это также увеличивает шанс ошибки.
Сложнее поддерживать код: представьте, что потребуется изменить эту функцию. Она уже принимает кучу параметров, и вам потребуется время, чтобы изменить её тело. Но на этом всё не заканчивается, вам нужно будет ещё найти все места, где она вызывается, и поддержать нужным образом код там.
Нарушается принцип единственной ответственности: скорее всего такая функция выполняет сразу несколько дел: и рисует, и жонглирует... На практике признаком этого является именно избыточное количество аргументов. Это приводит к усложнению и избыточности кода.

Из решений я вижу: объединить часть аргументов в какую-либо структуру и передавать её, либо разбить эту функцию на более мелкие функции, принимающие необходимые только им аргументы.

Переходим к следующей главе.

Фрагмент N13

Как и в первой главе, сразу же видим весьма странного демона:

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 Expression 'count == 1' is always false. codegen.cpp 9405

Вероятнее всего, такой код появился в результате рефакторинга, однако всё равно выглядит страшно, когда часть логики программы просто игнорируется таким образом.

Этот демон вряд ли тянет на босса, скорее имп. А вот следующий...

Фрагмент N14

... оказался призраком, который очень умело спрятался. Попробуйте обнаружить его сами во фрагменте ниже:

static void CreateIndexedFlatVertices(FFlatVertexBuffer* fvb, TArray<sector_t>& 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 Two similar code fragments were found. Perhaps, this is a typo and 'bottom' variable should be used instead of 'top'. hw_vertexbuilder.cpp 407

Обратите внимание на строки, где для полей ff->top и ff->bottom выставляется vindex. Они очень похожи, я бы даже сказал сильнее, чем нужно. Дело в том, что скорее всего они появились в результате copy-paste. В строке с ff->bottom.vindex, где в качестве индекса для sec.iboindex должно выступать поле ff->bottom.isceiling, просто забыли поменять top на bottom.

Фрагмент N15

Теперь нам попадаются 3 барона ада, разберёмся с первым:

void TParseContextBase::rValueErrorCheck(const TSourceLoc& loc, const char* op, TIntermTyped* node) { TIntermBinary* binaryNode = node->getAsBinaryNode(); const TIntermSymbol* symNode = node->getAsSymbolNode(); if (!node) return; .... }

Предупреждение анализатора: V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 231, 234. ParseContextBase.cpp 231

Мы видим, что здесь предусмотрели возможность появления нулевого указателя (и даже не забыли обработать!), но разыменовали его до проверки. Бах! И неопределённое поведение.

Другие два князя ада:

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

Босс этой главы — паук-предводитель, взглянем на него:

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 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

Красивый пример того, когда в конструкторе вроде бы предусмотрели проверку на разыменование нулевого указателя, но вот незадача, само разыменование происходит раньше — ещё в списке инициализации.

Финальная глава. Думгай изнеможён и весь в крови, но до победы осталось немного.

Фрагмент N17

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 Consider inspecting this expression. The expression is excessive or contains a misprint. sc_man.cpp 829
Предупреждение анализатора: V560 A part of conditional expression is always true: sym->tokenType != TK_FloatConst. sc_man.cpp 829

По отчёту я нашёл ещё одно похожее место:

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. sc_man.cpp 787

После предыдущих противников этот показался несущественным. Но всё ещё впереди.

Фрагмент N18

Один из сильнейших демонов, которых можно встретить в этом комплексе.

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 Expression 'dstFloatSize != srcFloatSize' is always false. x86regalloc.cpp 1115

Давайте разберёмся, что же здесь происходит.

В функции проверяется размер dstTypeId и srcTypeId. Размер может быть 0, 4, либо 8 байт. Если размер 4 или 8 байт, то в соответствующие переменные типа bool выставляется true. Если размер 0 байт, то false. Дальше, если размер обоих типов не 0 байт, то мы хотим узнать, различается их размер или нет. Но тут, вместо того чтобы сравнить на неравенство размеры типов (dstTypeId и srcTypeId), сравниваются сами флаги, причём после того, как мы убедились, что они оба true.

Соответственно, результат и поведение функции совсем не те, которые ожидались. Функция всегда будет считать, что размеры исходного и конечного типа не совпадают, причём компиляторы оптимизируют код так, что останется только 2-ой return.

Занятный факт.Внимательный читатель может заметить, что это код из third-party библиотеки. И сначала мы хотели не включать его в статью. Однако натолкнулись на занимательный факт. В 2017 году в проекте asmjit был открыт тикет по поводу того, что компилятор GCC 7.2 сгенерировал предупреждение на этот код. И авторы проекта его поправили:

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; }

Возможно, бравые морпехи обратят на это внимание, т.к. попытки обновления библиотеки уже были, но пришлось откатывать изменения.

Фрагмент N19

Перед комнатой с финальным боссом мы встречаем ещё одного противника:

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 The value of the 'len' index is checked after it was used. Perhaps there is a mistake in program logic. s_reverbedit.cpp 193

Фрагмент очень похожий на примеры с разыменованием указателей до проверки, но я бы назвал его улучшенной версией из-за большей скрытности. Тут получают доступ к элементу массива для того, чтобы проверить, что в ней не содержится символ пробела, и только потом проверяют корректность индекса, по которому происходит доступ.

Логичнее было бы сначала проверить индекс, а уже потом делать проверку элемента по этому индексу. Ведь тогда, если индекс некорректный, то при наличии логического оператора &&, за счёт вычислений по короткой схеме, разыменования по указанному индексу даже не произойдет.

Поскольку здесь максимальное значение len равно 32, то выхода за границу не будет, но я бы сказал, что это снова какой-то демонический паттерн xD.

Фрагмент N20

Итак, встречайте! Самый финальный босс всех финалов.

Любителей многопоточности прошу к столу. Нелюбителей — тоже.

void OpenALSoundRenderer::BackgroundProc() { std::unique_lock<std::mutex> 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 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

В коде в одном из потоков исполнения происходит обработка контейнера Streams (consumer). Если другой поток не отправил данные (producer), то consumer отправляют в ожидание, пока producer не пробудит его через условную переменную. В сон поток отправляют с помощью перегрузок функций std::condition_variable::wait и std::condition_variable::wait_for, не принимающих предикат в качестве второго/третьего аргумента.

Однако у условных переменных существует такая штука, как спонтанное пробуждение. Оно означает, что producer ещё не оповещал consumer'ов пробудиться, однако consumer'ы это сделали. При этом, судя по коду, пробуждение должно произойти в следующих ситуациях:

Контейнер Streams не пуст. Поток оповестят об этом через условную переменную StreamWake.
Выполнение функции BackgroundProc надо остановить. Оповестят об этом через атомарную переменную QuitThread.

Из-за спонтанного пробуждения поток может выйти из сна, и при этом Streams будет пуст. Тогда произойдёт ещё одно чтение атомарной переменной и при этом под полным барьером памяти (перегрузка std::atomic<T>::load делает это при аргументе по умолчанию).

Бравые морпехи не совершили ошибки как таковой. Но код можно сделать чуточку лучше:

void OpenALSoundRenderer::BackgroundProc() { std::unique_lock<std::mutex> 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).
2. Ослабили барьер памяти, под которым происходит чтение атомарной переменной QuitThread. Судя по кодовой базе, её модификация и чтение всегда происходит под мьютексом StreamLock. Мьютекс сам по себе является полным барьером памяти (std::memory_order_seq_cst), следовательно, чтение и запись можно производить в режиме std::memory_order_relaxed. Если хочется, чтобы компилятор/процессор не переупорядочивал инструкции, можно производить чтение в режиме std::memory_order_acquire, а запись — в режиме std::memory_order_release.
3. Добавили предикат, который будет проверять готовность общих данных и исключать спонтанные пробуждения. Предикат внутри перечитывает значение атомарной переменной QuitThread. Согласно стандарту, предикат исполняется под блокировкой, поэтому чтение QuitThread можно производить в режиме std::memory_order_relaxed.
4. Оставили один вызов std::condition_variable::wait_for, который будет ставить consumer в ожидание, пока не готовы общие данные. Для того, чтобы избежать возможного вечного зависания, каждые 100 миллисекунд будем пробуждать consumer. Например, если кто-то забудет вызвать std::condition_variable::notify_* при выставлении QuitThread в true.
5. Если wait_for возвращает false, то это означает, что за указанное время данных не поступило. Иначе надо перепроверить, не выставил ли producer QuitThread в true и остановить выполнение цикла.

Фух... Ну и квест нам удалось пережить. Каких только демонов мы не встретили на своём пути. Все читатели получают +опыт за каждого из них.

Закончить наше приключение хочется цитатой из кодекса Некрополиса:

"Теперь ты можешь вернуться к работе защитника, ибо теперь ты знаешь, зачем мы это делаем."

Теперь и вы можете вернуться к работе защитника своего кода от багов, ибо теперь знаете, что даже в коде легендарных игр, в которые играл каждый, могут быть самые разнообразные ошибки.

Всем читателям спасибо! Надеюсь, вы приятно провели время. С радостью прочитаю все ваши комментарии и приму участие в любом обсуждении!

30 лет DOOM: новый код — новые баги

По колено в трупах

Побережье ада

Инферно

Твоя плоть съедена

Заключение