PVS-Studio: analyze the Doom 3 code

The id Software company has a license for PVS-Studio. However, we decided to check out the source code for Doom 3, which was recently uploaded to the network. The result - few errors found, but still found. I guess this can be explained as follows.

Part of the Doom3 code is used now and, probably, the errors there have already been fixed. Part of the code is outdated and not used. Most likely, it was there that suspicious parts of the code were found.

For those who are interested in this topic, I offer you the code snippets pointed to by the PVS-Studio analyzer. As always, I remind you that I consider only some warnings. Other parts of the project require knowledge of the program structure, and I have not studied them.
')


The source code for Doom 3 is published on GitHub and on the company's official FTP under the GPL v3 license. For the 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. It is better to email 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 have probably forgotten to include 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 bits are zero. The correct code is:

  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 the wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. idLib surface_polytope.cpp 65

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

  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

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

Fragment 4. Indefinite 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 between sequence points. TypeInfo filesystem.cpp 1877

The variable “dir_cache_index” is changed twice at the same point . The fact that the prefix increment is used does not matter. The compiler theoretically has the right 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 you can't be sure about that. The result can be influenced by the type and version of the compiler, optimization settings. The correct code is:

  dir_cache_index = (dir_cache_index + 1)% MAX_CACHED_DIRS; 

Fragment 5. Suspicious cleaning of the array.

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

PVS-Studio diagnostic message: V579 It is possibly a mistake. Inspect the third argument. DoomDLL megatexture.cpp 542

Only part of the 'newBlock' array is filled with zeros. This seems to be wrong. It seems to me, it used to be 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 cleaning function. The correct code is:

  memset (newBlock, 0, tileSize); 

Fragment 6. Another suspicious array cleaning.

  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

Here the arguments are confused when calling the function 'memset'. The function clears 0 bytes. This is by the way 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. DoomDLL aasfile.cpp 1312

Notice how the top two and bottom two lines of code look like. Most likely, the last two lines were written using the 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 code is:

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

I have seen other “not cleaned” arrays in the code, but it is not interesting to write about them.

Fragment 8. Suspicious work with the 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 used against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421

First, the pointer 'node' is renamed: node-> occupied. And then, it is suddenly checked whether it is not equal to NULL. This is a very suspicious code. I do not know how to make it right, because I do not know the logic of the function. It may be enough to write this:

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

Fragment 9. Suspicious row 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 the 'sprintf' function. Expected: 7. Present: 8. Game syscvar.cpp 54

Suspiciously, the argument '__TIME__' is not used at all.

Fragment 10. Code that is confusing.

The code which as I understand it repeatedly works correctly, but it looks strange. I will give only one example of such a 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 clues, the programmer wrote that the 'frustum' array consists of 4 elements. But 6 elements are processed. If you look at the 'R_ClipLineToLight' call, then there is an array of 6 elements. That is, everything should work correctly, but the code makes you wonder.

Other errors and omissions can be seen by running the PVS-Studio analyzer. By the way, taking advantage of this opportunity, I want to say hello to John Carmack and inform him that we will soon fix a flaw 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 with which the company works, 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, and not a preprocessor from Visual C ++. This will significantly speed up 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 a preprocessor from Clang - in 16 minutes. The example, however, turned out not very good. On most projects, the gain in analysis speed will be much stronger.

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