Святослав Размыслов

21 Мар 2016

Теги:

#Cpp

Проверка исходного кода игрового движка Serious Engine v.1.10 к юбилею шутера Serious Sam

21 Мар 2016

Автор: Святослав Размыслов

Введение
Опечатки!
Много странных сравнений
Разные ошибки
Заключение

К юбилею выхода шутера от первого лица Serious Sam, который состоялся в марте 2016 года, разработчики игры из хорватской компании Croteam решили открыть исходный код игрового движка Serious Engine 1 v.1.10. Он заинтересовал много разработчиков, которые захотели изучить и улучшить движок. Я тоже решил поучаствовать в улучшении кода и подготовил статью с обзором ошибок, найденных с помощью статического анализатора PVS-Studio.

Введение

Serious Engine - игровой движок, разработанный хорватской компанией Croteam. Версия v.1.10 использовалась в играх Serious Sam Classic: The First Encounter и Serious Sam Classic: The Second Encounter. Впоследствии компанией Croteam были разработаны более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4, а исходный код движка Serious Engine версии 1.10 был официально открыт и доступен под лицензией GNU General Public License v.2.

Хочу отметить, что проект легко собирается в Visual Studio 2013 и легко проверяется с помощью статического анализатора PVS-Studio 6.02.

Опечатки!

V501 There are identical sub-expressions to the left and to the right of the '==' operator: tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180

class CTexParams {
public:

  inline BOOL IsEqual( CTexParams tp) {
    return tp_iFilter     == tp.tp_iFilter &&
           tp_iAnisotropy == tp_iAnisotropy && // <=
           tp_eWrapU      == tp.tp_eWrapU &&
           tp_eWrapV      == tp.tp_eWrapV; };
  ....
};

Для наглядности я изменил форматирование этого фрагмента кода. Так дефект, который обнаружил анализатор, намного заметней. Переменная сравнивается сама с собой. У объекта с именем 'tp' есть поле 'tp_iAnisotropy', следовательно, по аналогии с соседним кодом часть условия должна выглядеть как "tp_iAnisotropy == tp.tp_iAnisotropy".

V501 There are identical sub-expressions 'GetShadingMapWidth() < 32' to the left and to the right of the '||' operator. terrain.cpp 561

void CTerrain::SetShadowMapsSize(....)
{
  ....
  if(GetShadowMapWidth()<32 || GetShadingMapHeight()<32) {
    ....
  }

  if(GetShadingMapWidth()<32 || GetShadingMapWidth()<32) { // <=
    tr_iShadingMapSizeAspect = 0;
  }
  ....
  PIX pixShadingMapWidth  = GetShadingMapWidth();
  PIX pixShadingMapHeight = GetShadingMapHeight();
  ....
}

Здесь анализатор обнаружил подозрительный код для проверки ширины и высоты некой карты. Точнее только ширины, потому что в коде присутствуют две одинаковые проверки "GetShadingMapWidth()<32". Скорее всего, условие должно быть таким:

if(GetShadingMapWidth()<32 || GetShadingMapHeight()<32) {
  tr_iShadingMapSizeAspect = 0;
}

V501 There are identical sub-expressions '(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)' to the left and to the right of the '&&' operator. worldeditor.h 580

