Checking the open-source game Multi Theft Auto / PVS-Studio Blog

We have not tested games with PVS-Studio for a long time. We decided to fix it and chose the MTA project. Multi Theft Auto (MTA) is a modification for the PC version of the game Grand Theft Auto: San Andreas from Rockstar North. MTA allows players from all over the world to play against each other online. As written in Wikipedia, the feature of the game is "optimized code with the least number of crashes." Well, let's see what the code analyzer says.

Introduction

This time, I decided not to include in the article the diagnostic messages that the PVS-Studio analyzer generates on suspicious lines. Anyway, I give explanations to the code snippets. If it is interesting to find out in which particular line the error was found and which diagnostic message the analyzer issued, then this can be viewed in the mtasa-review.txt file .

When I looked at the project, I wrote out code fragments that seemed suspicious to the mtasa-review.txt file. Later, based on it, I wrote this article.

Important.Only those code fragments that I personally didn’t like were in the file. I do not participate in the development of MTA and can not imagine how and what works in it. Therefore, I must have been mistaken in places: I included the correct code in the list and skipped the real errors. And somewhere in general I was too lazy and did not describe the not quite correct call to the printf () function. I ask the developers from the MTA Team not to rely on this text and independently verify the project. The project is large enough and the demo version is not enough for a full check. However, we do support free open-source projects. Write to us and we will agree on the topic of the free version of PVS-Studio.

So, the Multi Theft Auto game is an open-source project written in C / C ++:

• project site ;
• source code ;
• MTA Wiki .

For verification, I used the PVS-Studio 5.05 analyzer:

• tool page ;
• download (demo version is limited only by the number of jumps to the code with errors);
• examples of errors found .

Now let's see what the PVS-Studio analyzer was able to find in this game. There are not so many mistakes. Moreover, most of them are contained in rarely used parts of the program (error handlers). No wonder. Most errors are corrected by other, slower and more expensive methods. The proper use of static analysis is to run regularly. For example, the PVS-Studio analyzer can start for newly modified and compiled files (see incremental analysis mode ). So the developer immediately finds and fixes many errors and typos. This is many times faster and cheaper than detecting these errors when testing. This topic was discussed in more detail in the article " Leo Tolstoy and Static Code Analysis.". Good article. I recommend reading the introductory part to understand the ideology of using tools such as PVS-Studio.

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;
}

Incidentally, instead of '&', '&&' is used. As a result, horns and legs remain in the form of zero or one from the color.

An identical misfortune can be observed in the file “ccheckpointsa.cpp”.

Another trouble with color rendering.

// 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, but not blue. The corrected code should be like this:

{ m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucBlue = ucBlue; };

An identical problem exists in the cdebugechopacket.h file.

In general, a number of errors in the game are duplicated in two files. It seems to me that one of the files relates to the client part, and the second to the server part. Feels the full power of Copy-Paste :) technology.

Something is 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 on Windows is 2 bytes. The range of its values ​​is [0..65535]. This means that comparing with the numbers 0x10000, 0x200000, 0x4000000, 0x7fffffff does not make sense. Probably, the code should have been written somehow differently.

Forgotten 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' statement is forgotten in this code. As a result, the situation “BANNED_IP” is handled 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 with the number 1009. Just below, you can find another identical double comparison.

The following suspicious comparison:

// cclientplayervoice.h
bool IsTempoChanged(void)
{ 
  return m_fSampleRate != 0.0f ||
         m_fSampleRate != 0.0f ||
         m_fTempo != 0.0f;
}

The same error was copied to the cclientsound.h file.

Dereferencing a null pointer

// 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 it was not possible to create a “player” object, then the program tries to print the corresponding information to the console. But not successful. It is a bad idea to use a null pointer by calling the function "pPlayer-> GetSourceIP ()".

Another null pointer is dereferenced here:

// clientcommands.cpp
void COMMAND_MessageTarget ( const char* szCmdLine )
{
  if ( !(szCmdLine || szCmdLine[0]) )
    return;
  ....
}

If the szCmdLine pointer is zero, then it will be dereferenced.

Most likely, it should be like this:

if ( !(szCmdLine && szCmdLine[0]) )

Most of all, I liked this piece of code:

// 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;
  }
  ....
}

Beautiful Copy-Paste. Instead of the last memcpy () function, the memset () function should be called.

Raw arrays

There are a number of errors related to uncleaned arrays. Errors can be divided into two categories. First, the items are not deleted. The second - not all array elements contain zero values.

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 empty () function checks if the container contains elements or not. To remove elements from the 'm_TimingMap' container, the clear () function should be called.

The following example:

// cclientcolsphere.cpp
void CreateSphereFaces (
  std::vector < SFace >& faceList, int iIterations )
{
  int numFaces = (int)( pow ( 4.0, iIterations ) * 8 );
  faceList.empty ();
  faceList.reserve ( numFaces );
  ....
}

