# In the world of anthropomorphic animals: PVS\-Studio checks Overgrowth

Recently, Wolfire Games released Overgrowth's source code\. We couldn't but check the game's quality with the help of PVS\-Studio\. Let's see where you can find the coolest action: in the game or in its source code\!

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

## Project overview

Overgrowth is a 3rd person action game released by Wolfire Games 14 years ago\. It is set in a dark medieval world inhabited by humanized animals\. The game provides a fascinating control system and a fairly advanced AI\. In this game, players are free to choose where to go and what to do\. You can also play Overgrowth multiplayer\.

The game uses the Phoenix Engine\. It supports an advanced motion model\. You can actually feel the smoothness of every run, jump, roll, and turn\. The game environment, characters' mood and personality affect their postures and game animations\. The game environment is weather dependent — even trees grow faster when the sun shines\.

Overgrowth was announced on September 17, 2008\. The developers released the game on October 16, 2017\.

Since developers released the source code, community members haven't stopped committing to the project\. So, I chose the [f2a67f7](https://github.com/WolfireGames/overgrowth/tree/f2a67f79e3205a45bd90ed5021276b9e6aa86132) version to check\.

Let's discuss the most interesting warnings that PVS\-Studio found when analyzing the project\.

## Analysis results

### Warnings N1, N2

Well, let's start with a function that triggered the analyzer twice — PVS\-Studio issued two warnings on adjacent code lines\.