inline BOOL CValuesForPrimitive::operator==(....)
{
  return (
 (....) &&
 (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&// <=
 (vfp_plPrimitive == vfpToCompare.vfp_plPrimitive) &&
 ....
 (vfp_bDummy == vfpToCompare.vfp_bDummy) &&
 (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&// <=
 ....
 (vfp_fXMin == vfpToCompare.vfp_fXMin) &&
 (vfp_fXMax == vfpToCompare.vfp_fXMax) &&
 (vfp_fYMin == vfpToCompare.vfp_fYMin) &&
 (vfp_fYMax == vfpToCompare.vfp_fYMax) &&
 (vfp_fZMin == vfpToCompare.vfp_fZMin) &&
 (vfp_fZMax == vfpToCompare.vfp_fZMax) &&
 ....
);

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

Много странных сравнений

V559 Suspicious assignment inside the condition expression of 'if' operator: pwndView = 0. mainfrm.cpp 697

void CMainFrame::OnCancelMode()
{
  // switches out of eventual direct screen mode
  CWorldEditorView *pwndView = (....)GetActiveView();
  if (pwndView = NULL) {                             // <=
    // get the MDIChildFrame of active window
    CChildFrame *pfrChild = (....)pwndView->GetParentFrame();
    ASSERT(pfrChild!=NULL);
  }
  CMDIFrameWnd::OnCancelMode();
}

В коде движка присутствует много подозрительных сравнений. Например, в этом фрагменте получают указатель "pwndView", а потом ему присваивают NULL, из-за чего условие всегда ложно.

Скорее всего, здесь хотели написать оператор неравенства '!=' и код должен быть таким:

if (pwndView != NULL) {
  // get the MDIChildFrame of active window
  CChildFrame *pfrChild = (....)pwndView->GetParentFrame();
  ASSERT(pfrChild!=NULL);
}

Есть ещё похожий фрагмент кода:

V559 Suspicious assignment inside the condition expression of 'if' operator: pwndView = 0. mainfrm.cpp 710

V547 Expression is always false. Probably the '||' operator should be used here. entity.cpp 3537

enum RenderType {
  ....
  RT_BRUSH       = 4,
  RT_FIELDBRUSH  = 8,
  ....
};

void
CEntity::DumpSync_t(CTStream &strm, INDEX iExtensiveSyncCheck)
{
  ....
  if( en_pciCollisionInfo == NULL) {
    strm.FPrintF_t("Collision info NULL\n");
  } else if (en_RenderType==RT_BRUSH &&       // <=
             en_RenderType==RT_FIELDBRUSH) {  // <=
    strm.FPrintF_t("Collision info: Brush entity\n");
  } else {
  ....
  }
  ....
}

Одну переменную с именем "en_RenderType" сравнивают с двумя разными константами. Ошибка заключается в том, что используется оператор логического умножения '&&'. Переменная не может быть равна двум константам одновременно, поэтому условие всегда ложно. В этом месте следует использовать оператор '||'.

V559 Suspicious assignment inside the condition expression of 'if' operator: _strModURLSelected = "". menu.cpp 1188

CTString _strModURLSelected;

void JoinNetworkGame(void)
{
  ....
  char strModURL[256] = {0};
  _pNetwork->ga_strRequiredMod.ScanF(...., &strModURL);
  _fnmModSelected = CTString(strModName);
  _strModURLSelected = strModURL; // <=
  if (_strModURLSelected="") {    // <=
    _strModURLSelected = "http://www.croteam.com/mods/Old";
  }
  ....
}

Интересная ошибка. В этой функции выполняется некий запрос и в буфер с именем "strModURL" записывается результат (url-ссылка на "мод"). Далее этот результат сохраняется в объект с именем "_strModURLSelected" класса "CTString". Это собственная реализация класса для работы со строками. Из-за опечатки, в условии "if (_strModURLSelected="")" полученная ранее url-ссылка перетирается пустой строкой вместо сравнения с ней. Далее в дело вступает оператор приведения строки к типу const char *. В результате, в условии выполнится сравнение с нулём указателя, который хранит ссылку на пустую строку. Такой указатель всегда неравен нулю. Следовательно, условие всегда будет истинным.

Результатом этого кода будет всегда использование ссылки, которую жёстко прописали в коде. Хотя, планировали использовать эту ссылка как значение по умолчанию.

V547 Expression is always true. Probably the '&&' operator should be used here. propertycombobar.cpp 1853

CEntity *CPropertyComboBar::GetSelectedEntityPtr(void) 
{
 // obtain selected property ID ptr
 CPropertyID *ppidProperty = GetSelectedProperty();
 // if there is valid property selected
 if( (ppidProperty == NULL) || 
 (ppidProperty->pid_eptType != CEntityProperty::EPT_ENTITYPTR) ||
 (ppidProperty->pid_eptType != CEntityProperty::EPT_PARENT) )
 {
   return NULL;
 }
 ....
}

Здесь анализатор обнаружил ошибку, противоположную предыдущей. Две проверки переменной "pid_eptType" на неравенство всегда дают истину из-за использования оператора '||'. Следовательно, выход из функции выполняется всегда, независимо от значения указателя "ppidProperty" и переменной "ppidProperty->pid_eptType".

V547 Expression 'ulUsedShadowMemory >= 0' is always true. Unsigned type value is always >= 0. gfxlibrary.cpp 1693

void CGfxLibrary::ReduceShadows(void)
{
  ULONG ulUsedShadowMemory = ....;
  ....
  ulUsedShadowMemory -= sm.Uncache();  // <=
  ASSERT( ulUsedShadowMemory>=0);      // <=
  ....
}

В этом фрагменте кода выполняется небезопасный декремент переменной беззнакового типа, т.к. может возникнуть переполнение переменной "ulUsedShadowMemory". При этом рядом присутствует Assert(), который никогда не выдаст предупреждение. Очень подозрительное место, которое разработчикам следует проверить.

V704 'this != 0' expression should be avoided - this expression is always true on newer compilers, because 'this' pointer can never be NULL. entity.h 697

inline void CEntity::AddReference(void) { 
  if (this!=NULL) { // <=
    ASSERT(en_ctReferences>=0);
    en_ctReferences++; 
  }
};

В коде движка присутствует 28 сравнений 'this' с нулём. Код писался давно, но согласно современному стандарту языка C++, указатель 'this' не может быть нулевым, следовательно, компилятор может выполнить оптимизацию и удалить проверку. В более сложных условиях это может приводить к неожиданным ошибкам. С примерами можно ознакомиться в документации к диагностике.

Конкретно Visual C++ пока так себя не ведёт. Но это дело времени. Такой код более вне закона.

V547 Expression 'achrLine != ""' is always true. To compare strings you should use strcmp() function. worldeditor.cpp 2254

void CWorldEditorApp::OnConvertWorlds()
{
  ....
  char achrLine[256];                // <=
  CTFileStream fsFileList;

  // count lines in list file
  try {
    fsFileList.Open_t( fnFileList);
    while( !fsFileList.AtEOF()) {
      fsFileList.GetLine_t( achrLine, 256);
      // increase counter only for lines that are not blank
      if( achrLine != "") ctLines++; // <=
    }
    fsFileList.Close();
  }
  ....
}

Анализатор обнаружил неправильное сравнение строки с пустой строкой. Ошибка заключается в том, что проверка (achrLine != "") всегда истинна и инкремент переменной "ctLines" выполняется всегда. Хотя в комментарии написано, что это должно выполняться только для непустых строк.

Такое поведение вызвано тем, что в этом условии сравнивают два указателя: "achrLine" и указатель на временную пустую строку. Такие указатели никогда не будут равны.

Правильный код с использованием функции strcmp():

if(strcmp(achrLine, "") != 0) ctLines++;

Есть ещё два таких неправильных сравнения:

V547 Expression is always true. To compare strings you should use strcmp() function. propertycombobar.cpp 965
V547 Expression 'achrLine == ""' is always false. To compare strings you should use strcmp() function. worldeditor.cpp 2293

Разные ошибки

V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359

BOOL CDlgCreateAnimatedTexture::OnInitDialog() 
{
  ....
  // allocate 16k for script
  char achrDefaultScript[ 16384];
  // default script into edit control
  sprintf( achrDefaultScript, ....); // <=
  ....
  // add finishing part of script
  sprintf( achrDefaultScript,        // <=
           "%sANIM_END\r\nEND\r\n",  // <=
           achrDefaultScript);       // <=
  ....
}

Здесь формируют некую строку в буфере. Потом хотят получить новую строку, сохранив предыдущее значение строки, и добавить к ней ещё два слова. Вроде всё просто.

Для объяснения, почему здесь может получиться неожиданный результат, я процитирую простой пример из документации к этой диагностике:

char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);

В результате работы этого кода хочется получить строку:

N = 123, S = test

Но на практике в буфере будет сформирована строка:

N = 123, S = N = 123, S =

В подобных ситуациях аналогичный код может привести не только к выводу некорректного текста, но и к аварийному завершению программы. Код может быть исправлен, если использовать для сохранения результата новый буфер. Безопасный вариант:

char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);

Аналогично, стоит поступить и в коде Serious Engine. Хотя благодаря везению код может работать, будет безопасней использовать дополнительный буфер для формирования строки.

V579 The qsort function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. mesh.cpp 224

// optimize lod of mesh
void CMesh::OptimizeLod(MeshLOD &mLod)
{
  ....
  // sort array
  qsort(&_aiSortedIndex[0]           // <=
        ctVertices
        sizeof(&_aiSortedIndex[0]),  // <=
        qsort_CompareArray);
  ....
}

Функция qsort() принимает третьим аргументом размер элемента сортируемого массива. Очень подозрительно, что туда всегда передают размер указателя. Скорее всего кто-то скопировал первый аргумент функции в третий, но забыл стереть символ взятия адреса.

V607 Ownerless expression 'pdecDLLClass->dec_ctProperties'. entityproperties.cpp 107

void CEntity::ReadProperties_t(CTStream &istrm) // throw char *
{
  ....
  CDLLEntityClass *pdecDLLClass = en_pecClass->ec_pdecDLLClass;
  ....
  // for all saved properties
  for(INDEX iProperty=0; iProperty<ctProperties; iProperty++) {
    pdecDLLClass->dec_ctProperties;  // <=
    ....
  }
  ....
}

Непонятно, что делает выделенная строчка кода. Вернее, понятно, что как раз ничего. Поле класса никак не используется. Возможно это ошибка возникла после рефакторинга или просто строчка осталась после отладки.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 2)' is negative. layermaker.cpp 363

