We haven't used PVS-Studio to check games for a long time. So, this time we decided to return to this practice and picked out the MTA project.
Multi Theft Auto (MTA) is a multiplayer modification for PC versions of the Grand Theft Auto: San Andreas game by Rockstar North that adds online multiplayer functionality. As Wikipedia tells us, the specific feature of the game is "well optimized code with fewest bugs possible". OK, let's ask our analyzer for its opinion.
Introduction
This time I decided to omit the texts of diagnostic messages generated by PVS-Studio for every particular defect. I comment upon examples anyway, so if you want to find out in which particular line and by which diagnostic rule a certain bug was found, see the file
mtasa-review.txt.
When looking through the project, I noted in the mtasa-review.txt file those code fragments which I found suspicious and used it to prepare the article.
Important! I added only those code fragments which I personally didn't like. I'm not a MTA developer, so I'm not familiar with its logic and principles. That's why I
must have made a few mistakes attacking correct code fragments and missing genuine bugs. Also, when studying certain fragments, I felt lazy indeed to describe some slightly incorrect
printf() function calls. So, I'm asking MTA Team developers not to rely on this article and consider checking the project by themselves. It is pretty large, so the demo version of PVS-Studio won't be enough. However, we
support free open-source projects. If you're an open source developer, contact us and we'll discuss the question of giving you a free registration key.
So, Multi Theft Auto is an open-source project in C/C++:
Analysis was performed by the PVS-Studio 5.05 analyzer:
Now let's see what bugs PVS-Studio has managed to find in the game. They aren't numerous, and most of them are found in rarely-used parts of the program (error handlers). It's no wonder: most bugs are found and fixed through other, more expensive and slow, methods. To use static analysis properly is to use it regularly. By the way, PVS-Studio can be called to analyze recently modified and compiled files only (see
incremental analysis mode). This mechanism allows the developer to find and fix many bugs and misprints immediately, which makes it much faster and cheaper than detecting errors through testing. This subject was discussed in detail in the article "
Leo Tolstoy and static code analysis". It's a worthy article, and I do recommend reading the introduction to understand the ideology of using PVS-Studio and other static analysis tools.
Strange Colors
// c3dmarkersa.cpp
SColor C3DMarkerSA::GetColor()
{
DEBUG_TRACE("RGBA C3DMarkerSA::GetColor()");
// From ABGR
unsigned long ulABGR = this->GetInterface()->rwColour;
SColor color;
color.A = ( ulABGR >> 24 ) && 0xff;
color.B = ( ulABGR >> 16 ) && 0xff;
color.G = ( ulABGR >> 8 ) && 0xff;
color.R = ulABGR && 0xff;
return color;
}
By mistake '&&' is used instead of '&'. The color is torn into bits and pieces to leave only 0 or 1.
The same problem is found in the file "ccheckpointsa.cpp".
One more problem with colors.
// cchatechopacket.h
class CChatEchoPacket : public CPacket
{
....
inline void SetColor( unsigned char ucRed,
unsigned char ucGreen,
unsigned char ucBlue )
{ m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucRed = ucRed; };
....
}
Red is copied twice, while blue is not copied at all. The fixed code should look like this:
{ m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucBlue = ucBlue; };
The same problem is found in the file cdebugechopacket.h.
By the way, quite a number of bugs of the game are duplicated in two files which, I suspect, refer to the client-side and the server-side correspondingly. Do you feel the great power of the Copy-Paste technology? :).
Something Wrong with utf8
// utf8.h
int
utf8_wctomb (unsigned char *dest, wchar_t wc, int dest_size)
{
if (!dest)
return 0;
int count;
if (wc < 0x80)
count = 1;
else if (wc < 0x800)
count = 2;
else if (wc < 0x10000)
count = 3;
else if (wc < 0x200000)
count = 4;
else if (wc < 0x4000000)
count = 5;
else if (wc <= 0x7fffffff)
count = 6;
else
return RET_ILSEQ;
....
}
The size of the
wchar_t type in Windows is 2 bytes. Its value range is [0..65535], which means that comparing it to values 0x10000, 0x200000, 0x4000000, 0x7fffffff is pointless. I guess the code should be written in some different way.
Missing break
// cpackethandler.cpp
void CPacketHandler::Packet_ServerDisconnected (....)
{
....
case ePlayerDisconnectType::BANNED_IP:
strReason = _("Disconnected: You are banned.\nReason: %s");
strErrorCode = _E("CD33");
bitStream.ReadString ( strDuration );
case ePlayerDisconnectType::BANNED_ACCOUNT:
strReason = _("Disconnected: Account is banned.\nReason: %s");
strErrorCode = _E("CD34");
break;
....
}
The
break operator is missing in this code. It results in processing the situation
BANNED_IP in the same way as
BANNED_ACCOUNT.
Strange Checks
// cvehicleupgrades.cpp
bool CVehicleUpgrades::IsUpgradeCompatible (
unsigned short usUpgrade )
{
....
case 402: return ( us == 1009 || us == 1009 || us == 1010 );
....
}
The variable is compared twice to the number 1009. A bit ahead in the code there is a similar double comparison.
Another strange comparison:
// cclientplayervoice.h
bool IsTempoChanged(void)
{
return m_fSampleRate != 0.0f ||
m_fSampleRate != 0.0f ||
m_fTempo != 0.0f;
}
This error was also copied into the cclientsound.h file.
Null Pointer Dereferencing
// cgame.cpp
void CGame::Packet_PlayerJoinData(CPlayerJoinDataPacket& Packet)
{
....
// Add the player
CPlayer* pPlayer = m_pPlayerManager->Create (....);
if ( pPlayer )
{
....
}
else
{
// Tell the console
CLogger::LogPrintf(
"CONNECT: %s failed to connect "
"(Player Element Could not be created.)\n",
pPlayer->GetSourceIP() );
}
....
}
If the object
player can't be created, the program will attempt printing the corresponding error message into the console. It will fail because it's a bad idea to use a
null pointer when calling the function
pPlayer->GetSourceIP().
Another
null pointer is dereferenced in the following fragment:
// clientcommands.cpp
void COMMAND_MessageTarget ( const char* szCmdLine )
{
if ( !(szCmdLine || szCmdLine[0]) )
return;
....
}
If the
szCmdLine pointer is
null, it will be dereferenced.
The fixed code must look like this, I suppose:
if ( !(szCmdLine && szCmdLine[0]) )
The following code fragment I like most of all:
// cdirect3ddata.cpp
void CDirect3DData::GetTransform (....)
{
switch ( dwRequestedMatrix )
{
case D3DTS_VIEW:
memcpy (pMatrixOut, &m_mViewMatrix, sizeof(D3DMATRIX));
break;
case D3DTS_PROJECTION:
memcpy (pMatrixOut, &m_mProjMatrix, sizeof(D3DMATRIX));
break;
case D3DTS_WORLD:
memcpy (pMatrixOut, &m_mWorldMatrix, sizeof(D3DMATRIX));
break;
default:
// Zero out the structure for the user.
memcpy (pMatrixOut, 0, sizeof ( D3DMATRIX ) );
break;
}
....
}
Very nice Copy-Paste. The function
memset() must be called instead of the last
memcpy() function.
Uncleared Arrays
There are a number of errors related to uncleared arrays. They all can be arranged into two categories. The first includes unremoved items, the second includes partial array clearing errors.
Unremoved Items
// cperfstat.functiontiming.cpp
std::map < SString, SFunctionTimingInfo > m_TimingMap;
void CPerfStatFunctionTimingImpl::DoPulse ( void )
{
....
// Do nothing if not active
if ( !m_bIsActive )
{
m_TimingMap.empty ();
return;
}
....
}
The function
empty() only checks whether or not the container contains items. To remove items from the
m_TimingMap container one should call the
clear() function.
Another example:
// cclientcolsphere.cpp
void CreateSphereFaces (
std::vector < SFace >& faceList, int iIterations )
{
int numFaces = (int)( pow ( 4.0, iIterations ) * 8 );
faceList.empty ();
faceList.reserve ( numFaces );
....
}
Some more similar bugs are found in the file cresource.cpp.
If you have started reading the article from the middle and therefore skipped the beginning, see the file mtasa-review.txt to find out exact locations of all the bugs.
Partial Array Clearing Errors
// crashhandler.cpp
LPCTSTR __stdcall GetFaultReason(EXCEPTION_POINTERS * pExPtrs)
{
....
PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ;
FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ;
....
}
Everything looks alright at the first sight. But
FillMemory() will in fact have no effect.
FillMemory() and
memset() are different functions. Have a look at this fragment:
#define RtlFillMemory(Destination,Length,Fill) \
memset((Destination),(Fill),(Length))
#define FillMemory RtlFillMemory
The second and the third arguments are swapped. That's why the correct code should look like this:
FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ;
The same thing is found in the file ccrashhandlerapi.cpp.
And here is the last error sample of this type. Only one byte gets cleared.
// hash.hpp
unsigned char m_buffer[64];
void CMD5Hasher::Finalize ( void )
{
....
// Zeroize sensitive information
memset ( m_buffer, 0, sizeof (*m_buffer) );
....
}
Asterisk '*' should be removed:
sizeof (m_buffer).
Uninitialized Variable
// ceguiwindow.cpp
Vector2 Window::windowToScreen(const UVector2& vec) const
{
Vector2 base = d_parent ?
d_parent->windowToScreen(base) + getAbsolutePosition() :
getAbsolutePosition();
....
}
The variable
base initializes itself. Another bug of this kind can be found a few lines ahead.
Array Index out of Bounds
// cjoystickmanager.cpp
struct
{
bool bEnabled;
long lMax;
long lMin;
DWORD dwType;
} axis[7];
bool CJoystickManager::IsXInputDeviceAttached ( void )
{
....
m_DevInfo.axis[6].bEnabled = 0;
m_DevInfo.axis[7].bEnabled = 0;
....
}
The last line
m_DevInfo.axis[7].bEnabled = 0; is not needed.
Another error of this kind
// cwatermanagersa.cpp
class CWaterPolySAInterface
{
public:
WORD m_wVertexIDs[3];
};
CWaterPoly* CWaterManagerSA::CreateQuad ( const CVector& vecBL, const CVector& vecBR, const CVector& vecTL, const CVector& vecTR, bool bShallow )
{
....
pInterface->m_wVertexIDs [ 0 ] = pV1->GetID ();
pInterface->m_wVertexIDs [ 1 ] = pV2->GetID ();
pInterface->m_wVertexIDs [ 2 ] = pV3->GetID ();
pInterface->m_wVertexIDs [ 3 ] = pV4->GetID ();
....
}
One more:
// cmainmenu.cpp
#define CORE_MTA_NEWS_ITEMS 3
CGUILabel* m_pNewsItemLabels[CORE_MTA_NEWS_ITEMS];
CGUILabel* m_pNewsItemShadowLabels[CORE_MTA_NEWS_ITEMS];
void CMainMenu::SetNewsHeadline (....)
{
....
for ( char i=0; i <= CORE_MTA_NEWS_ITEMS; i++ )
{
m_pNewsItemLabels[ i ]->SetFont ( szFontName );
m_pNewsItemShadowLabels[ i ]->SetFont ( szFontName );
....
}
....
}
At least one more error of this kind can be found in the file cpoolssa.cpp. But I decided not to describe it in the article because that would be a pretty large sample and I didn't know how to make it brief and clear. As I've already said, this and all the rest of the bugs can be found in the detailed report.
The Word 'throw' is Missing
// fallistheader.cpp
ListHeaderSegment*
FalagardListHeader::createNewSegment(const String& name) const
{
if (d_segmentWidgetType.empty())
{
InvalidRequestException(
"FalagardListHeader::createNewSegment - "
"Segment widget type has not been set!");
}
return ....;
}
The correct line is throw
InvalidRequestException(....).
Another code fragment.
// ceguistring.cpp
bool String::grow(size_type new_size)
{
// check for too big
if (max_size() <= new_size)
std::length_error(
"Resulting CEGUI::String would be too big");
....
}
The correct code should look like this:
throw std::length_error(....).
Oops: free(new T[n])
// cresourcechecker.cpp
int CResourceChecker::ReplaceFilesInZIP(....)
{
....
// Load file into a buffer
buf = new char[ ulLength ];
if ( fread ( buf, 1, ulLength, pFile ) != ulLength )
{
free( buf );
buf = NULL;
}
....
}
The
new operator is used to allocate memory, while the function
free() is used to release it. The result is unpredictable.
Always True/False Conditions
// cproxydirect3ddevice9.cpp
#define D3DCLEAR_ZBUFFER 0x00000002l
HRESULT CProxyDirect3DDevice9::Clear(....)
{
if ( Flags | D3DCLEAR_ZBUFFER )
CGraphics::GetSingleton().
GetRenderItemManager()->SaveReadableDepthBuffer();
....
}
The programmer wanted to check a particular bit in the
Flag variable. By mistake he wrote the '|' operation instead of '&'. This results in the condition being always true.
A similar mess-up is found in the file cvehiclesa.cpp.
Another bug in a check is found here:
unsigned_value < 0.
// crenderitem.effectcloner.cpp
unsigned long long Get ( void );
void CEffectClonerImpl::MaybeTidyUp ( void )
{
....
if ( m_TidyupTimer.Get () < 0 )
return;
....
}
The
Get() function returns the value of the unsigned
unsigned long long type. It means that the check
m_TidyupTimer.Get () < 0 is pointless. Other errors of this type can be found in the files csettings.cpp, cmultiplayersa_1.3.cpp and cvehiclerpcs.cpp.
This Code May Work, but You'd Better Refactor It
Many PVS-Studio diagnostics detected bugs which will most likely in no way manifest themselves. I don't like describing such bugs because they are not interesting. So, here are just a couple of examples.
// cluaacldefs.cpp
int CLuaACLDefs::aclListRights ( lua_State* luaVM )
{
char szRightName [128];
....
strncat ( szRightName, (*iter)->GetRightName (), 128 );
....
}
The third argument of the
strncat() function refers, instead of the buffer size, to the number of characters you can put into the buffer. A buffer overflow can theoretically occur here, but in practice it will most probably never happen. This type of error is described in detail in the
V645 diagnostic's description.
The second example.
// cscreenshot.cpp
void CScreenShot::BeginSave (....)
{
....
HANDLE hThread = CreateThread (
NULL,
0,
(LPTHREAD_START_ROUTINE)CScreenShot::ThreadProc,
NULL,
CREATE_SUSPENDED,
NULL );
....
}
In many game fragments, the functions
CreateThread()/
ExitThread() are used. This is in most cases a bad idea. You should use the functions
_beginthreadex()/
_endthreadex() instead. For details on this issue see the
V513 diagnostic's description.
I Have to Stop Somewhere
I have described only a part of all the defects I noticed. But I have to stop here: the article is already big enough. See the file
mtasa-review.txt for other bug samples.
There you will find bugs which I haven't mentioned in the article:
- identical branches in the conditional operator if () { aa } else { aa };
- checking a pointer returned by the new operator for being a null pointer: p = new T; if (!p) { aa };
- a poor way of using #pragma to suppress compiler warnings (instead of push/pop);
- classes contain virtual functions but no virtual destructors;
- a pointer gets dereferenced first and only then checked for being a null pointer;
- identical conditions: if (X) { if (X) { aa } };
- miscellaneous.
Conclusion
The PVS-Studio analyzer can be efficiently used to eliminate various bugs at early development stages both in game projects and projects of any other type. It won't find algorithmic errors of course (it needs AI to do that), but it will help save a lot of time programmers usually waste searching for silly mistakes and misprints. Developers actually spend much more time on finding plain defects than they may think. Even debugged and tested code contains
numbers of such errors, while 10 times more of them get fixed when writing new code.
For anyone curious, 'new' throws an exception if it can't allocate the memory, unless you're using std::nothrow.