* [V611](https://pvs-studio.com/en/docs/warnings/v611/) \[CERT\-MEM51\-CPP\] The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] heightfieldData;'\. PhysicsServerCommandProcessor\.cpp 4741
* [V773](https://pvs-studio.com/en/docs/warnings/v773/) \[CERT\-MEM31\-C, CERT\-MEM51\-CPP\] The function was exited without releasing the 'worldImporter' pointer\. A memory leak is possible\. PhysicsServerCommandProcessor\.cpp 4742

```cpp
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....
  delete heightfieldData;
  return ....;
}
```

Seems like the developer who wrote this function is not very familiar with dynamic memory in C\+\+\.

Let's first discuss the [V773](https://pvs-studio.com/en/docs/warnings/v773/) warning as more trivial\. Developers use the _new_ operator to allocate the memory for the _worldImporter_ pointer\. But at the end of the function, they forget to deallocate memory\. This is a bad practice that leads to memory leaks\. One way to fix this code fragment is to call the _delete_ operator when finish working with this pointer\.

Let's proceed to the [V611](https://pvs-studio.com/en/docs/warnings/v611/) warning and the _heightfieldData_ buffer\. Developers wanted to deallocate memory allocated with the _new\[\]_ operator\. However, to do this they used the _delete_ operator instead of the _delete\[\]_ one\. According to the standard, such code leads to undefined behavior\. Here is the [link](https://timsong-cpp.github.io/cppwp/n4861/expr.delete#2) to the corresponding item\.

And that's how we can fix this code fragment:

```cpp
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....

  delete   worldImporter;
  delete[] heightfieldData;
  return ....;
}
```

Also, developers can avoid problems with manual memory deallocation by using modern coding techniques\. For example, they can call _std::unique\_ptr_ to deallocate memory automatically\. This makes code shorter and more secure\. Such code also protects against unallocated memory errors if early return happens:

```cpp
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  auto worldImporter = std::make_unique<btMultiBodyWorldImporter> ();
  ....
  std::unique_ptr<unsigned char[]> heightfieldData;
  ....
  heightfieldData = std::make_unique_for_overwrite<unsigned char[]>
                                (width * height * sizeof(btScalar));
  ....
  return ....;
}
```

### Warning N3

[V772](https://pvs-studio.com/en/docs/warnings/v772/) \[CERT\-MSC15\-C\] Calling a 'delete' operator for a void pointer will cause undefined behavior\. OVR\_CAPI\_Util\.cpp 380

```cpp
typedef struct ovrHapticsClip_
{
  const void* Samples;
  ....
} ovrHapticsClip;
....

OVR_PUBLIC_FUNCTION(void) ovr_ReleaseHapticsClip(ovrHapticsClip* hapticsClip)
{
  if (hapticsClip != NULL && hapticsClip->Samples != NULL) 
  {
    delete[] hapticsClip->Samples;
  ....
  }
}
```

The_ delete_ and _delete\[\]_ operators used for a pointer to _void_ lead to [undefined behavior](https://timsong-cpp.github.io/cppwp/n4861/expr.delete#footnote-73)\. To avoid an error, the developer deallocating memory needs to explicitly cast the pointer to its actual type\.

To understand the problem better I inspected the code manually\. Here's what I found — the _Samples_ field is initialized only once and is of the _uint8\_t\*_ type\. Here's the proof:

```cpp
.... ovr_GenHapticsFromAudioData(ovrHapticsClip* outHapticsClip, ....)
{
  ....
  uint8_t* hapticsSamples = new uint8_t[hapticsSampleCount];
  ....

  outHapticsClip->Samples = hapticsSamples;

  ....
}
```

This indicates an architectural error in the class design\. Developers may have used different types to initialize the field and removed them during refactoring\. But they forgot to change the _Samples_ field's type from _void\*_ to _uint8\_t\*_\.

Anyway, this code fragment looks strange and leads to UB\. It would be better to double\-check it\.

### Warning N4

[V595](https://pvs-studio.com/en/docs/warnings/v595/) \[CERT\-EXP12\-C\] The 'ctx' pointer was utilized before it was verified against nullptr\. Check lines: 130, 131\. ascontext\.cpp 130

```cpp
class ASContext
{
public:
  asIScriptContext *ctx;
}

ASContext::ASContext(....)
{
  ctx = ....;
  ctx->SetUserData(this, 0);
  if( ctx == 0 ) 
  {
    FatalError("Error","Failed to create the context.");
    return;
  }
  ....
}
```

In this code fragment, developers first dereference the _ctx_ pointer and then check it for _0_ — looks pretty suspicious\. If _ctx_ could be equal to _nullptr_, it would be a good idea to first check _ctx_, and only then dereference it:

```cpp
ASContext::ASContext(....)
{
  ctx = ....;
  if( !ctx )
  {
    FatalError("Error","Failed to create the context.");
    return;
  }

  ctx->SetUserData(this, 0);
  ....
}
```

### Warning N5

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'connect\_id\_ \=\= \- 1' is always true\. placeholderobject\.cpp 342

```cpp
class PlaceholderObject
{
private:
  int connect_id_;
  ....
};

ObjectSanityState PlaceholderObject::GetSanity()
{
  ....
  if( .... && connect_id_ == -1) 
  {
    if( connect_id_ == -1) 
    {
      ....
    } 
  } 
  ....
}
```

The analyzer detects a redundant _connect\_id\_ \=\= \-1_ check in this code fragment\. The outer _if_ statement condition already contains this check\. The _connect\_id\__ variable has not changed since then\.

Perhaps the condition that triggered the analyzer should check some other variable\. Otherwise, this check is redundant — the developer can simplify the code as follows:

```cpp
ObjectSanityState PlaceholderObject::GetSanity()
{
  ....
  if( .... && connect_id_ == -1 ) 
  {
      ....
  } 
  ....
}
```

### Warning N6

[V791](https://pvs-studio.com/en/docs/warnings/v791/) The initial value of the index in the nested loop equals 'i'\. Perhaps, 'i \+ 1' should be used instead\. navmeshhintobject\.cpp 65

```cpp
NavmeshHintObject::NavmeshHintObject()
{
  ....
  for( int i = 0; i < 8; i++ )
  {
    for( int k = i; k < 8; k++ )
    {
      if( i != k )
      {
        if( 
            corners[i][0] == corners[k][0] ||
            corners[i][1] == corners[k][1] ||
            corners[i][2] == corners[k][2] 
          )
          {
            cross_marking.push_back(corners[i]);   
            cross_marking.push_back(corners[k]);   
          }
      }
    }
  }
  ....
}
```

Here the analyzer finds a non\-optimal loop\. The loop contains a code pattern that performs several operations for pairs of array elements\. It's useless to perform an operation for a pair consisting of the same _i \=\= j_ element\. So, we can simplify this code fragment:

```cpp
NavmeshHintObject::NavmeshHintObject()
{
  ....
  for( int i = 0; i < 8; i++ )
  {
    for( int k = i + 1; k < 8; k++ )
    {
      if( 
          corners[i][0] == corners[k][0] ||
          corners[i][1] == corners[k][1] ||
          corners[i][2] == corners[k][2] 
        )
        {
          cross_marking.push_back(corners[i]);   
          cross_marking.push_back(corners[k]);   
        }
    }
  }
  ....
}
```

### Warning N7

[V561](https://pvs-studio.com/en/docs/warnings/v561/) \[CERT\-DCL01\-C\] It's probably better to assign value to 'other\_radius\_sq' variable than to declare it anew\. Previous declaration: scenegraph\.cpp, line 2006\. scenegraph\.cpp 2010

```cpp
bool SceneGraph::AddDynamicDecal(....)
{
  ....
  float other_radius_sq = ....;
  if(....)
  {
    ....
    float other_radius_sq = ....;
  }
  ....
}
```

This suspicious code fragment also triggers the analyzer\. Here the _other\_radius\_sq_ variable is redefined\. Entities with identical names often appear when code has been copy\-pasted\.

### Warnings N8, N9

* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'imageBits \=\= 8' is always false\. texture\_data\.cpp 305
* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'imageBits \=\= 24' is always false\. texture\_data\.cpp 313

```cpp
void TextureData::GetUncompressedData(unsigned char* data) 
{
  int imageBits = 32;
  ....
  if (imageBits == 8)
  {
    ....
  }
  else if (imageBits == 24)
  {
    ....
  }
  ....
}
```

The_ imageBits_'s value is not changed between the variable's initialization and checks\. This doesn't look like an actual error — just a strange unfinished or redundant code fragment\. Probably a good candidate for code review\!

### Warnings N10, N11

[V769](https://pvs-studio.com/en/docs/warnings/v769/) \[CERT\-EXP08\-C\] The 'idx\_buffer\_offset' pointer in the 'idx\_buffer\_offset \+\= pcmd\-\>ElemCount' expression equals nullptr\. The resulting value is senseless and it should not be used\. imgui\_impl\_sdl\_gl3\.cpp 138

```cpp
void ImGui_ImplSdlGL3_RenderDrawLists(ImDrawData* draw_data)
{
  const ImDrawIdx* idx_buffer_offset = 0;
  ....
  idx_buffer_offset += pcmd->ElemCount;
  ....
}
```

The analyzer detects a suspicious addition operation applied to the null pointer\. The pointer is not used further\. Moreover, it can't be used\. Either way, the purpose of this code is not very clear\.

Here's one more similar warning:

[V769](https://pvs-studio.com/en/docs/warnings/v769/) \[CERT\-EXP08\-C\] The 'cp' pointer in the 'cp \+\+' expression equals nullptr\. The resulting value is senseless and it should not be used\. crn\_file\_utils\.cpp 547

```cpp
int file_utils::wildcmp(...., const char* pString)
{
  const char* cp = NULL;
  ....
  pString = cp++;
  ....
}
```

Someone may have a mistake during refactoring or algorithm\. We can only guess what developers had in mind\.\.\.

### Warning N12

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. skeleton\.cpp 152

```cpp
void Skeleton::SetGravity( bool enable ) 
{
  if(enable)
  {
    for(unsigned i=0; i<physics_bones.size(); i++)
    {
      if(!physics_bones[i].bullet_object)
      {
        continue;
      }
      physics_bones[i].bullet_object->SetGravity(true);
      //physics_bones[i].bullet_object->SetDamping(0.0f);
    }
  } 
  else 
  {
    for(unsigned i=0; i<physics_bones.size(); i++)
    {
      if(!physics_bones[i].bullet_object)
      {
        continue;
      }
      physics_bones[i].bullet_object->SetGravity(true);
      //physics_bones[i].bullet_object->SetDamping(1.0f);
    }
  }
}
```

Let's discuss one more strange code fragment\. The analyzer detects the _if_ statement that has identical _then_ and _else_ branches\. In the two condition branches, the commented\-out code fragments are different\. This could mean that the developer just forgot to finish the second code fragment\.

### Warning N13

[V728](https://pvs-studio.com/en/docs/warnings/v728/) An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. as\_compiler\.cpp 4317

```cpp
void asCCompiler::CompileIfStatement(....)
{
  bool constructorCall1 = ....;
  bool constructorCall2 = ....;
  ....
  if (  (constructorCall1 && !constructorCall2) 
      ||(constructorCall2 && !constructorCall1) )
  {
    ....
  }
}
```

Let's take a look at the code fragment that does not actually contain an error\. To be honest, I just really like this diagnostic\. It is simple and elegant\.

PVS\-Studio detects the pattern in the checked condition\. It would be better if developers simplified this condition — this would make the code more readable\. The developer is trying to understand which of the constructors was called\. The performed operation is very similar to _XOR_\. But C\+\+ has no _exclusive "OR"_ for the _bool_ type\. That's why sometimes it results in spaghetti code\. Here's one way to make the code fragment more simple:

```cpp
void asCCompiler::CompileIfStatement(....)
{
  bool constructorCall1 = ....;
  bool constructorCall2 = ....;
  ....
  if (constructorCall1 != constructorCall2)
  {
    ....
  }
}
```

### Warnings N14, N15, N16

[V610](https://pvs-studio.com/en/docs/warnings/v610/) \[CERT\-INT34\-C\] Undefined behavior\. Check the shift operator '<<'\. The right operand \('i' \= \[0\.\.63\]\) is greater than or equal to the length in bits of the promoted left operand\. bitarray\.cpp 77

```cpp
class Bitarray 
{
private:
  uint64_t *arr;
  ....
};

void Bitarray::SetBit( size_t index )
{
  size_t p = index/64;
  size_t i = index%64;

  arr[p] |= (1UL << i);
}
```

PVS\-Studio finds a dangerous code fragment containing a left shift of an unsigned character\. According to the standard, if the right operand is greater than or equal to the left operand — this is undefined behavior\. The _1UL_ literal on _MSVC_ is represented by _32_ bits while the right operand is in the range from _0_ to _63_\.

Since this code is for builds on Windows too, it would be better to double\-check the code\. Here are other warnings describing the same issue:

* [V610](https://pvs-studio.com/en/docs/warnings/v610/) \[CERT\-INT34\-C\] Undefined behavior\. Check the shift operator '<<'\. The right operand \('i' \= \[0\.\.63\]\) is greater than or equal to the length in bits of the promoted left operand\. bitarray\.cpp 85
* [V610](https://pvs-studio.com/en/docs/warnings/v610/) \[CERT\-INT34\-C\] Undefined behavior\. Check the shift operator '<<'\. The right operand \('i' \= \[0\.\.63\]\) is greater than or equal to the length in bits of the promoted left operand\. bitarray\.cpp 93

### Warning N17

[V751](https://pvs-studio.com/en/docs/warnings/v751/) \[CERT\-MSC13\-C\] Parameter 'rayTo' is not used inside function body\. btSoftBody\.cpp 2148

```cpp
btScalar btSoftBody::RayFromToCaster::rayFromToTriangle(
  const btVector3& rayFrom,
  const btVector3& rayTo,
  const btVector3& rayNormalizedDirection,
  const btVector3& a,
  const btVector3& b,
  const btVector3& c,
  btScalar maxt)
{
  static const btScalar ceps = -SIMD_EPSILON * 10;
  static const btScalar teps = SIMD_EPSILON * 10;

  const btVector3 n = btCross(b - a, c - a);
  const btScalar d = btDot(a, n);
  const btScalar den = btDot(rayNormalizedDirection, n);
  if (!btFuzzyZero(den))
  {
    const btScalar num = btDot(rayFrom, n) - d;
    const btScalar t = -num / den;
    if ((t > teps) && (t < maxt))
    {
      const btVector3 hit = rayFrom + rayNormalizedDirection * t;
      if ((btDot(n, btCross(a - hit, b - hit)) > ceps) &&
          (btDot(n, btCross(b - hit, c - hit)) > ceps) &&
          (btDot(n, btCross(c - hit, a - hit)) > ceps))
      {
        return (t);
      }
    }
  }
  return (-1);
}
```

Here the analyzer spots the formal _rayTo_ parameter that is not used in the function's body\. But the _rayFrom_ parameter is used several times\. This looks like an error in coding or refactoring\.

## Conclusion

So, the analyzer found various kinds of errors in the project, including traditional typos, memory management errors and logical errors\. We hope that this article will help the developers of Overgrowth fix some defects\. It would be even greater if they could double\-check their code base with PVS\-Studio\. The analyzer can help developers create new bug\-free builds of this wonderful game and make the Overgrowth community happier :\)