PVS-Studio: analyzing Doom 3 code

Id Software is licensed to PVS-Studio. However, we decided to check out the Doom 3 source codes that were recently posted on the network. Result - few errors were found, but still found. I suppose this can be explained as follows.

Part of Doom3 code is used now and, probably, the errors have already been fixed there. Part of the code is deprecated and not used. Most likely, this is where suspicious code fragments were found.

For those who are interested in this topic, I offer you the code snippets pointed out by the PVS-Studio analyzer. As always, I remind you that I am considering only a few warnings. Other parts of the project require knowledge of the structure of the program, and I did not study them.



Doom 3 source code published on GitHub and official FTPGPL v3 licensed companies. For analysis, I used the PVS-Studio 4.39 tool . Cracks for the program, please do not look. I have already seen trojans disguised as keys and cracks for PVS-Studio. Better write to us and we will give a trial key for a while.

Fragment 1. Suspicious condition.

#define BIT (num) (1 << (num))
const int BUTTON_ATTACK = BIT (0);
void idTarget_WaitForButton :: Think (void) {
  ...
  if (player &&
      (! player-> oldButtons & BUTTON_ATTACK) &&
      (player-> usercmd.buttons & BUTTON_ATTACK)) {
  ...
}

PVS-Studio diagnostic message: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. Game target.cpp 257

Note the snippet "! Player-> oldButtons & BUTTON_ATTACK". Here they wanted to check that the least significant bit is 0. But the priority of the '!' Operator higher than the '&' operator. This means that the condition works as follows:

(! player-> oldButtons) & 1

It turns out that the condition is true only if all the bits are zero. The correct version of the code:

if (player &&
    (! (player-> oldButtons & BUTTON_ATTACK)) &&
    (player-> usercmd.buttons & BUTTON_ATTACK)) {

Fragment 2. Suspicious cycle.

void idSurface_Polytope :: FromPlanes (...)
{
  ...
  for (j = 0; j <w.GetNumPoints (); j ++) {
    for (k = 0; k <verts.Num (); j ++) {
      if (verts [k] .xyz.Compare (w [j] .ToVec3 (),
                                POLYTOPE_VERTEX_EPSILON)) {
        break;
      }
    }
    ...
  }
  ...
}

PVS-Studio diagnostic message: V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. idLib surface_polytope.cpp 65 A

nested loop increments the variable 'j' rather than 'k'. The variable 'k' does not increase at all. The consequences of such a cycle are unpredictable. The correct version of the code:

for (k = 0; k <verts.Num (); k ++) {

Fragment 3. Another suspicious cycle.

bool idMatX :: IsOrthonormal (const float epsilon) const {
  ...
  for (int i = 0; i <numRows; i ++) {
    ...
    for (i = 1; i <numRows; i ++) {
      ...
    }
    if (idMath :: Fabs (sum)> epsilon) {
      return false;
    }
  }
  return true;
}

PVS-Studio diagnostic message: V535 The variable 'i' is being used for this loop and for the outer loop. idLib matrix.cpp 3128 A

nested loop is organized using the same variable as the outer loop. Both loops have a stop condition: i <numRows. It turns out that the outer loop always performs only one iteration. To fix the code, you can use another variable in the inner loop.

Fragment 4. Undefined behavior.

int idFileSystemLocal :: ListOSFiles (...)
{
  ...
  dir_cache_index = (++ dir_cache_index)% MAX_CACHED_DIRS;
  ...
}

PVS-Studio diagnostic message: V567 Undefined behavior. The 'dir_cache_index' variable is modified while being used twice between sequence points. TypeInfo filesystem.cpp 1877 The

variable dir_cache_index is changed twice at the same point . The fact that a prefix increment is used does not matter. The compiler is theoretically entitled to create the following code:

A = dir_cache_index;
A = A + 1;
B = A% MAX_CACHED_DIRS;
dir_cache_index = B;
dir_cache_index = A;

Of course, most likely, the expression is calculated correctly. But one cannot be sure of this. The result may be affected by the type and version of the compiler, optimization settings. The correct version of the code:

dir_cache_index = (dir_cache_index + 1)% MAX_CACHED_DIRS;

Fragment 5. Suspicious array cleaning.

void idMegaTexture :: GenerateMegaMipMaps () {
  ...
  byte * newBlock = (byte *) _ alloca (tileSize);
  ...
  memset (newBlock, 0, sizeof (newBlock));
  ...
}

PVS-Studio diagnostic message: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. DoomDLL megatexture.cpp 542

Only part of the 'newBlock' array is filled with zeros. Apparently, this is wrong. It seems to me that it was previously written like this:

byte newBlock [CONST_ARRAY_SIZE];
...
memset (newBlock, 0, sizeof (newBlock));

Then the requirements changed and the size of the 'newBlock' array began to change. But they forgot about the function of cleaning it. The correct version of the code:

memset (newBlock, 0, tileSize);

Fragment 6. Another suspicious array cleanup.

void Sys_GetCurrentMemoryStatus (sysMemoryStats_t & stats) {
  ...
  memset (& statex, sizeof (statex), 0);
  ...
}

PVS-Studio diagnostic message: V575 The 'memset' function processes '0' elements. Inspect the third argument. DoomDLL win_shared.cpp 177 Arguments are mixed

here when calling the 'memset' function. The function clears 0 bytes. This incidentally is a very common mistake. I met her many times in different projects.

Correct function call:

memset (& statex, 0, sizeof (statex));

Fragment 7. Hello, Copy-Paste.

void idAASFileLocal :: DeleteClusters (void) {
  ...
  memset (& portal, 0, sizeof (portal));
  portals.Append (portal);
  memset (& cluster, 0, sizeof (portal));
  clusters.Append (cluster);
}

PVS-Studio diagnostic message: V512 A call of the 'memset' function will lead to underflow of the buffer '& cluster'. DoomDLL aasfile.cpp 1312

Notice how the two upper and two lower lines of code are similar. Most likely, the last two lines were written using Copy-Paste technology. This led to an error. In one place, they forgot to replace the word 'portal' with the word 'cluster'. As a result, only part of the structure is cleared. The correct version of the code:

memset (& cluster, 0, sizeof (cluster));

I saw other “underdeveloped" arrays in the code, but it is not interesting to write about them.

Fragment 8. Suspicious work with a pointer.

void idBrushBSP :: FloodThroughPortals_r (idBrushBSPNode * node, ...)
{
  ...
  if (node-> occupied) {
    common-> Error ("FloodThroughPortals_r: node already occupied \ n");
  }
  if (! node) {
    common-> Error ("FloodThroughPortals_r: NULL node \ n");
  }
  ...
}

PVS-Studio diagnostic message: V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421

First, the 'node' pointer is renamed: node-> occupied. And then, suddenly it is checked if it is NULL. This is a very suspicious code. I don’t know how to make it right, because I don’t know the logic of the function. Perhaps it’s enough to write like this:

if (node ​​&& node-> occupied) {

Fragment 9. Suspicious string format.

struct gameVersion_s {
  gameVersion_s (void)
  {
    sprintf (string, "% s.% d% s% s% s",
            ENGINE_VERSION, BUILD_NUMBER, BUILD_DEBUG,
            BUILD_STRING, __DATE__, __TIME__);
  }
  char string [256];
} gameVersion;

PVS-Studio diagnostic message: V576 Incorrect format. A different number of actual arguments is expected while calling 'sprintf' function. Expected: 7. Present: 8. Game syscvar.cpp 54 It is

suspicious that the argument '__TIME__' is not used at all.

Fragment 10. Code that is confusing.

Repeatedly there is code that, as I understand it, works correctly, but it looks strange. I will give only one example of such code.

static bool R_ClipLineToLight (..., const idPlane frustum [4], ...)
{
  ...
  for (j = 0; j <6; j ++) {
    d1 = frustum [j] .Distance (p1);
    d2 = frustum [j] .Distance (p2);
    ...
  }
  ...
}

For hint, the programmer wrote that the array 'frustum' consists of 4 elements. But 6 elements are processed. If you look at the call to 'R_ClipLineToLight', then there is an array of 6 elements. That is, everything should work correctly, but the code makes you think.

Other errors and shortcomings can be seen by starting the PVS-Studio analyzer. By the way, taking this opportunity, I want to convey my regards to John Carmack and let him know that we will soon correct a defect that does not allow full use of PVS-Studio at id Software.

This drawback is the low speed of the analyzer. Given the large amount of source code that the company works with, this is a significant limitation. In PVS-Studio version 4.50, which will be released this year, it will be possible to use Clang as a preprocessor, rather than a Visual C ++ preprocessor. This will significantly accelerate the verification of projects. For example, when using a preprocessor from Visual C ++, Doom 3 source codes are checked in 26 minutes. And when using the Clang preprocessor - in 16 minutes. The example, however, was not very successful. On most projects, the gain in analysis speed will be much stronger.

True, by default, while you have to use a preprocessor from Visual C ++. Clang still has some incompatibilities and flaws regarding the Windows platform. So only 80% of projects are successfully verified.