Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

Проверка open-source игры Multi Theft Auto

Anna | 25.06.2014 | нет комментариев
MTA & PVS-Studio
Мы давным-давно не проверяли игры с поддержкой 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.

Вообще, целый ряд ошибок в игре дублируется в 2-х файлах. Как мне кажется, один из файлов относится к клиентской части, а 2-й к серверной. Чувствуется каждая мощь спецтехнологии 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]. Это значит, что сопоставление с числами 0×10000, 0×200000, 0×4000000, 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 ) ;
  ....
}

На 1-й взор всё отлично. Вот только FillMemory() не будет иметь никакого результата. FillMemory() и memset() это не одно и тоже. Вот глядите:

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

2-й и 3-й довод меняются местами. По этому, верно будет так:

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

3-й довод функции 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 раз огромнее.

 

Источник: programmingmaster.ru

 

Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB