30 years of DOOM: new code, new bugs

Today marks the 30th anniversary of the first game in the DOOM series! We couldn't miss the event. To honor the occasion, we decided to see what the code of this legendary game looks like after all these years.

Introduction

DOOM will forever go down in history as one of the greatest classic games that had a huge impact on the gaming industry. The game was revolutionary for its time. It set new gameplay and technical standards for all first-person shooters. Its fast-paced, tense gameplay, dark atmosphere, and impressive weapon arsenal have forever captured the hearts of gamers. Not to mention the amazing OST! As they say, "The heavy metal isn't playing because you're fighting demons, it's playing because the demons are fighting you!"

Obviously, it's impossible (and also boring) to read through all the thousands of code lines in an article. So, today we will literally become the Doomguy and follow a quote from the latest DOOM Eternal:

Against all the evil that hell can conjure, all the wickedness that mankind can produce. We'll send unto them, only you. Rip and tear until it is done.

We'll rip and tear all the evil that hell can conjure. And what's the worst possible evil for a programmer? Bugs, of course! We will find them and kill fix them.

GZDoom v4.11.3 serves us as a landing area. GZDoom is one of the most popular ports of the original DOOM game. And the PVS-Studio static analyzer is our assistant.

Well, let's get it started!

A note from the author: section titles correspond to the chapter titles in the game. Why? You may think that each name corresponds to a type of a bug in one way or another; for example, in the "Knee-Deep in the Dead" section, there are bugs related to dangling pointers or references, or dead code. And in the "The Shores of Hell" section... okay, stop. Actually, I just felt like it. Sounds cool, right?

By the way, a few years ago we checked the source code for a port of the DOOM Engine on Linux. We also recommend it to all the fans of the series :)

Knee-Deep in the Dead

So, we successfully landed in the space complex. We immediately see demons that have filled the station, as well as the path further down the complex. Our objective is to clear the complex of all demons.

Fragment N1

The first enemy we encounter is a zombieman who, for some reason, is pointlessly banging his head against a wall. Take your time to figure it out, and I've just aimed the crosshair on the target.

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 doesn't change here. I swear by BFG.
  }
  ....
}

The analyzer warning:

• 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

As we can see, the fuzzstep variable is declared and immediately initialized with 1 in the code snippet. After that, its value doesn't change anywhere. The while loop keeps checking the if (available % fuzzstep != 0) condition over and over again expecting changes... (damn, that's the wrong game). However, fuzzstep doesn't change anywhere, and we divide available modulo by 1 each time, and each time the result is 0, and we check it for inequality with 0, and...

Let's get this over with and move on.

Fragment N2

We see a trap in our path, someone has left it for us.

Great work by the other marines: they found a possible path for demons to take and planted a mine. But when the demons decide to come through here... nothing happens. They forgot to activate the mine...

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

The analyzer warning: V522 There might be dereferencing of a potential null pointer 'mem'. Check lines: 100, 95. fs_stringpool.cpp 100

Let's take a closer look. The mem variable is declared and immediately initialized with the result of the malloc function. As we know, malloc can return NULL, and the developers knew this as well. They even made the necessary check in the form of if (mem == nullptr) but forgot to write what to do if the condition is true. By the way, if you don't check the result of the malloc function, this article may be a good read for you.

It remains on the developers' conscience what exactly they forgot to write. Perhaps there should be a call to std::exit here, or some value returned, or something else.

Let's not risk triggering the mine and keep going.

Fragment N3

On our way, we meet an imp who doesn't even notice us or try to attack us. It does nothing at all.

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

The analyzer warning: V575 The 'memset' function processes '0' elements. Inspect the third argument. info.cpp 518

Using memset, the developers wanted to overwrite the memory that Meta points to with zeros. The problem is that we get into the else branch only if MetaSize is 0. For memset, such a call means, "Fill a memory area of 0 bytes at this address with this value" == "do nothing".

Moving on.

Fragment N4

We meet another imp who, unlike the previous one, immediately attacks.

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

The analyzer warning: V598 The 'memset' function is used to nullify the fields of 'FDecalTemplate' class. Virtual table pointer will be damaged by this. decallib.cpp 367

This is where the memset function, already discussed above, is called on the newdecal object of the FDecalTemplate type. This way they want to zeroize the object. The thing is that the FDecalTemplate type contains a pointer to a virtual table:

class FDecalTemplate : public FDecalBase { .... }

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

The sizeof operator returns the size of the object considering this pointer size. By zeroing the data members in this way, the pointer to the virtual function table is also zeroed. A good way to shoot yourself in the foot get damaged by a demon.

