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!
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 version to check.
Let's discuss the most interesting warnings that PVS-Studio found when analyzing the project.
Well, let's start with a function that triggered the analyzer twice — PVS-Studio issued two warnings on adjacent code lines.
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 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 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 to the corresponding item.
And that's how we can fix this code fragment:
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:
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 ....;
}V772 [CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. OVR_CAPI_Util.cpp 380
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. 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:
.... 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.
V595 [CERT-EXP12-C] The 'ctx' pointer was utilized before it was verified against nullptr. Check lines: 130, 131. ascontext.cpp 130
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:
ASContext::ASContext(....)
{
ctx = ....;
if( !ctx )
{
FatalError("Error","Failed to create the context.");
return;
}
ctx->SetUserData(this, 0);
....
}V547 Expression 'connect_id_ == - 1' is always true. placeholderobject.cpp 342
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:
ObjectSanityState PlaceholderObject::GetSanity()
{
....
if( .... && connect_id_ == -1 )
{
....
}
....
}V791 The initial value of the index in the nested loop equals 'i'. Perhaps, 'i + 1' should be used instead. navmeshhintobject.cpp 65
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:
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]);
}
}
}
....
}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
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.
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!
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
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 [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
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...
V523 The 'then' statement is equivalent to the 'else' statement. skeleton.cpp 152
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.
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
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:
void asCCompiler::CompileIfStatement(....)
{
bool constructorCall1 = ....;
bool constructorCall2 = ....;
....
if (constructorCall1 != constructorCall2)
{
....
}
}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
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:
V751 [CERT-MSC13-C] Parameter 'rayTo' is not used inside function body. btSoftBody.cpp 2148
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.
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 :)