Показано с 1 по 4 из 4

Тема: Анализ кода MTA от PVS-Studio

  1. #1
    Пользователь Аватар для Ted_Bundy
    Регистрация
    24.05.2013
    Возраст
    31
    Сообщений
    46
    Репутация: 9

    Звание: пока не определено

    Анализ кода MTA от PVS-Studio

    Внимание: Copy-Paste



    Мы давно не проверяли игры с помощью 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.

    Странные цвета

    PHP код:
    // c3dmarkersa.cpp
    SColor C3DMarkerSA::GetColor()
    {
      
    DEBUG_TRACE("RGBA C3DMarkerSA::GetColor()");
      
    // From ABGR
      
    unsigned long ulABGR this->GetInterface()->rwColour;
      
    SColor color;
      
    color.= ( ulABGR >> 24 ) && 0xff;
      
    color.= ( ulABGR >> 16 ) && 0xff;
      
    color.= ( ulABGR >> ) && 0xff;
      
    color.ulABGR && 0xff;
      return 
    color;

    Случайно, вместо '&' используется '&&'. В результате от цвета остаются рожки и ножки в виде нуля или единицы.

    Идентичную беду можно наблюдать в файле «ccheckpointsa.cpp».

    Другая беда с цветопередачей.

    PHP код:
    // cchatechopacket.h
    class CChatEchoPacket : public CPacket
    {
      ....
      
    inline void SetColorunsigned char ucRed,
                            
    unsigned char ucGreen,
                            
    unsigned char ucBlue )
      { 
    m_ucRed ucRedm_ucGreen ucGreenm_ucRed ucRed; };
      ....

    Два раза копируется красный цвет, но не копируется синий. Правленый код, должен быть таким:
    Код:
    { m_ucRed = ucRed; m_ucGreen = ucGreen; m_ucBlue = ucBlue; };
    Идентичная проблема имеется в файле cdebugechopacket.h.

    Вообще, целый ряд ошибок в игре дублируется в двух файлах. Как мне кажется, один из файлов относится к клиентской части, а второй к серверной. Чувствуется вся мощь технологии Copy-Paste .

    Что-то не так с utf8

    PHP код:
    // utf8.h
    int
    utf8_wctomb 
    (unsigned char *destwchar_t wcint 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

    PHP код:
    // 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».

    Странные проверки

    PHP код:
    // cvehicleupgrades.cpp
    bool CVehicleUpgrades::IsUpgradeCompatible (
      
    unsigned short usUpgrade )
    {
      ....
      case 
    402: return ( us == 1009 || us == 1009 || us == 1010 );
      ....

    Переменная два раза сравнивается с числом 1009. Чуть ниже можно найти ещё идентичное двойное сравнение.

    Следующее подозрительное сравнение:
    PHP код:
    // cclientplayervoice.h
    bool IsTempoChanged(void)

      return 
    m_fSampleRate != 0.0f ||
             
    m_fSampleRate != 0.0f ||
             
    m_fTempo != 0.0f;

    Эта же ошибка скопирована в файл cclientsound.h.

    Разыменовывание нулевого указателя

    PHP код:
    // cgame.cpp
    void CGame::Packet_PlayerJoinData(CPlayerJoinDataPacketPacket)
    {
      ....
      
    // Add the player
      
    CPlayerpPlayer 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()».

    Другой нулевой указатель разыменовывается здесь:
    PHP код:
    // clientcommands.cpp
    void COMMAND_MessageTarget ( const charszCmdLine )
    {
      if ( !(
    szCmdLine || szCmdLine[0]) )
        return;
      ....

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

    Скорее всего, здесь должно быть так:
    Код:
    if ( !(szCmdLine && szCmdLine[0]) )
    Больше всего, мне понравился вот этот фрагмент кода:
    PHP код:
    // cdirect3ddata.cpp
    void CDirect3DData::GetTransform (....) 
    {
      switch ( 
    dwRequestedMatrix )
      {
        case 
    D3DTS_VIEW:
          
    memcpy (pMatrixOut, &m_mViewMatrixsizeof(D3DMATRIX));
          break;
        case 
    D3DTS_PROJECTION:
          
    memcpy (pMatrixOut, &m_mProjMatrixsizeof(D3DMATRIX));
          break;
        case 
    D3DTS_WORLD:
          
    memcpy (pMatrixOut, &m_mWorldMatrixsizeof(D3DMATRIX));
          break;
        default:
          
    // Zero out the structure for the user.
          
    memcpy (pMatrixOut0sizeof D3DMATRIX ) );
          break;
      }
      ....

    Красивый Copy-Paste. Вместо последней функции memcpy() должна вызываться функция memset().

    Неочищенные массивы

    Есть целый ряд ошибок, связанный с неочищенными массивами. Ошибки можно разделить на две категории. Первая — не удалены элементы. Вторая — не во все элементы массива записаны нулевые значения.

    Неудалённые элементы

    PHP код:
    // cperfstat.functiontiming.cpp
    std::map SStringSFunctionTimingInfo m_TimingMap;

    void CPerfStatFunctionTimingImpl::DoPulse void )
    {
      ....
      
    // Do nothing if not active
      
    if ( !m_bIsActive )
      {
        
    m_TimingMap.empty ();
        return;
      }
      ....

    Функция empty() проверяет, содержит контейнер элементы или нет. Чтобы удалить элементы из контейнера 'm_TimingMap', следовало вызвать функцию clear().

    Следующий пример:
    PHP код:
    // cclientcolsphere.cpp
    void CreateSphereFaces (
      
    std::vector SFace >& faceListint iIterations )
    {
      
    int numFaces = (int)( pow 4.0iIterations ) * );
      
    faceList.empty ();
      
    faceList.reserve numFaces );
      ....

    Ещё несколько таких ошибок есть в файле cresource.cpp.

    Примечание. Если кто-то пропустил начало статьи и решил читать с середины: где находятся эта и остальные шибки, можно узнать в файле mtasa-review.txt.

    Теперь об ошибках заполнения нулями

    PHP код:
    // crashhandler.cpp
    LPCTSTR __stdcall GetFaultReason(EXCEPTION_POINTERS pExPtrs)
    {
      ....
      
    PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ;
      
    FillMemory pSym NULL SYM_BUFF_SIZE ) ;
      ....

    На первый взгляд всё хорошо. Вот только FillMemory() не будет иметь никакого эффекта. FillMemory() и memset() это не одно и тоже. Вот смотрите:
    PHP код:
    #define RtlFillMemory(Destination,Length,Fill) \
      
    memset((Destination),(Fill),(Length))
    #define FillMemory RtlFillMemory 
    Второй и третий аргумент меняются местами. По этому, правильно будет так:
    Код:
    FillMemory ( pSym , SYM_BUFF_SIZE, 0 ) ;
    Аналогичная путаница существует в файле ccrashhandlerapi.cpp.

    И последний фрагмент кода на рассматриваемую тему. Здесь очищается только один байт.
    PHP код:
    // hash.hpp
    unsigned char m_buffer[64];
    void CMD5Hasher::Finalize void )
    {
      ....
      
    // Zeroize sensitive information
      
    memset m_buffer0sizeof (*m_buffer) );
      ....

    Звездочка '*' является лишней. Должно быть написано «sizeof (m_buffer)».

    Неинициализированная переменная

    PHP код:
    // ceguiwindow.cpp
    Vector2 Window::windowToScreen(const UVector2vec) const
    {
      
    Vector2 base d_parent ?
        
    d_parent->windowToScreen(base) + getAbsolutePosition() :
        
    getAbsolutePosition();
      ....

    Переменная 'base' используется для инициализации самой себя. Ещё одна такая ошибка есть несколькими строчками ниже.

    Выход за границу массива

    PHP код:
    // 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;» лишняя.

    Другая ошибка
    PHP код:
    // cwatermanagersa.cpp
    class CWaterPolySAInterface
    {
    public:
      
    WORD m_wVertexIDs[3];
    };

    CWaterPolyCWaterManagerSA::CreateQuad ( const CVectorvecBL, const CVectorvecBR, const CVectorvecTL, const CVectorvecTRbool bShallow )
    {
      ....
      
    pInterface->m_wVertexIDs ] = pV1->GetID ();
      
    pInterface->m_wVertexIDs ] = pV2->GetID ();
      
    pInterface->m_wVertexIDs ] = pV3->GetID ();
      
    pInterface->m_wVertexIDs ] = pV4->GetID ();
      ....

    И ещё одна:
    PHP код:
    // cmainmenu.cpp
    #define CORE_MTA_NEWS_ITEMS 3

    CGUILabelm_pNewsItemLabels[CORE_MTA_NEWS_ITEMS];
    CGUILabelm_pNewsItemShadowLabels[CORE_MTA_NEWS_ITEMS];

    void CMainMenu::SetNewsHeadline (....)
    {
      ....
      for ( 
    char i=0<= CORE_MTA_NEWS_ITEMSi++ )
      {
        
    m_pNewsItemLabels]->SetFont szFontName );
        
    m_pNewsItemShadowLabels]->SetFont szFontName );
        ....
      }
      ....

    Ещё, как минимум одна ошибка выхода за границы массива есть в файле cpoolssa.cpp. Но описывать её в статье я не стал. Получается длинноватый пример, а как сделать коротко и понятно, я не знаю. Эту и другие ошибки, как я уже говорил можно посмотреть в более полном отчёте.

    Пропущено слово 'throw'

    PHP код:
    // fallistheader.cpp
    ListHeaderSegment*
    FalagardListHeader::createNewSegment(const Stringname) const
    {
      if (
    d_segmentWidgetType.empty())
      {
        
    InvalidRequestException(
          
    "FalagardListHeader::createNewSegment - "
          "Segment widget type has not been set!"
    );
      }
      return ....;

    Здесь должно было быть «throw InvalidRequestException(....)».

    Другой фрагмент кода.
    PHP код:
    // 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])

    PHP код:
    // cresourcechecker.cpp
    int CResourceChecker::ReplaceFilesInZIP(....)
    {
      ....
      
    // Load file into a buffer
      
    buf = new charulLength ];
      if ( 
    fread buf1ulLengthpFile ) != ulLength )
      {
        
    freebuf );
        
    buf NULL;
      }
      ....


    Память выделяется с помощью оператора 'new'. А освободить её пытаются с помощью функции free(). Результат непредсказуем.

    Всегда истинные/ложные условия

    PHP код:
    // cproxydirect3ddevice9.cpp
    #define D3DCLEAR_ZBUFFER 0x00000002l
    HRESULT CProxyDirect3DDevice9::Clear(....)
    {
      if ( 
    Flags D3DCLEAR_ZBUFFER )
        
    CGraphics::GetSingleton().
          
    GetRenderItemManager()->SaveReadableDepthBuffer();
      ....

    Программист хотел проверить определенный бит в переменной Flag. Из-за опечатки, вместо операции '&' он использовал операцию '|'. В результате, условие всегда выполняется.

    Аналогичная путаница есть в файле cvehiclesa.cpp.

    Следующая ошибка с проверкой: unsigned_value < 0.
    PHP код:
    // crenderitem.effectcloner.cpp
    unsigned long long Get void );

    void CEffectClonerImpl::MaybeTidyUp void )
    {
      ....
      if ( 
    m_TidyupTimer.Get () < )
        return;
      ....

    Функция Get() возвращает значение беззнакового типа 'unsigned long long'. Значит проверка «m_TidyupTimer.Get () < 0» не имеет смысла. Другие ошибки этой разновидности в встречаются в файлах: csettings.cpp, cmultiplayersa_1.3.cpp, cvehiclerpcs.cpp.

    Это может работать, но лучше провести рефакторинг

    Многие сообщения, выданные PVS-Studio, диагностируют ошибки, которые, скорее всего, никак не проявят себя. Я не люблю писать про такие ошибки. Это не интересно. Приведу только несколько примеров.

    PHP код:
    // cluaacldefs.cpp
    int CLuaACLDefs::aclListRights lua_StateluaVM )
    {
      
    char szRightName [128];
      ....
      
    strncat szRightName, (*iter)->GetRightName (), 128 );
      ....

    Третий аргумент функции strncat() указывает не размер буфера, а сколько ещё символов в него можно поместить. Теоретически, здесь возможно переполнение буфера. На практике, скорее всего это никогда не произойдёт. Подробнее про данный тип ошибки, можно почитать в описании диагностики V645.

    Ещё один пример.
    PHP код:
    // 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 раз больше.

    via habrahabr.ru

  2. #2
    Диванный критик
    Регистрация
    03.07.2011
    Адрес
    Вологда
    Возраст
    28
    Сообщений
    2,107
    Репутация: 269

    Звание: как роза среди колючек

    Re: Анализ кода MTA от PVS-Studio

    Если не выходить за границу «объектно-ориентированных» методов, чтобы остаться в рамках «хорошего программирования и проектирования», то в итоге обязательно получается нечто, по большей части не имеющее смысла. (C) Bjarne Stroustrup


    http://www.lua.org/about.html
    Please do not write it as "LUA", which is both ugly and confusing, because then it becomes an acronym with different meanings for different people. So, please, write "Lua" right!

  3. #3
    Забаненный
    Регистрация
    03.08.2015
    Сообщений
    4
    Репутация: 10

    Звание: на пути к лучшему

    Анализ кода MTA от PVS Studio

    Но, тем не менее, там, где благодаря Coverity «было найдено несколько новых ошибок», PVS-Studio находит их целый вагон и маленькую тележку.

  4. #4
    Проверенный Аватар для Arios Jentu
    Регистрация
    15.02.2012
    Адрес
    Таанаб - Пандат
    Сообщений
    2,433
    Репутация: 319

    Звание: как роза среди колючек

    Re: Анализ кода MTA от PVS Studio

    Цитата Сообщение от AlexKotovSi Посмотреть сообщение
    Но, тем не менее, там, где благодаря Coverity «было найдено несколько новых ошибок», PVS-Studio находит их целый вагон и маленькую тележку.
    Если бы болотники выложили свои исходники, вы бы нашли там больше ошибок, чем тут помарок. Идея бред. А кал просто боится публиковать свой кал.

Похожие темы

  1. [Other] [MAP] Foto Studio
    от Lutyk в разделе Вопросы по маппингу
    Ответов: 3
    Последнее сообщение: 11.07.2013, 05:05
  2. [Web] Studio-Fox
    от Des в разделе Услуги
    Ответов: 4
    Последнее сообщение: 30.06.2013, 00:40
  3. XN-STUDIO.
    от Мики^_^ в разделе Услуги
    Ответов: 1
    Последнее сообщение: 21.06.2013, 17:01
  4. Нужна помощь по постройке кода в notepad++
    от Bond_Man в разделе Вопросы по скриптингу
    Ответов: 10
    Последнее сообщение: 20.06.2013, 09:50
  5. [WEB] One-Mix Studio
    от Skyfall в разделе Услуги
    Ответов: 1
    Последнее сообщение: 25.04.2013, 23:50

Ваши права

  • Вы не можете создавать новые темы
  • Вы не можете отвечать в темах
  • Вы не можете прикреплять вложения
  • Вы не можете редактировать свои сообщения
  •