Checking open-source games Multi Theft Auto

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 Rockstar North Grand Theft Auto: San Andreas game. 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 amount of failures”. Well, let's see what the code analyzer says.

Introduction

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

When I looked through the project, I wrote out to the mtasa-review.txt file code snippets that I found suspicious. Later, based on it, I wrote this article.
')
Important. Only those code fragments that I personally did not like got into the file. I do not participate in the development of MTA and can not imagine how and what it works. Therefore, for sure I was mistaken in some places: I included the correct code in the list and missed the real errors. And somewhere in general he was too lazy and did not describe the not quite correct call of the printf () function. I ask the developers of the MTA Team not to rely on this text and independently check the project. The project is large enough and the demo version is not enough for a full test. However, we support free open-source projects. Write to us and we will agree on 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 (the 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 could 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. Proper use of static analysis - regular launch. For example, the PVS-Studio analyzer can run for newly modified and compiled files (see incremental analysis mode ). So the developer immediately finds and fixes many errors and typos. It 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 introduction to understand the ideology of using tools like 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; } 

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

Identical trouble 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 blue is not copied. The correct code should be:

 { 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 is duplicated in two files. It seems to me that one of the files refers to the client part, and the second to the server part. One 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 in Windows is 2 bytes. The range of its values ​​is [0..65535]. This means that a comparison with the numbers 0x10000, 0x200000, 0x4000000, 0x7fffffff does not make sense. Probably the code should have been written in some other way.

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

In this code, the 'break' statement is forgotten. As a result, the “BANNED_IP” situation 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.

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 “player” object could not be created, the program tries to print the corresponding information to the console. But not well. It’s a bad idea to use a null pointer by calling the pPlayer-> GetSourceIP () function.

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 code snippet:

 // 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 associated with uncleaned arrays. Errors can be divided into two categories. The first is that the items are not deleted. The second is that not all elements of the array have zero values.

Items Not Removed

 // 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 whether the container contains elements or not. To remove items from the 'm_TimingMap' container, you should call the clear () function.

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

There are a few more such errors in the cresource.cpp file.

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

Now for the errors of filling with zeros

 // 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 have no effect. FillMemory () and memset () are not the same. Look here:

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

The second and third arguments are reversed. Therefore, it would be correct to:

 FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ; 

Similar confusion exists in the file ccrashhandlerapi.cpp.

And the last piece of code on the subject. 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 redundant. It should be written "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.

Overrun 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 another one:

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

Also, at least one error of going out of the array is in the file cpoolssa.cpp. But I did not describe it in the article. It turns out a long example, and how to make it short and clear, I do not know. This and other errors, as I said before, can be viewed in a more comprehensive report.

Missing the 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 code snippet.

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

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 are trying to free her with the help of 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 particular 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.

There is a similar confusion in the cvehiclesa.cpp file.

The following check error: 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 an unsigned long unsigned type. This means that the m_TidyupTimer.Get () <0 check does not make sense. Other errors of this variety are found in the files: csettings.cpp, cmultiplayersa_1.3.cpp, cvehiclerpcs.cpp.

This may work, but refactoring is best.

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 only a few examples.

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

The third argument to the strncat () function does not indicate the size of the buffer, but how many more characters can be placed into it. Theoretically, a buffer overflow is possible here. In practice, most likely this will never happen. You can read more about this type of error 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. The _beginthreadex () / _ endthreadex () functions should be used. You can learn more about this in the description of the V513 diagnostic.

Must stop somewhere

I have described far from all of the shortcomings that I have noticed. However, it is time to stop. The article is already quite large. With other errors, you can find in the file mtasa-review.txt .

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

• identical branches in the conditional operator: if () {aa} else {aa};
• checking the pointer, which returns the operator 'new', 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;
• recurring conditions: if (x) {if (x) {aa}};
• other

Conclusion

The PVS-Studio analyzer can effectively be used for early elimination of many errors, both in game projects, and in any other. Algorithmic errors, he will not find (for this we need artificial intelligence). But time is of the essence, which is wasted on finding silly mistakes and typos. Programmers spend on simple defects much more than they think. Even in the already debugged and tested code, there are many such errors . And when writing a new code, such errors are corrected 10 times more.