Мы давно не проверяли игры с помощью PVS-Studio. Решили это исправить и выбрали проект MTA. Multi Theft Auto (MTA) является модификацией для PC версий игры Grand Theft Auto: San Andreas от Rockstar North. MTA позволяет игрокам со всего мира играть друг против друга в режиме онлайн. Как написано в Wikipedia, особенностью игры является "оптимизированный код с наименьшим количеством сбоев". Что же, давайте посмотрим, что скажет анализатор кода.
Рисунок 1. Логотип игры Multi Theft Auto
В этот раз, я решил не включать в статью диагностические сообщения, которые анализатор 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.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 не имеет смысла. Наверное, код должен был быть написан как-то иначе.
// 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. Но описывать её в статье я не стал. Получается длинноватый пример, а как сделать коротко и понятно, я не знаю. Эту и другие ошибки, как я уже говорил можно посмотреть в более полном отчёте.
// 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(....).
// 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.
Например, там есть следующие типы ошибок, отсутствующие в статье:
Анализатор PVS-Studio может эффективно быть использован для раннего устранения многих ошибок, как в игровых проектах, так и в любых других. Алгоритмические ошибки он не найдёт (для этого нужен искусственный интеллект). Зато существенно время, которое бесполезно тратится на поиск глупых ошибок и опечаток. Программисты тратят на простые дефекты гораздо больше, чем думают. Даже в уже отлаженном и оттестированном коде, таких ошибок много. А при написании нового кода, таких ошибок исправляется в 10 раз больше.