Fragment N5

Walking a little further, we come across a marine sitting alone:

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];                     // <=
      ....
    }
    ....
  }
  ....
}

The analyzer warning: V607 Ownerless expression 'GPalette.BaseColors[k]'. d_main.cpp 762

Unfortunately, we can't find out exactly what his mission was. He's been here alone for too long, and he's already forgotten.

Fragment N6

After leaving the marine and turning around the corner, we meet the first boss, the Baron of Hell. It can cause a lot of trouble:

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

The analyzer warning: 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

The work array is declared as an 829-element array of the uint8_t type. The sig pointer is then initialized by casting the work array pointer to the uint32_t* type.

Such code may violate the strict aliasing rules. The work array is aligned on a 1-byte boundary, and the sig pointer requires alignment on a 4-byte boundary. If the start address of the array is not a multiple of 4, we can get unpredictable results. For example, the processor may refuse to work with unaligned data (ARM).

We can resolve the issue by using the alignas specifier:

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

The array address is now aligned on a 4-byte boundary.

However, the code continues to use memory poorly. 829 is not a very good number to divide by 4, and there may be some other issues as well.

We've completed the first chapter. Let's move on to the next one.

The Shores of Hell

We can encounter different demons in the code, even ones that seem weak at first but turn out to be much stronger. For example, here's one in the 1730 line of the vectors.h file.

Fragment N7

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

The declaration seems harmless. What can go wrong? Nearby, however, we notice a wounded marine lying on the ground.

What if I told you that all translation units included in this header file would have their own copy of this constant?

Now imagine that each of these variables occupies 100 bytes in memory. And there are 100 of them. And they're included in 100 other files. Do you get the picture? Forget it, it's not the worst problem.

Things are much worse if it's a variable declaration of your custom type that has a non-trivial constructor that performs heavyweight logic. In this case, we have to wait a little (I'm lying, we have to wait a lot) when the application starts.

The analyzer warning: 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

There are 11 more such declarations in this file.

Let's deal with the demon and heal the wounded marine. When using the C++17 standard, we can just add the inline specifier to the declaration:

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

When using an older standard, we need to separate the declaration from the definition:

// vectors.h
extern const DAngle nullAngle;

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

Okay, now the marine is back in action, and we can continue our adventure.

Fragment N8

Another enemy stands in our way.

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

The analyzer warning: 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

Here we see that we get into the else branch if pp == nullptr. As a result, after traversing the loop, we get a pointer to something that isn't safe to use. I doubt the developers would use the pointer afterwards. However, if there is a chance that you could shoot yourself in the foot, this is likely to happen. Or, in our case, if a marine gives a demon a chance to bite them, the demon is likely to do so.

Fragment N9

We enter a room and meet a demon who is behaving unusually. Let's take advantage of this and examine its innards it in more detail.

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

The analyzer warning:

• 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

Here is a little warm-up exercise for your brain. The code contains unspecified behavior if it's compiled using a standard lower than C23 or C++20. Let's see where this unspecified behavior is hidden.

Bitwise shift operators are used in expressions where values are assigned to the u and v variables. The thing is that the intermediate values for which the shift occurs can be negative. The comments to the right of the lines we're interested in give the ranges for the r, g, b variables, and intermediate values.

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

So, the loops are shifted bitwise to the right of negative values resulting in unspecified behavior. Most likely, everything will be fine on the main platforms that DOOM runs on. Keep in mind, though, that people run DOOM on the weirdest of devices :)

Fragment N10

The next demon we meet is a strange one. The demon's strangeness comes from its appearance: as soon as we look at it, it changes the way it looks.

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

The analyzer warning: V519 The 'c' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 475, 476. pcxtexture.cpp 476

The c variable is first initialized by reading a value from the buffer. Then the 0x0c value is immediately assigned to it.

Such demons are common here:

• 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
• ...etc.

However, the weirdness of this fragment doesn't end there: the assignment is immediately followed by a check — if (c !=0x0c). Obviously, the then branch is unreachable. The analyzer also issues the following warning:

• V547 Expression 'c != 0x0c' is always false. pcxtexture.cpp 477

Well, maybe there was just reading from a buffer with the check, but then the devs decided that this code branch should become unreachable. Or maybe not, who knows what these demons are up to :)

Fragment N11

There are two twin cacodemons in this code fragment.

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

The analyzer warning:

• 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

Or is it the same cacodemon appearing in two places at once?