void CLayerMaker::SpreadShadowMaskOutwards(void)
{
  #define ADDNEIGHBOUR(du, dv)                                  \
  if ((pixLayerU+(du)>=0)                                       \
    &&(pixLayerU+(du)<pixLayerSizeU)                            \
    &&(pixLayerV+(dv)>=0)                                       \
    &&(pixLayerV+(dv)<pixLayerSizeV)                            \
    &&(pubPolygonMask[slOffsetMap+(du)+((dv)<<pixSizeULog2)])) {\
    ....                                                        \
    }

  ADDNEIGHBOUR(-2, -2); // <=
  ADDNEIGHBOUR(-1, -2); // <=
  ....                  // <=
}

Макрос "ADDNEIGHBOUR" объявлен в теле функции и используется подряд 28 раз. В этот макрос передают отрицательные числа, где выполняется их сдвиг. Согласно современным стандартам языка C и C++, сдвиг отрицательного числа приводит к неопределённому поведению.

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sessionstate.cpp 1191

void CSessionState::ProcessGameStream(void)
{
  ....
  if (res==CNetworkStream::R_OK) {
    ....
  } if (res==CNetworkStream::R_BLOCKNOTRECEIVEDYET) { // <=
    ....
  } else if (res==CNetworkStream::R_BLOCKMISSING) {
    ....
  }
  ....
}

