Мы давно не проверяли игры с помощью PVS-Studio. Решили это исправить и выбрали проект MTA. Multi Theft Auto (MTA) является модификацией для PC версий игры Grand Theft Auto: San Andreas от Rockstar North. MTA позволяет игрокам со всего мира играть друг против друга в режиме онлайн. Как написано в Wikipedia, особенностью игры является «оптимизированный код с наименьшим количеством сбоев». Что же, давайте посмотрим, что скажет анализатор кода.
Введение
В этот раз, я решил не включать в статью диагностические сообщения, которые анализатор PVS-Studio выдаёт на подозрительные строки. Все равно я даю пояснения к фрагментам кода. Если интересно узнать, в какой именно строке найдена ошибка и какое диагностическое сообщение выдал анализатор, то это можно посмотреть в файле mtasa-review.txt.
Когда я просматривал проект, я выписывал в файл mtasa-review.txt фрагменты кода, которые мне показались подозрительными. Позже, основываясь на нём, я написал эту статью.
Важно. В файл попали только те фрагменты код, которые не понравились лично мне. Я не участвую в разработке MTA и не представляю, как и что в ней работает. Поэтому, наверняка я местами ошибся: включил в список корректный код и пропустил настоящие ошибки. А где-то вообще поленился и не стал описывать не совсем правильный вызов функции printf(). Прошу разработчиков из MTA Team не полагаться на этот текст и самостоятельно проверить проект. Проект достаточно большой и демонстрационной версии не хватит для полноценной проверки. Однако мы поддерживаем бесплатные open-source проекты. Напишите нам, и мы договоримся на тему бесплатной версии PVS-Studio.
Итак, игра Multi Theft Auto является open-source проектом, написанным на языке Си/Си++:
Для проверки я использовал анализатор PVS-Studio 5.05:
- страница инструмента;
- скачать (демонстрационная версия ограничена только количеством переходов к коду с ошибками);
- примеры найденных ошибок.
Посмотрим теперь, что смог найти анализатор PVS-Studio в этой игре. Ошибок не так уж и много. Причём большинство из них содержится в редко используемых частях программы (обработчики ошибок). Это не удивительно. Большинство ошибок исправляется другими, более медленными и дорогими методами. Правильное использование статического анализа — регулярный запуск. Например, анализатор PVS-Studio умеет запускаться для только что изменённых и скомпилированных файлов (см. режим инкрементального анализа). Так разработчик сразу находит и устраняет многие ошибки и опечатки. Это во много раз быстрее и дешевле, чем обнаружить эти ошибки при тестировании. Подробнее эта тема рассматривалась в статье "Лев Толстой и статический анализ кода". Хорошая статья. Рекомендую почитать вводную часть, чтобы понять идеологию использования таких инструментов, как PVS-Studio.
Странные цвета
// 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».
Другая беда с цветопередачей.
// 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; }; .... }Два раза копируется красный цвет, но не копируется синий. Правленый код, должен быть таким:
{ m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucBlue = ucBlue; };Идентичная проблема имеется в файле cdebugechopacket.h.
Вообще, целый ряд ошибок в игре дублируется в двух файлах. Как мне кажется, один из файлов относится к клинской части, а второй к серверной. Чувствуется вся мощь технологии Copy-Paste :).
Что-то не так с 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; .... }Размер типа wchar_t в Windows составляет 2 байта. Диапазон его значений равен [0..65535]. Это значит, что сравнение с числами 0x10000, 0x200000, 0x4000000, 0x7fffffff не имеет смысла. Наверное, код должен был быть написан как-то иначе.
Забытый 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; .... }В этом коде забыт оператор ‘break’. В результате ситуация «BANNED_IP», обрабатывается так же, как и «BANNED_ACCOUNT».
Странные проверки
// cvehicleupgrades.cpp bool CVehicleUpgrades::IsUpgradeCompatible ( unsigned short usUpgrade ) { .... case 402: return ( us == 1009 || us == 1009 || us == 1010 ); .... }Переменная два раза сравнивается с числом 1009. Чуть ниже можно найти ещё идентичное двойное сравнение.
Следующее подозрительное сравнение:
// cclientplayervoice.h bool IsTempoChanged(void) { return m_fSampleRate != 0.0f || m_fSampleRate != 0.0f || m_fTempo != 0.0f; }Эта же ошибка скопирована в файл cclientsound.h.
Разыменовывание нулевого указателя
// 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()».
Другой нулевой указатель разыменовывается здесь:
// clientcommands.cpp void COMMAND_MessageTarget ( const char* szCmdLine ) { if ( !(szCmdLine || szCmdLine[0]) ) return; .... }Если указатель szCmdLine равен нулю, то произойдет его разыменование.
Скорее всего, здесь должно быть так:
if ( !(szCmdLine && szCmdLine[0]) )Больше всего, мне понравился вот этот фрагмент кода:
// 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().
Неочищенные массивы
Есть целый ряд ошибок, связанный с неочищенными массивами. Ошибки можно разделить на две категории. Первая — не удалены элементы. Вторая — не во все элементы массива записаны нулевые значения.
Неудалённые элементы
// 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().
Следующий пример:
// 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.
Теперь об ошибках заполнения нулями
// crashhandler.cpp LPCTSTR __stdcall GetFaultReason(EXCEPTION_POINTERS * pExPtrs) { .... PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ; FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ; .... }На первый взгляд всё хорошо. Вот только FillMemory() не будет иметь никакого эффекта. FillMemory() и memset() это не одно и тоже. Вот смотрите:
#define RtlFillMemory(Destination,Length,Fill) \ memset((Destination),(Fill),(Length)) #define FillMemory RtlFillMemoryВторой и третий аргумент меняются местами. По этому, правильно будет так:
FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ;Аналогичная путаница существует в файле ccrashhandlerapi.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)».
Неинициализированная переменная
// ceguiwindow.cpp Vector2 Window::windowToScreen(const UVector2& vec) const { Vector2 base = d_parent ? d_parent->windowToScreen(base) + getAbsolutePosition() : getAbsolutePosition(); .... }Переменная ‘base’ используется для инициализации самой себя. Ещё одна такая ошибка есть несколькими строчками ниже.
Выход за границу массива
// 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;» лишняя.
Другая ошибка
// 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 (); .... }И ещё одна:
// 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’
// 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(….)».
Другой фрагмент кода.
// 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])
// 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(). Результат непредсказуем.
Всегда истинные/ложные условия
// cproxydirect3ddevice9.cpp #define D3DCLEAR_ZBUFFER 0x00000002l HRESULT CProxyDirect3DDevice9::Clear(....) { if ( Flags | D3DCLEAR_ZBUFFER ) CGraphics::GetSingleton(). GetRenderItemManager()->SaveReadableDepthBuffer(); .... }Программист хотел проверить определенный бит в переменной Flag. Из-за опечатки, вместо операции ‘&’ он использовал операцию ‘|’. В результате, условие всегда выполняется.
Аналогичная путаница есть в файле cvehiclesa.cpp.
Следующая ошибка с проверкой: unsigned_value < 0.
// 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, диагностируют ошибки, которые, скорее всего, никак не проявят себя. Я не люблю писать про такие ошибки. Это не интересно. Приведу только несколько примеров.
// cluaacldefs.cpp int CLuaACLDefs::aclListRights ( lua_State* luaVM ) { char szRightName [128]; .... strncat ( szRightName, (*iter)->GetRightName (), 128 ); .... }Третий аргумент функции strncat() указывает не размер буфера, а сколько ещё символов в него можно поместить. Теоретически, здесь возможно переполнение буфера. На практике, скорее всего это никогда не произойдёт. Подробнее про данный тип ошибки, можно почитать в описании диагностики V645.
Ещё один пример.
// cscreenshot.cpp void CScreenShot::BeginSave (....) { .... HANDLE hThread = CreateThread ( NULL, 0, (LPTHREAD_START_ROUTINE)CScreenShot::ThreadProc, NULL, CREATE_SUSPENDED, NULL ); .... }Во многих местах игры используются функции CreateThread()/ExitThread(). Как правило, это не совсем верно. Следует использовать функции _beginthreadex()/_endthreadex(). Подробнее про это, можно узнать из описания диагностики V513.
Надо где-то остановиться
Я описал далеко не все из недочетов, которые я заметил. Однако, пора останавливаться. Статья уже и так достаточно велика. С другими ошибками, вы можете познакомиться в файле mtasa-review.txt.
Например, там есть следующие типы ошибок, отсутствующие в статье:
- одинаковые ветки в условном операторе: if () { aa } else { aa };
- проверка указателя, который возвращает оператор ‘new’, на равенство нулю: p = new T; if (!p) { aa };
- плохой способ использования #pragma для подавления предупреждений (не используется push/pop);
- в классах есть виртуальные функции, но нет виртуального деструктора;
- указатель в начале разыменовывается, а уже затем проверяется на равенство нулю;
- повторяющиеся условия: if (X) { if (X) { aa } };
- прочее.
Заключение
Анализатор PVS-Studio может эффективно быть использован для раннего устранения многих ошибок, как в игровых проектах, так и в любых других. Алгоритмические ошибки он не найдёт (для этого нужен искусственный интеллект). Зато существенно время, которое бесполезно тратится на поиск глупых ошибок и опечаток. Программисты тратят на простые дефекты гораздо больше, чем думают. Даже в уже отлаженном и оттестированном коде, таких ошибок много. А при написании нового кода, таких ошибок исправляется в 10 раз больше.
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/190292/
Добавить комментарий