The length variable is of the size_t type, which is unsigned. So, length < 0 checks are completely meaningless.

P.S. By the way, such errors can cause vulnerabilities. It's not such a big deal for a game. However, this is a serious potential vulnerability in applications where information security is crucial. Since some size isn't calculated correctly, we can use it and try to overflow a buffer.

Fragment N12

This brings us to the boss of this chapter, the Cyberdemon. I'm sure not many readers have come across it.

While viewing the analyzer report, I accidentally opened the hudmessages.cpp file. And I want to show you a function call with up to 25 ARGUMENTS!

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

I wonder, what kind of marine would write that. However, my surprise was so great because the DrawText declarations are not so demonic after all:

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,
              ...);

Only 7 arguments are required here :) However, the call to this variadic function has grown up to 25 arguments... Maybe this is a demonic pattern, but there are a number of reasons why we shouldn't do this.

1. Code readability gets worse: it's much harder to understand what such a function does. In this example, we can see from the name that the function seems to render text. However, it takes a lot of time to figure out what each passed argument does. So, if the function had a different, less comprehensible name, all that would be left to do is to cry.
2. The probability of an error increases: we are much more likely to pass arguments in the wrong order or with the wrong value. And if we need to rewrite the function, we have to rewrite all the fragments where it's called, which also increases the chance of an error.
3. It's harder to maintain the code: imagine we need to change the function. It already takes a bunch of parameters, so it will take a while to change its body. It doesn't end there, though. We need to find all the areas where the function is called and maintain the code there.
4. The principle of sole responsibility is violated: such a function is likely to do several things at once — both drawing and juggling... In reality, this is indicated by the excessive number of arguments used. This leads to code complexity and redundancy.

Here are the solutions I see: combine some of the arguments into some structure and pass it; or break the function into smaller functions that take only the arguments they need.

Let's move on to the next chapter.

Inferno

Fragment N13

Just like in the first chapter, we see a very strange demon right away:

ExpEmit FxVMFunctionCall::Emit(VMFunctionBuilder *build)
{
  int count = 0;
  if (count == 1)
  {
    ExpEmit reg;
    if (CheckEmitCast(build, false, reg))
    {
      ArgList.DeleteAndClear();
      ArgList.ShrinkToFit();
      return reg;
    }
  }
  ....
}

The analyzer warning: V547 Expression 'count == 1' is always false. codegen.cpp 9405

The code is probably the result of refactoring, but it still looks scary when a part of the program logic is simply ignored.

This demon is hardly a boss, more like an imp. The next one, though...

Fragment N14

...turned out to be a very sneaky spectre. Try to find it in the code snippet below:

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

It's hard, isn't it? With our auto-aiming weapon, though, we can easily spot the spectre.

The analyzer warning: 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

Look at the lines where vindex is set for the ff->top and ff->bottom data members. They are very similar to each other. Even more than they have to be, I would say. The thing is, they are most likely the result of copy-paste. In the line with ff->bottom.vindex, where the ff->bottom.isceiling data member should act as an index for sec.iboindex, the developers simply forgot to change top to bottom.

Fragment N15

Now it's time to meet the three Barons of Hell. Let's deal with the first one:

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

  if (!node) return;
  ....
}

The analyzer warning: V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 231, 234. ParseContextBase.cpp 231

The developers here provided for the possibility of a null pointer (and even remembered to handle it!) but dereferenced it before checking. Bam! And there we have undefined behavior.

Here are the remaining two Barons of Hell:

• 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

Fragment N16

The boss of this chapter is the Spider Mastermind, let's take a look at it:

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

The analyzer warning: 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

This is a nice example where the constructor seems to contain a check for dereferencing a null pointer, but the issue is that the dereferencing itself happens earlier — in the member initializer list.

Thy Flesh Consumed

This is the final chapter. Doomguy is exhausted and bloodied, but the victory is just around the corner.

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

The devs have done everything right here: they've made sure that there's no null pointer. They've also checked that the token type is TK_IntConst. And then... they check again that the token type is not TK_FloatConst. Caution is good, but everything should be done in moderation. In this case, the code just becomes bloated and less readable.

The analyzer issued two warnings:

• The analyzer warning: V590 Consider inspecting this expression. The expression is excessive or contains a misprint. sc_man.cpp 829
• The analyzer warning: V560 A part of conditional expression is always true: sym->tokenType != TK_FloatConst. sc_man.cpp 829

I found another similar fragment in the report:

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

After the previous opponents, this one seemed insignificant. There's more to come, though.

