VPF::Проверка open-source игры Multi Theft Auto - Форум программистов

Thunderbolt

Дата 16.8.2013, 14:14 (ссылка)

(нет голосов)

Загрузка ...

DevRel

Профиль
Группа: Участник
Сообщений: 122
Регистрация: 7.11.2007
Где: Тула

Репутация: 6
Всего: 16

Мы давно не проверяли игры с помощью PVS-Studio. Решили это исправить и выбрали проект MTA. Multi Theft Auto (MTA) является модификацией для PC версий игры Grand Theft Auto: San Andreas от Rockstar North. MTA позволяет игрокам со всего мира играть друг против друга в режиме онлайн. Как написано в Wikipedia, особенностью игры является "оптимизированный код с наименьшим количеством сбоев". Что же, давайте посмотрим, что скажет анализатор кода.

user posted image

Введение

В этот раз, я решил не включать в статью диагностические сообщения, которые анализатор 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 smile

.

Что-то не так с 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 раз больше.

--------------------

Карпов Андрей, DevRel в PVS-Studio.

EgoBrain

Дата 19.8.2013, 02:45 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 537
Регистрация: 23.3.2008
Где: Комната

Репутация: нет
Всего: 2

Странно, многие ошибки должны были быть замеченными на этапе написания, если разрабы конечно не в нодпаде++ каком-нибудь пишут.
Например пропущенный break, студия тут же выдаст предупреждение. Ну или двойное сравнение

Цитата

us == 1009 || us == 1009

, думаю оптимизатор его опустит.
Разве не так?
На многие ошибки посмотришь, и даже не верится что такое могли допустить опытные программисты задействованные в таком масштабном проекте

Цитата

И последний фрагмент кода на рассматриваемую тему. Здесь очищается только один байт.

Код


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

Цитата

Другой нулевой указатель разыменовывается здесь:

Код


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

Если указатель szCmdLine равен нулю, то произойдет его разыменование.

Скорее всего, здесь должно быть так:

Код


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

bsa

Дата 19.8.2013, 13:42 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Модератор
Сообщений: 9185
Регистрация: 6.4.2006
Где: Москва, Россия

Репутация: 85
Всего: 196

EgoBrain, и на старуху бывает проруха. smile

--------------------

Правильно заданный вопрос - половина ответа
Отзывы о работе модератора.

0 Пользователей читают эту тему (0 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| C/C++: Для новичков \| Следующая тема »