# Проверка open\-source игры Multi Theft Auto Мы давно не проверяли игры с помощью PVS\-Studio\. Решили это исправить и выбрали проект MTA\. Multi Theft Auto \(MTA\) является модификацией для PC версий игры Grand Theft Auto: San Andreas от Rockstar North\. MTA позволяет игрокам со всего мира играть друг против друга в режиме онлайн\. Как написано в Wikipedia, особенностью игры является "оптимизированный код с наименьшим количеством сбоев"\. Что же, давайте посмотрим, что скажет анализатор кода\. ![0207_Multi_Theft_Auto_ru/image1.png](https://import.viva64.com/docx/blog/0207_Multi_Theft_Auto_ru/image1.png) ## Введение ![0207_Multi_Theft_Auto_ru/image2.png](https://import.viva64.com/docx/blog/0207_Multi_Theft_Auto_ru/image2.png) Рисунок 1\. Логотип игры Multi Theft Auto В этот раз, я решил не включать в статью диагностические сообщения, которые анализатор PVS\-Studio выдаёт на подозрительные строки\. Все равно я даю пояснения к фрагментам кода\. Если интересно узнать, в какой именно строке найдена ошибка и какое диагностическое сообщение выдал анализатор, то это можно посмотреть в файле [mtasa\-review\.txt](https://pvs-studio.com/external-pictures/txt/mtasa-review.txt)\. Когда я просматривал проект, я выписывал в файл mtasa\-review\.txt фрагменты кода, которые мне показались подозрительными\. Позже, основываясь на нём, я написал эту статью\. **Важно\.** В файл попали только те фрагменты код, которые не понравились лично мне\. Я не участвую в разработке MTA и не представляю, как и что в ней работает\. Поэтому, наверняка я местами ошибся: включил в список корректный код и пропустил настоящие ошибки\. А где\-то вообще поленился и не стал описывать не совсем правильный вызов функции printf\(\)\. Прошу разработчиков из MTA Team не полагаться на этот текст и самостоятельно проверить проект\. Проект достаточно большой и демонстрационной версии не хватит для полноценной проверки\. Однако мы [поддерживаем](https://pvs-studio.ru/ru/blog/posts/0193/) бесплатные open\-source проекты\. Напишите нам, и мы договоримся на тему бесплатной версии PVS\-Studio\. Итак, игра Multi Theft Auto является open\-source проектом, написанным на языке Си/Си\+\+: * [сайт проекта](http://mtasa.com/); * [исходный код](https://code.google.com/archive/p/mtasa-blue/source); * [MTA Wiki](http://wiki.multitheftauto.com/wiki/Main_Page)\. Для проверки я использовал анализатор PVS\-Studio 5\.05: * [страница инструмента](https://pvs-studio.ru/ru/pvs-studio/); * [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) \(демонстрационная версия ограничена только количеством переходов к коду с ошибками\); * [примеры найденных ошибок](https://pvs-studio.ru/ru/blog/examples/)\. Посмотрим теперь, что смог найти анализатор PVS\-Studio в этой игре\. Ошибок не так уж и много\. Причём большинство из них содержится в редко используемых частях программы \(обработчики ошибок\)\. Это не удивительно\. Большинство ошибок исправляется другими, более медленными и дорогими методами\. Правильное использование статического анализа \- регулярный запуск\. Например, анализатор PVS\-Studio умеет запускаться для только что изменённых и скомпилированных файлов \(см\. [режим инкрементального анализа](https://pvs-studio.ru/ru/docs/manual/0024/)\)\. Так разработчик сразу находит и устраняет многие ошибки и опечатки\. Это во много раз быстрее и дешевле, чем обнаружить эти ошибки при тестировании\. Подробнее эта тема рассматривалась в статье "[Лев Толстой и статический анализ кода](https://pvs-studio.ru/ru/blog/posts/0105/)"\. Хорошая статья\. Рекомендую почитать вводную часть, чтобы понять идеологию использования таких инструментов, как PVS\-Studio\. ## Странные цвета ```cpp // 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; } ``` Случайно, вместо '&' используется '&&'\. В результате от цвета остаются рожки и ножки в виде нуля или единицы\. Идентичную беду можно наблюдать в файле "ccheckpointsa\.cpp"\. Другая беда с цветопередачей\. ```cpp // 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; }; .... } ``` Два раза копируется красный цвет, но не копируется синий\. Правленый код, должен быть таким: ```cpp { m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucBlue = ucBlue; }; ``` Идентичная проблема имеется в файле cdebugechopacket\.h\. Вообще, целый ряд ошибок в игре дублируется в двух файлах\. Как мне кажется, один из файлов относится к клиентской части, а второй к серверной\. Чувствуется вся мощь технологии Copy\-Paste :\)\. ## Что\-то не так с utf8 ```cpp // 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; .... } ``` Размер типа wchar\_t в Windows составляет 2 байта\. Диапазон его значений равен \[0\.\.65535\]\. Это значит, что сравнение с числами 0x10000, 0x200000, 0x4000000, 0x7fffffff не имеет смысла\. Наверное, код должен был быть написан как\-то иначе\. ## Забытый break ```cpp // 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; .... } ``` В этом коде забыт оператор 'break'\. В результате ситуация "BANNED\_IP", обрабатывается так же, как и "BANNED\_ACCOUNT"\. ## Странные проверки ```cpp // cvehicleupgrades.cpp bool CVehicleUpgrades::IsUpgradeCompatible ( unsigned short usUpgrade ) { .... case 402: return ( us == 1009 || us == 1009 || us == 1010 ); .... } ``` Переменная два раза сравнивается с числом 1009\. Чуть ниже можно найти ещё идентичное двойное сравнение\. Следующее подозрительное сравнение: ```cpp // cclientplayervoice.h bool IsTempoChanged(void) { return m_fSampleRate != 0.0f || m_fSampleRate != 0.0f || m_fTempo != 0.0f; } ``` Эта же ошибка скопирована в файл cclientsound\.h\. ## Разыменовывание нулевого указателя ```cpp // 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() ); } .... } ``` Если не удалось создать объект "игрок", то программа пытается напечатать соответствующую информацию в консоль\. Но не удачно\. Плохая идея использовать нулевой указатель, вызывая функцию "pPlayer\-\>GetSourceIP\(\)"\. Другой нулевой указатель разыменовывается здесь: ```cpp // clientcommands.cpp void COMMAND_MessageTarget ( const char* szCmdLine ) { if ( !(szCmdLine || szCmdLine[0]) ) return; .... } ``` Если указатель szCmdLine равен нулю, то произойдет его разыменование\. Скорее всего, здесь должно быть так: ```cpp if ( !(szCmdLine && szCmdLine[0]) ) ``` Больше всего, мне понравился вот этот фрагмент кода: ```cpp // 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; } .... } ``` **Красивый Copy\-Paste\.** Вместо последней функции memcpy\(\) должна вызываться функция memset\(\)\. ## Неочищенные массивы Есть целый ряд ошибок, связанный с неочищенными массивами\. Ошибки можно разделить на две категории\. Первая \- не удалены элементы\. Вторая \- не во все элементы массива записаны нулевые значения\. ### Неудалённые элементы ```cpp // 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; } .... } ``` Функция empty\(\) проверяет, содержит контейнер элементы или нет\. Чтобы удалить элементы из контейнера 'm\_TimingMap', следовало вызвать функцию clear\(\)\. Следующий пример: ```cpp // cclientcolsphere.cpp void CreateSphereFaces ( std::vector < SFace >& faceList, int iIterations ) { int numFaces = (int)( pow ( 4.0, iIterations ) * 8 ); faceList.empty (); faceList.reserve ( numFaces ); .... } ``` Ещё несколько таких ошибок есть в файле cresource\.cpp\. Примечание\. Если кто\-то пропустил начало статьи и решил читать с середины: где находятся эта и остальные шибки, можно узнать в файле [mtasa\-review\.txt](https://pvs-studio.com/external-pictures/txt/mtasa-review.txt)\. ### Теперь об ошибках заполнения нулями ```cpp // crashhandler.cpp LPCTSTR __stdcall GetFaultReason(EXCEPTION_POINTERS * pExPtrs) { .... PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ; FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ; .... } ``` На первый взгляд всё хорошо\. Вот только FillMemory\(\) не будет иметь никакого эффекта\. FillMemory\(\) и memset\(\) это не одно и тоже\. Вот смотрите: ```cpp #define RtlFillMemory(Destination,Length,Fill) \ memset((Destination),(Fill),(Length)) #define FillMemory RtlFillMemory ``` Второй и третий аргумент меняются местами\. По этому, правильно будет так: ```cpp FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ; ``` Аналогичная путаница существует в файле ccrashhandlerapi\.cpp\. И последний фрагмент кода на рассматриваемую тему\. Здесь очищается только один байт\. ```cpp // hash.hpp unsigned char m_buffer[64]; void CMD5Hasher::Finalize ( void ) { .... // Zeroize sensitive information memset ( m_buffer, 0, sizeof (*m_buffer) ); .... } ``` Звездочка '\*' является лишней\. Должно быть написано "sizeof \(m\_buffer\)"\. ## Неинициализированная переменная ```cpp // ceguiwindow.cpp Vector2 Window::windowToScreen(const UVector2& vec) const { Vector2 base = d_parent ? d_parent->windowToScreen(base) + getAbsolutePosition() : getAbsolutePosition(); .... } ``` Переменная 'base' используется для инициализации самой себя\. Ещё одна такая ошибка есть несколькими строчками ниже\. ## Выход за границу массива ```cpp // 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; .... } ``` Последняя строчка "m\_DevInfo\.axis\[7\]\.bEnabled \= 0;" лишняя\. Другая ошибка ```cpp // 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 (); .... } ``` И ещё одна: ```cpp // 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 ); .... } .... } ``` Ещё, как минимум одна ошибка выхода за границы массива есть в файле cpoolssa\.cpp\. Но описывать её в статье я не стал\. Получается длинноватый пример, а как сделать коротко и понятно, я не знаю\. Эту и другие ошибки, как я уже говорил можно посмотреть в более полном отчёте\. ## Пропущено слово 'throw' ```cpp // fallistheader.cpp ListHeaderSegment* FalagardListHeader::createNewSegment(const String& name) const { if (d_segmentWidgetType.empty()) { InvalidRequestException( "FalagardListHeader::createNewSegment - " "Segment widget type has not been set!"); } return ....; } ``` Здесь должно было быть "throw InvalidRequestException\(\.\.\.\.\)"\. Другой фрагмент кода\. ```cpp // 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"); .... } ``` Должно быть: throw std::length\_error\(\.\.\.\.\)\. ## Ой: free\(new T\[n\]\) ```cpp // 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; } .... } ``` Память выделяется с помощью оператора 'new'\. А освободить её пытаются с помощью функции free\(\)\. Результат непредсказуем\. ## Всегда истинные/ложные условия ```cpp // cproxydirect3ddevice9.cpp #define D3DCLEAR_ZBUFFER 0x00000002l HRESULT CProxyDirect3DDevice9::Clear(....) { if ( Flags | D3DCLEAR_ZBUFFER ) CGraphics::GetSingleton(). GetRenderItemManager()->SaveReadableDepthBuffer(); .... } ``` Программист хотел проверить определенный бит в переменной Flag\. Из\-за опечатки, вместо операции '&' он использовал операцию '\|'\. В результате, условие всегда выполняется\. Аналогичная путаница есть в файле cvehiclesa\.cpp\. Следующая ошибка с проверкой: unsigned\_value < 0\. ```cpp // crenderitem.effectcloner.cpp unsigned long long Get ( void ); void CEffectClonerImpl::MaybeTidyUp ( void ) { .... if ( m_TidyupTimer.Get () < 0 ) return; .... } ``` Функция Get\(\) возвращает значение беззнакового типа 'unsigned long long'\. Значит проверка "m\_TidyupTimer\.Get \(\) < 0" не имеет смысла\. Другие ошибки этой разновидности в встречаются в файлах: csettings\.cpp, cmultiplayersa\_1\.3\.cpp, cvehiclerpcs\.cpp\. ## Это может работать, но лучше провести рефакторинг Многие сообщения, выданные PVS\-Studio, диагностируют ошибки, которые, скорее всего, никак не проявят себя\. Я не люблю писать про такие ошибки\. Это не интересно\. Приведу только несколько примеров\. ```cpp // cluaacldefs.cpp int CLuaACLDefs::aclListRights ( lua_State* luaVM ) { char szRightName [128]; .... strncat ( szRightName, (*iter)->GetRightName (), 128 ); .... } ``` Третий аргумент функции strncat\(\) указывает не размер буфера, а сколько ещё символов в него можно поместить\. Теоретически, здесь возможно переполнение буфера\. На практике, скорее всего это никогда не произойдёт\. Подробнее про данный тип ошибки, можно почитать в описании диагностики [V645](https://pvs-studio.ru/ru/docs/warnings/v645/)\. Ещё один пример\. ```cpp // cscreenshot.cpp void CScreenShot::BeginSave (....) { .... HANDLE hThread = CreateThread ( NULL, 0, (LPTHREAD_START_ROUTINE)CScreenShot::ThreadProc, NULL, CREATE_SUSPENDED, NULL ); .... } ``` Во многих местах игры используются функции CreateThread\(\)/ExitThread\(\)\. Как правило, это не совсем верно\. Следует использовать функции \_beginthreadex\(\)/\_endthreadex\(\)\. Подробнее про это, можно узнать из описания диагностики [V513](https://pvs-studio.ru/ru/docs/warnings/v513/)\. ## Надо где\-то остановиться Я описал далеко не все из недочетов, которые я заметил\. Однако, пора останавливаться\. Статья уже и так достаточно велика\. С другими ошибками, вы можете познакомиться в файле [mtasa\-review\.txt](https://pvs-studio.com/external-pictures/txt/mtasa-review.txt)\. Например, там есть следующие типы ошибок, отсутствующие в статье: * одинаковые ветки в условном операторе: if \(\) \{ aa \} else \{ aa \}; * проверка указателя, который возвращает оператор 'new', на равенство нулю: p \= new T; if \(\!p\) \{ aa \}; * плохой способ использования \#pragma для подавления предупреждений \(не используется push/pop\); * в классах есть виртуальные функции, но нет виртуального деструктора; * указатель в начале разыменовывается, а уже затем проверяется на равенство нулю; * повторяющиеся условия: if \(X\) \{ if \(X\) \{ aa \} \}; * прочее\. ## Заключение Анализатор PVS\-Studio может эффективно быть использован для раннего устранения многих ошибок, как в игровых проектах, так и в любых других\. Алгоритмические ошибки он не найдёт \(для этого нужен искусственный интеллект\)\. Зато существенно время, которое бесполезно тратится на поиск глупых ошибок и опечаток\. Программисты тратят на простые дефекты гораздо больше, чем думают\. Даже в уже отлаженном и оттестированном коде, [таких ошибок много](https://pvs-studio.ru/ru/blog/posts/0203/)\. А при написании нового кода, таких ошибок исправляется в 10 раз больше\.