A few more such errors are in the cresource.cpp file.

Note. If someone missed the beginning of the article and decided to read from the middle: where this and other errors are located, you can find out in the mtasa-review.txt file .

Now about zero padding errors

// crashhandler.cpp
LPCTSTR __stdcall GetFaultReason(EXCEPTION_POINTERS * pExPtrs)
{
  ....
  PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ;
  FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ;
  ....
}

At first glance, everything is fine. That's just FillMemory () will not have any effect. FillMemory () and memset () are not the same thing. Look here:

#define RtlFillMemory(Destination,Length,Fill) \
  memset((Destination),(Fill),(Length))
#define FillMemory RtlFillMemory

The second and third arguments are reversed. For this, it will be correct like this:

FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ;

A similar confusion exists in the ccrashhandlerapi.cpp file.

And the last piece of code on this topic. Only one byte is cleared here.

// hash.hpp
unsigned char m_buffer[64];
void CMD5Hasher::Finalize ( void )
{
  ....
  // Zeroize sensitive information
  memset ( m_buffer, 0, sizeof (*m_buffer) );
  ....
}

The asterisk '*' is superfluous. It should say “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' is used to initialize itself. Another such error is a few lines below.

Going abroad an array

// 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 superfluous.

Another mistake

// 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 ();
  ....
}

And 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 );
    ....
  }
  ....
}

Another, at least one error of exceeding the boundaries of the array is in the cpoolssa.cpp file. But I did not describe it in the article. It turns out to be a long example, but I don’t know how to make it short and clear. This and other errors, as I said, can be found in a more complete report.

Missed word 'throw'

// fallistheader.cpp
ListHeaderSegment*
FalagardListHeader::createNewSegment(const String& name) const
{
  if (d_segmentWidgetType.empty())
  {
    InvalidRequestException(
      "FalagardListHeader::createNewSegment - "
      "Segment widget type has not been set!");
  }
  return ....;
}

There should have been a "throw InvalidRequestException (....)".

Another piece of code.

// 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");
  ....
}

It should be: 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;
  }
  ....
}

Memory is allocated using the 'new' operator. And they try to release it using the free () function. 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 specific bit in the Flag variable. Because of a typo, instead of the '&' operation, he used the '|' operation. As a result, the condition is always satisfied.

A similar confusion exists in the cvehiclesa.cpp file.

The following error with validation: 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 a value of the unsigned type 'unsigned long long'. So the check "m_TidyupTimer.Get () <0" does not make sense. Other errors of this variety are found in the files: csettings.cpp, cmultiplayersa_1.3.cpp, cvehiclerpcs.cpp.

It may work, but it is better to refactor

Many messages issued by PVS-Studio diagnose errors that most likely will not manifest themselves. I do not like to write about such errors. It is not interesting. I will give just a few examples.

// cluaacldefs.cpp
int CLuaACLDefs::aclListRights ( lua_State* luaVM )
{
  char szRightName [128];
  ....
  strncat ( szRightName, (*iter)->GetRightName (), 128 );
  ....
}

The third argument to strncat () does not indicate the size of the buffer, but how many more characters can be placed in it. Theoretically, a buffer overflow is possible here. In practice, most likely this will never happen. More information about this type of error can be found in the description of the V645 diagnostic .

One more example.

// cscreenshot.cpp
void CScreenShot::BeginSave (....)
{
  ....
  HANDLE hThread = CreateThread (
    NULL,
    0,
    (LPTHREAD_START_ROUTINE)CScreenShot::ThreadProc,
    NULL,
    CREATE_SUSPENDED,
    NULL );
  ....
}

Many places in the game use the CreateThread () / ExitThread () functions. As a rule, this is not entirely true. Use the _beginthreadex () / _ endthreadex () functions. You can learn more about this from the description of the V513 diagnostic .

I have to stop somewhere

I have described far from all of the shortcomings that I noticed. However, it's time to stop. The article is already quite large. Other errors can be found in the mtasa-review.txt file .

For example, there are the following types of errors that are not in the article:

• same branches in the conditional statement: if () {aa} else {aa};
• checking the pointer that the 'new' operator returns for equality to zero: p = new T; if (! p) {aa};
• bad way to use #pragma to suppress warnings (push / pop is not used);
• classes have virtual functions, but no virtual destructor;
• the pointer is dereferenced at the beginning, and only then it is checked for equality to zero;
• repeating conditions: if (X) {if (X) {aa}};
• other things.

Conclusion

The PVS-Studio analyzer can be effectively used for early elimination of many errors, both in game projects and in any others. He will not find algorithmic errors (this requires artificial intelligence). But essentially the time that is uselessly spent searching for stupid mistakes and typos. Programmers spend much more on simple defects than they think. Even in already debugged and tested code, there are many such errors . And when writing new code, such errors are corrected 10 times more.