The Vangers: One for the Road video game just recently turned 20. To celebrate this event, we decided to check the source code of the project and make a review of interesting bugs found. This task was assigned to our new team member George. Checking a project is a good way to explore PVS-Studio's functionality and develop one's article-writing skill.
Vangers: One For The Road is a video game written in C++, developed by K-D LAB and released in 1998. An updated re-release is available on Steam and can run on modern operating systems, although only in the resolution 800x600 as for now.
The defects were found using PVS-Studio, a static code analyzer for programs in C, C++, and C#. What follows is a collection of faulty-code snippets accompanied by my comments. I recommend that you try finding the bug on your own first and only then read on for an explanation - it'll be more fun that way.
Here comes the first snippet:
void iInitText(....)
{
char* buf;
buf = new char[text_len];
memcpy(buf,text,text_len);
....
i = 0;
while(i < text_len){
while(!buf[i]) i ++;
if(i < text_len){
....
while(buf[i]) i ++;
}
}
}PVS-Studio diagnostic message: V773 CWE-401 Visibility scope of the 'buf' pointer was exited without releasing the memory. A memory leak is possible. iscr_fnc.cpp 1174
This function processes strings consisting of several words. The string being processed is stored using the buf pointer to a char array allocated by the new[] operator. The pointer is in the scope of the iInitText function.
When the function terminates, buf will go out of its scope and cease to exist, making the memory block it pointed to no longer available. Errors like that are called memory leaks: they result in uncontrolled reduction of the amount of available RAM (or virtual memory).
To avoid such errors, one should free memory when it's no longer needed. In this case, the last closing brace should be preceded by the "delete [] buf" expression. An even better solution is to use smart pointers.
Moving on to the next snippet.
void VangerUnit::CreateVangerUnit(void)
{
....
DoorFlag = 0;
ExternalMode = EXTERNAL_MODE_NORMAL;
ExternalTime = 0;
ExternalLock = 0;
ExternalDraw = 1;
ExternalObject = ExternalSensor = ExternalSensor = NULL;
ExternalTime2 = 0;
ExternalAngle = 0;
Go2World();
....
};PVS-Studio diagnostic message: V570 The same value is assigned twice to the 'ExternalSensor' variable. mechos.cpp 5828
Assigning the same value to one variable twice doesn't look good. Let's look into the VangerUnit structure:
struct VangerUnit : TrackUnit , uvsUnitType , aiFactorType
{
....
int ExternalMode, ExternalTime, ExternalLock,
ExternalDraw, ExternalAngle;
int ExternalTime2;
SensorDataType* ExternalObject;
SensorDataType* ExternalLastSensor;
SensorDataType* ExternalSensor;
int RandomUpdate;
....
};Now that we know about the similar names and identical types of the variables ExternalObject, ExternalLastSensor, and ExternalSensor, we can infer that the code was initially meant to look like this:
void VangerUnit::CreateVangerUnit(void)
{
....
DoorFlag = 0;
ExternalMode = EXTERNAL_MODE_NORMAL;
ExternalTime = 0;
ExternalLock = 0;
ExternalDraw = 1;
ExternalObject = ExternalLastSensor = ExternalSensor = NULL;
ExternalTime2 = 0;
ExternalAngle = 0;
Go2World();
....};
What's bad about this error? Well, it's that the ExternalLastSensor pointer is left uninitialized, potentially leading to a run-time error. Using such a pointer means attempting to access a non-existent object in a random memory location, with unknown results. Such bugs are not always easy to catch. By the way, if you scroll 8000 lines down, you'll find an exact copy of this code - that's a product of the copy-paste technique.
I found this example quite amusing:
const char* iGetJoyBtnNameText(int vkey,int lang)
{
const char* ret;
if(vkey & VK_BUTTON){
if(vkey >= VK_BUTTON_1 && vkey <= VK_BUTTON_32){
ret = (lang)
? iJoystickButtons2[vkey - VK_BUTTON_1]
: iJoystickButtons1[vkey - VK_BUTTON_1];
return ret;
}
else
return NULL; //WARNING NEED VIEW!!!
}
if(vkey & VK_STICK_SWITCH){
if(vkey >= VK_STICK_SWITCH_1 && vkey <= VK_STICK_SWITCH_9){
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1];
return ret;
}
else
return NULL; //WARNING NEED VIEW!!!
}
return NULL; //WARNING NEED VIEW!!!
}You surely noted the programmer's comments, just as I did. I was wondering where the NULL would go if returned by the iGetJoyBtnNameText function, so I tracked it down and found only two calls. Here's what they look like:
//NEED Full Rewrite
/*if(!(key & iJOYSTICK_MASK)){
str = iGetKeyNameText(key,iRussian);
}
else {
str = iGetJoyBtnNameText(key,iRussian);
}*/
//NEED rewrite
/*if(!(k & iJOYSTICK_MASK))
key_name = iGetKeyNameText(k,iRussian);
else
key_name = iGetJoyBtnNameText(k,iRussian);
*/It seems we're looking at incomplete code, which is yet to be finished, and I happened to peek into it just in the midst of construction. It evoked a vivid picture of a "CONSTRUCTION SITE" sign, with all the noise and dust and a huge excavator doing some ground digging. Unnoticed in this great tumult, a bug has slipped into the code of the function pointed out by PVS-Studio:
V583 CWE-783 The '?:' operator, regardless of its conditional expression, always returns one and the same value. iextern.cpp 2461
The bug is in the second '?:' operator. It's a classic copy-paste bug.
Note that I reformatted the code somewhat when writing the article to make this bug easier to spot. Originally, though, the whole expression with the ternary operator was written in one line.
Here's another example. Going through the report, I came across a bug in a terribly lengthy switch statement. To make things easier for you, here's an abridged version:
int uvsgetDGdata(int code){
switch( code ){
....
// about 230 lines of case
....
case DG_EXTERNS::HERE_PALOCHKA:
return
(uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PALOCHKA, uvsTreasureInShop)
||
uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PALOCHKA, 0));
break;
case DG_EXTERNS::HERE_NOBOOL:
return
(uvsReturnTreasureStatus
(UVS_ITEM_TYPE::NOBOOL, uvsTreasureInShop)
||
uvsReturnTreasureStatus
(UVS_ITEM_TYPE::NOBOOL, 0));
break;
case DG_EXTERNS::HERE_PIPKA:
return
(uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)
||
uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop));
break;
....
// 40 more lines
....
}// end switch
return 0;
}Found it? If you're thinking about PIPKA, then you're on the right track.
PVS-Studio diagnostic message: V501 CWE-570 There are identical sub-expressions 'uvsReturnTreasureStatus(UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)' to the left and to the right of the '||' operator. univang.cpp 10230
Good ol' copy-paste once again. The case block checking the constant expression DG_EXTERNS::HERE_PIPKA has the same expression as both operands of the '||' operator. The fixed version should obviously look like this:
case DG_EXTERNS::HERE_PIPKA:
return
(uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)
||
uvsReturnTreasureStatus
(UVS_ITEM_TYPE::PIPKA, 0));Sadly, this particular typo is almost impossible to find through a code review because the switch statement takes up over 300 lines, and all the case blocks are very similar in structure. Trying to manually find a bug here is indeed like looking for a needle in a haystack!
Now try to quickly find the bug here:
void uvsVanger::break_harvest(void){
....
pg = Pworld -> escT[0] -> Pbunch
-> cycleTable[Pworld -> escT[0] -> Pbunch -> currentStage].Pgame;
if (!pg) {
return;
ErrH.Abort("uvsVanger::break_harvest : don't know where to go ");
}
....
}PVS-Studio diagnostic message: V779 CWE-561 Unreachable code detected. It is possible that an error is present. univang.cpp 4441
The return statement is followed by the ErrH.Abort() method: if the pq pointer happens to be null, the function will terminate and fail to handle the error. To fix that, we should swap Err.Abort() and return.
Sometimes certain logic expressions can be simplified, as in the following code:
void iScreen::CheckScanCode(int sc)
{
....
iScreenObject* obj;
iScreenEvent* p;
....
obj = (iScreenObject*)objList -> last;
while(obj){
....
while(p){
if(
(!(obj -> flags & OBJ_LOCKED) && !(p -> flags & EV_IF_LOCKED))
||
((obj -> flags & OBJ_LOCKED) && (p -> flags & EV_IF_LOCKED))){
....
}
}
....
}
....
}PVS-Studio diagnostic message: V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. iscreen.cpp 2221
The analyzer warns us that the if statement's conditional expression could be simplified and suggests a better version. Indeed, the original check is functionally equivalent to the following expression:
if(bool(obj -> flags & OBJ_LOCKED) == bool(p -> flags & EV_IF_LOCKED))Here's one example:
void XZIP_FileHeader::SetName(char* p)
{
int i,sz = strlen(p);
fileName = strdup(p);
for(i = 0; i < sz; i ++)
if(fileName[i] == '/') fileName[i] = '\\';
}PVS-Studio diagnostic message: V522 CWE-690 There might be dereferencing of a potential null pointer 'fileName'. Check lines: 72, 70. zip_resource.cpp 72
This code lacks a check of the fileName pointer. The strdup() function creates a copy of a C-style string on the heap and returns a pointer to it. If it fails to allocate memory, it will return NULL instead. Thus, if strdup(p) fails to allocate memory, the program will end up attempting to dereference a null pointer two lines later, resulting in undefined behavior, which is a critical error.
There's also another pretty similar bug in the code of Vangers:
char* iGetMergedName(char* name,char* path)
{
....
return strdup(out.c_str());
}
void ivrtMap::fileLoad(void)
{
....
XBuffer buf;
buf < fileName < (isCompressed ? ".vmc" : ".vmp");
std::string sbuf=strdup(iGetMergedName(buf.GetBuf(),iniName));
std::string sbuf2;
....
}If the iGetMergedName() function returns NULL, it will be passed to the strdup() function. Just like in the previous example, it will result in null pointer dereferencing and undefined behavior.
So what's the way out? Well, the answer is simple: always check the pointer returned by such functions as malloc(), calloc(), strdup(), and the like. And if it's found to be a null pointer, you'll have to handle it as an error, for example, by throwing an exception. If this recommendation doesn't seem convincing enough, then check out the article "Why it is important to check what the malloc function returned".
The analyzer found a few more errors of this type:
Another error found by the analyzer:
int dastPoly3D::quant_make_sign(void){
....
for( int s = 0; s < dastResSign->once[n]; s++){
....
switch (dastResSign -> type[n][count]){
....
case DAST_SIGN_TYPE::DOWN:
{
uchar *data = new uchar[16];
if ( dastResSign -> type[n][count] == DAST_SIGN_TYPE::DOWN )
memset(data, 251, 16);
else
memset(data, 2, 16);
....
}
delete[] data;
break;
}
....
}
....
}
return (count != dastResSign->poly[n]);
}PVS-Studio diagnostic message: V819 Decreased performance. Memory is allocated and deleted multiple times inside the body of the loop. Consider moving memory allocation outside of the loop. poly3d.cpp 161
Here we are dealing with reduced performance. Dynamic memory allocation and release statements are placed within a loop and, therefore, execute at each iteration. It's better to take such functions out of the loop to save precious computational resources. This is especially crucial with video games. It looks like the uchar *data buffer and all the function calls related to it are refactoring leftovers. The array is allocated, filled with values, and destroyed - that's all; it doesn't go anywhere and just 'pops up' there at each iteration. The developers should revise this function's code and remove all the unnecessary lines to make it faster. This will also prevent it from triggering the warning.
Finally, the last snippet:
void aciPackFile(char* fname)
{
int sz,sz1;
char* p,*p1;
....
p = new char[sz];
p1 = new char[sz1];
....
delete p;
delete p1;
}PVS-Studio diagnostic messages:
The total number of V611 warnings triggered by the project is pretty big - about twenty five. The reported defect has to do with incorrect use of the memory release operator: arrays must be deleted using the delete[] operator rather than the scalar delete.
So what happens when such an error occurs? The answer is we get undefined behavior. With some implementations, you may be lucky to have your code working without errors, but it's still faulty.
Consider this scenario: rather than freeing the memory blocks allocated for the arrays p and p1, the program deletes only their first elements, which are also pointers to those arrays. The rest of that memory will be left unreleased, and you'll no longer be able to use it.
However, the problem is much trickier, and there may be other outcomes as well.
The new[] operator is usually implemented in such a way that the beginning of the memory block allocated for the array also stores additional information such as the size of the block and the number of array elements. When calling delete (without brackets) for an array allocated using new[], it will almost surely fail to interpret that additional information correctly and will end up with undefined behavior.
Another possible scenario is that memory for arrays and single elements is allocated from different memory pools. In that case, attempting to return memory allocated for arrays back to the pool meant for scalars will result in a crash.
Remember that the compiler doesn't warn you about attempts to delete an array as a scalar since it doesn't distinguish between pointers to arrays and pointers to single elements. It means you have to make sure that your memory allocation and release operators match; it's something that the developer is responsible for. If you allocate memory using the new operator, then you must free it using the delete operator; and if you use new[] for allocation, then you must use delete[] for deallocation. Otherwise, you risk ending up with just any random sort of problem depending on the implementation, such as memory corruption or a crash - each of them is very tedious to debug.
I hope the developers of Vangers will find this review helpful and fix a few bugs here and there. As to the total number of bugs, I'd say there aren't many of them, which means high code quality.
Welcome to download and try PVS-Studio with your own project.