# Reviewing Defects in the Source Code of Video Game Vangers: One for the Road 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\. ![0578_Vangers/image1.png](https://import.viva64.com/docx/blog/0578_Vangers/image1.png) ## Introduction _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](https://store.steampowered.com/app/264080/Vangers/) and can run on modern operating systems, although only in the resolution 800x600 as for now\. The defects were found using [PVS\-Studio](https://pvs-studio.com/en/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\. ## Potential memory leaks Here comes the first snippet: ```cpp 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](https://en.wikipedia.org/wiki/Smart_pointer)\. ## Same\-value reassignments Moving on to the next snippet\. ```cpp 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: ```cpp 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: ```cpp 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\. * V570 The same value is assigned twice to the 'ExternalSensor' variable\. mechos\.cpp 13967 ## Careless copy\-paste I found this example quite amusing: ```cpp 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: ```cpp //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\. ## Bugs in switch statement 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: ```cpp 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: ```cpp 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\! ## Unreachable code Now try to quickly find the bug here: ```cpp 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_\. ## Overcomplicated check Sometimes certain logic expressions can be simplified, as in the following code: ```cpp 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: ```cpp if(bool(obj -> flags & OBJ_LOCKED) == bool(p -> flags & EV_IF_LOCKED)) ``` ## Potential null pointer dereferencing Here's one example: ```cpp 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_: ```cpp 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](https://pvs-studio.com/en/blog/posts/cpp/0558/)"\. The analyzer found a few more errors of this type: * V522 CWE\-690 There might be dereferencing of a potential null pointer 'item'\. ascr\_fnc\.cpp 5356 * V522 CWE\-690 There might be dereferencing of a potential null pointer\. A constructor of the string class expects a valid pointer\. ivmap\.cpp 309 * V522 CWE\-690 There might be dereferencing of a potential null pointer 'idxName'\. Check lines: 94, 92\. zip\_resource\.cpp 94 * V575 CWE\-628 The potential null pointer is passed into 'strlen' function\. Inspect the first argument\. Check lines: 2156, 2155\. road\.cpp 2156 * V575 CWE\-628 The potential null pointer is passed into 'strlen' function\. Inspect the first argument\. Check lines: 810, 809\. vmap\.cpp 810 * V575 CWE\-628 The potential null pointer is passed into 'strlen' function\. Inspect the first argument\. Check lines: 813, 812\. vmap\.cpp 813 ## Performance drop and refactoring leftovers Another error found by the analyzer: ```cpp 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\. ## Improper dynamic memory deallocation Finally, the last snippet: ```cpp 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:** * V611 CWE\-762 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 \[\] p;'\. ascr\_fnc\.cpp 4401 * V611 CWE\-762 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 \[\] p1;'\. ascr\_fnc\.cpp 4402 The total number of V611 warnings triggered by the project is pretty big \- about [twenty five](http://cppfiles.com/Vangers_V611.txt)\. 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\. ## Conclusion 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](https://pvs-studio.com/en/pvs-studio/download/) and try PVS\-Studio with your own project\.