Fragment N18

Meet one of the strongest demons in the complex.

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

The analyzer warning: V547 Expression 'dstFloatSize != srcFloatSize' is always false. x86regalloc.cpp 1115

Let's get to the bottom of what's going on here.

There is the size check for dstTypeId and srcTypeId in the function. The size can be 0, 4, or 8 bytes. If the size is 4 or 8 bytes, the corresponding bool variables are set to true. If it's 0 bytes, they are set to false. Next, if both types are not 0 bytes, we want to know if their sizes are different. Here, however, instead of comparing the type sizes (dstTypeId and srcTypeId) for inequality, we compare the flags themselves after making sure they are both true.

So, we get a different result and function behavior than we expected. The function always assumes that the sizes of the source and destination types don't match, and compilers optimize the code so that only the second return is left.

The attentive reader may notice that this code is from a third-party library. So, we didn't want to include it in the article at first. However, we found something interesting. In 2017, somebody opened an issue in the asmjit project: the GCC 7.2 compiler issued a warning to the code above. The project authors fixed it:

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

The brave marines may have noticed. Developers tried to update the library before but had to revert the changes.

Fragment N19

We encounter another enemy in front of the room where the final boss is waiting:

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++] = ' ';
  }
}

The analyzer warning: 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

The code fragment is very similar to the examples with pointer dereferencing before checking. However, I'd call it an improved version because it's even more sneaky. This is where an array element is accessed to check that it doesn't contain a space character, and only then it's checked that the index being accessed is correct.

It would be more logical to check the index first, and then check the element using the index. Then, if the index is incorrect, dereferencing at the specified index will not occur due to short-circuit evaluation if the logical operator && is present.

Since the max len value here is 32, it's not out of bounds, but I'd say it's some demonic pattern again xD.

Fragment N20

Behold, everybody! The ultimate final boss of all final bosses.

Multithreading fans, please gather 'round. Multithreading nonfans, you too.

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

The analyzer warning: 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

In the code fragment, one of execution threads processes the Streams (consumer) container. If another thread hasn't sent data (producer), the consumer is told to wait until the producer wakes it up via a conditional variable. The thread is put to sleep using overloads of the std::condition_variable::wait and std::condition_variable::wait_for functions which don't accept the predicate as the second/third argument.

However, conditional variables have a thing called spurious wakeup. It means that the producer hasn't yet told the consumers to wake up, but the consumers have awakened. However, if we look at the code, awakening should occur in the following situations:

• The Streams container is not empty. The thread is notified using the StreamWake conditional variable.
• The execution of the BackgroundProc function must be stopped. This is notified via the QuitThread atomic variable.

Due to spurious wakeup, the stream may come out of sleep when Streams is empty. Then another read of the atomic variable occurs, this time under the full memory barrier (the std::atomic::load overload does this with the default argument).

The brave marines made no mistakes here. However, we can enhance the code:

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
    for (size_t i = 0; i < Streams.Size(); i++)
    {
      Streams[i]->Process();
    }
  }
}

1. The loop is now executed relative to the repeat local variable. It reflects the state of the QuitThread atomic variable (whether the consumer should be stopped).
2. We have weakened the memory barrier under which the QuitThread atomic variable is read. It's always modified and read under the StreamLock mutex, according to the code base. The mutex itself is a full memory barrier (std::memory_order_seq_cst), so reading and writing can be done in the std::memory_order_relaxed mode. If you don't want the compiler/processor to reorder instructions, you can read in the std::memory_order_acquire mode and write in the std::memory_order_release mode.
3. We added a predicate that checks if the shared data is ready and excludes spurious wakeups. The predicate inside re-reads the value of the QuitThread atomic variable. According to the standard, the predicate is executed under locking, so QuitThread can be read in the std::memory_order_relaxed mode.
4. We left a call to std::condition_variable::wait_for that puts the consumer on hold until all the data is ready. To avoid a possible eternal hang, we wake the consumer up every 100 milliseconds. For example, if someone forgets to call std::condition_variable::notify_* when setting QuitThread to true.
5. If wait_for returns false, then no data has been received for the specified time. Otherwise, double-check that you have set QuitThread to true and stop the loop execution.

Conclusion

Phew... What a quest we have survived. We've met all kinds of demons along the way. All readers get +experiences for each of them.

I'd like to end our adventure with a quote from the Necropolis Codex:

Now you can return to work advocate, for now you know why we do this.

Now you can return to work advocating your code from bugs, for now you know that even the code of legendary games that everyone has played can contain various errors.