Глядя на оформление кода можно предположить, что в каскаде условий действительно пропущено ключевое слово 'else'.

Ещё такое место:

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. terrain.cpp 759

V595 The 'pAD' pointer was utilized before it was verified against nullptr. Check lines: 791, 796. anim.cpp 791

void CAnimObject::SetData(CAnimData *pAD) {
  // mark new data as referenced once more
  pAD->AddReference();                      // <=
  // mark old data as referenced once less
  ao_AnimData->RemReference();
  // remember new data
  ao_AnimData = pAD;
  if( pAD != NULL) StartAnim( 0);           // <=
  // mark that something has changed
  MarkChanged();
}

В завершение статьи хочу привести пример ошибки с потенциальным разыменованием нулевого указателя. Прочитав предупреждение анализатора, в этой небольшой функции легко заметить, как опасно используется указатель "pAD". Почти сразу после вызова "pAD->AddReference()" выполняется проверка "pAD != NULL", что говорит о возможной передаче нулевого указателя в эту функцию.

Весь список найденных опасных мест с указателями:

V595 The '_ppenPlayer' pointer was utilized before it was verified against nullptr. Check lines: 851, 854. computer.cpp 851
V595 The '_meshEditOperations' pointer was utilized before it was verified against nullptr. Check lines: 416, 418. modelermeshexporter.cpp 416
V595 The '_fpOutput' pointer was utilized before it was verified against nullptr. Check lines: 654, 664. modelermeshexporter.cpp 654
V595 The '_appPolPnts' pointer was utilized before it was verified against nullptr. Check lines: 647, 676. modelermeshexporter.cpp 647
V595 The 'pModelerView' pointer was utilized before it was verified against nullptr. Check lines: 60, 63. dlginfopgglobal.cpp 60
V595 The 'pNewWT' pointer was utilized before it was verified against nullptr. Check lines: 736, 744. modeler.cpp 736
V595 The 'pvpViewPort' pointer was utilized before it was verified against nullptr. Check lines: 1327, 1353. serioussam.cpp 1327
V595 The 'pDC' pointer was utilized before it was verified against nullptr. Check lines: 138, 139. tooltipwnd.cpp 138
V595 The 'm_pDrawPort' pointer was utilized before it was verified against nullptr. Check lines: 94, 97. wndanimationframes.cpp 94
V595 The 'penBrush' pointer was utilized before it was verified against nullptr. Check lines: 9033, 9035. worldeditorview.cpp 9033

Заключение

Проверка игрового движка Serious Engine 1 v.1.10 показала, что ошибки в коде проектов могут жить очень долго и даже отмечать юбилей! В статью вошли только некоторые самые интересные примеры из отчёта анализатора. Много предупреждений я привёл просто списком. Но весь отчёт содержит довольно много предупреждений для такого маленького проекта. У компании Croteam есть более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4. Боюсь представить, сколько опасного кода могло перекочевать в новые серии движка. Я надеюсь, что после прочтения статьи, разработчики воспользуются статическим анализатором PVS-Studio в своих проектах и будут радовать пользователей качественными играми. Ведь анализатор легко скачать, легко запустить в Visual Studio, а для любых других сборочных систем есть утилита Standalone.

#Cpp