# 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\.

![1087_gzdoom/image1.png](https://import.viva64.com/docx/blog/1087_gzdoom/image1.png)

## 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](https://github.com/ZDoom/gzdoom/tree/g4.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](https://pvs-studio.com/en/) 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\]\(https://pvs\-studio\.com/en/blog/posts/cpp/0662/\) 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

![1087_gzdoom/image2.png](https://import.viva64.com/docx/blog/1087_gzdoom/image2.png)

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

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

The analyzer warning:

* [V1063](https://pvs-studio.com/en/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.com/en/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)

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

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

The analyzer warning: [V522](https://pvs-studio.com/en/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)

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](https://pvs-studio.com/en/blog/posts/cpp/0938/) 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_](https://en.cppreference.com/w/cpp/utility/program/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\.

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

The analyzer warning: [V575](https://pvs-studio.com/en/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)

Using [_memset_](https://en.cppreference.com/w/cpp/string/byte/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\.

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

The analyzer warning: [V598](https://pvs-studio.com/en/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)

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:

```cpp
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: 

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

The analyzer warning: [V607](https://pvs-studio.com/en/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)

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:

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

The analyzer warning: [V641](https://pvs-studio.com/en/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)

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:

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

![1087_gzdoom/image3.png](https://import.viva64.com/docx/blog/1087_gzdoom/image3.png)

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

```cpp
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](https://pvs-studio.com/en/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)

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:

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

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

```cpp
// 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\.

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

The analyzer warning: [V769](https://pvs-studio.com/en/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)

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

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

The analyzer warning:

* [V610](https://pvs-studio.com/en/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.com/en/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)

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

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

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](https://www.ign.com/articles/weirdest-devices-that-run-doom-1993) 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\.

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

The analyzer warning: [V519](https://pvs-studio.com/en/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)

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](https://pvs-studio.com/en/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)

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

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

The analyzer warning:

* [V547](https://pvs-studio.com/en/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.com/en/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)

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](https://pvs-studio.com/en/blog/terms/6441/) 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](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/g_statusbar/hudmessages.cpp#L858) file\. And I want to show you a function call with up to 25 ARGUMENTS\!

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

I wonder, what kind of marine would write that\. However, my surprise was so great because the _DrawText_ [declarations](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/2d/v_draw.h#L279) are not so demonic after all:

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

Only 7 arguments are required here :\) However, the call to this [variadic function](https://pvs-studio.com/en/blog/terms/0069/) 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\.
1. 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\.
1. 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\.
1. 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

![1087_gzdoom/image4.png](https://import.viva64.com/docx/blog/1087_gzdoom/image4.png)

**Fragment N13**

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

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

The analyzer warning: [V547](https://pvs-studio.com/en/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)

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:



```cpp
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](https://pvs-studio.com/en/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)

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:

```cpp
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](https://pvs-studio.com/en/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)

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:

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

The analyzer warning: [V664](https://pvs-studio.com/en/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)

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

![1087_gzdoom/image5.png](https://import.viva64.com/docx/blog/1087_gzdoom/image5.png)

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

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

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](https://pvs-studio.com/en/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)
* The analyzer warning: [V560](https://pvs-studio.com/en/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)

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](https://github.com/ZDoom/gzdoom/blob/g4.11.3/src/common/engine/sc_man.cpp#L787)

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

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

The analyzer warning: [V547](https://pvs-studio.com/en/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)

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](https://godbolt.org/z/Eoeje14TE) so that only the second _return_ is left\.

<details>
   <summary>Fun fact</summary>

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 ](https://github.com/asmjit/asmjit/issues/178) in the asmjit project: the GCC 7\.2 compiler issued a warning to the code above\. The project authors [fixed it](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;
}
```

The brave marines may have noticed\. Developers [tried](https://github.com/ZDoom/gzdoom/commits/g4.11.3/libraries/asmjit) to update the library before but had to revert the changes\.


</details>
**Fragment N19**

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

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

The analyzer warning: [V781](https://pvs-studio.com/en/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)

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](https://en.wikipedia.org/wiki/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\.

```cpp
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](https://pvs-studio.com/en/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)

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_](https://en.cppreference.com/w/cpp/thread/condition_variable/wait) and [_std::condition\_variable::wait\_for_](https://en.cppreference.com/w/cpp/thread/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](https://en.wikipedia.org/wiki/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<T\>::load_](https://en.cppreference.com/w/cpp/atomic/atomic/load) overload does this with the default argument\)\.

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

```cpp
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();
    }
  }
}
```

<details>
   <summary>If you're wondering what's going on here</summary>

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\)\.
1. 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\.
1. 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\.
1. 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_\. 
1. 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\.


</details>
## 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\. 

Thanks to all the readers\! I hope you had a good time\. I look forward to reading your comments and participating in any discussion\!