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

03 Дек 2014

Теги:

#Cpp #GameDev

Техосмотр движка Spring RTS

03 Дек 2014

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

Опечатки
Copy-Paste
Форматирование
Указатели
Операции с памятью
Неопределённое поведение
Заключение

Spring RTS — это движок для игр в жанре "стратегия реального времени". Изначально Spring был написан для повторения популярной в 90\00-е игры Total Annihilation. В дальнейшем появилось много других красивых и интересных стратегий на этом движке, в том числе и коммерческих. Игры под него кроссплатформенные и представляют из себя трёхмерные стратегии реального времени с огромными картами и большим количеством боевых и строительных юнитов. У игр возникают проблемы со стабильностью. Попробуем взглянуть на исходники (благо, проект открытый).

Официальный сайт.

Исходный код.

Являясь открытым проектом, Spring RTS включает в себя ряд открытых библиотек, которые тоже могут содержать ошибки, и, в конечном итоге, они тоже становятся частью движка/игр. Некоторые сообщения будут из библиотек, которые поставляются вместе с движком. Особенно много предупреждений было в Assimp (Open Asset Import Library).

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

Опечатки

V501 There are identical sub-expressions 'aha->mNumWeights != oha->mNumWeights' to the left and to the right of the '||' operator. assimp findinstancesprocess.cpp 87

struct aiBone
{
  C_STRUCT aiString mName;
  unsigned int mNumWeights;
  C_STRUCT aiVertexWeight* mWeights;
  C_STRUCT aiMatrix4x4 mOffsetMatrix;
  ....
};

bool CompareBones(const aiMesh* orig, const aiMesh* inst)
{
  ....
  aiBone* aha = orig->mBones[i];
  aiBone* oha = inst->mBones[i];
  if (aha->mNumWeights   != oha->mNumWeights   ||  // <=
      aha->mOffsetMatrix != oha->mOffsetMatrix ||
      aha->mNumWeights   != oha->mNumWeights) {    // <=
      return false;
  }
  ....
}

Два одинаковых условных выражения. Возможно, в одном из случаев необходимо сравнивать поле 'mName' или 'mWeights' структуры aiBone.

V501 There are identical sub-expressions to the left and to the right of the '||' operator: 0 == pArchive || 0 == pArchive assimp q3bspfileimporter.cpp 631

bool Q3BSPFileImporter::importTextureFromArchive(
  const Q3BSP::Q3BSPModel *pModel,
  Q3BSP::Q3BSPZipArchive *pArchive, aiScene* /*pScene*/,
  aiMaterial *pMatHelper, int textureId )
{
  ....
  if( NULL == pArchive || NULL == pArchive || NULL == pMatHelper)
  {
    return false;
  }

  if ( textureId < 0 ||
    textureId >= static_cast<int>( pModel->m_Textures.size() ) )
  {
    return false;
  }
  ....
}

Ещё две одинаковые проверки. Скорее всего, не хватает проверки указателя 'pModel', так как здесь проверяются переданные в функцию указатели.

V560 A part of conditional expression is always true: 0xFFFF. engine-dedicated%engine-headless%engine-legacy%unitsync cpuid.cpp 144

void CpuId::getMasksIntelLeaf11Enumerate()
{
  ....
  if ((ebx && 0xFFFF) == 0)        // <=
    return;

  if (((ecx >> 8) & 0xFF) == 1) {
    LOG_L(L_DEBUG,"[CpuId] SMT level found");
    shiftCore = eax & 0xf;
  } else {
    LOG_L(L_DEBUG,"[CpuId] No SMT level supported");
  }
  ....
}

Вместо оператора '&&' должен был быть использован оператор '&'.

V530 The return value of function 'size' is required to be utilized. assimp b3dimporter.cpp 536

void B3DImporter::ReadBB3D( aiScene *scene ){
  _textures.clear();
  _materials.size();     // <=
  _vertices.clear();
  _meshes.clear();
  ....
}

Звать функцию size() без использования возвращаемого значения явно не имеет смысла. Скорее всего, здесь, как и везде, необходимо вызвать функцию clear().

V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. engineSim weapon.cpp 597

bool CWeapon::AttackUnit(CUnit* newTargetUnit, bool isUserTarget)
{
  if ((!isUserTarget && weaponDef->noAutoTarget)) {
    return false;
  }
  ....
}

Всё условное выражение взято в двойные скобки. Возможно, оператор отрицания как раз должен применяться ко всему выражению, а не только к переменной 'isUserTarget'. Например, так:

if (!(isUserTarget && weaponDef->noAutoTarget)) {
  return false;
}

V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp plyparser.cpp 185

PLY::ESemantic PLY::Property::ParseSemantic(....)
{
  ....
  else if (TokenMatch(pCur,"specular_alpha",14))
  {
    eOut = PLY::EST_SpecularAlpha;
  }
  else if (TokenMatch(pCur,"opacity",7))
  {
    eOut = PLY::EST_Opacity;
  }
  else if (TokenMatch(pCur,"specular_power",6))
  {
    eOut = PLY::EST_PhongPower;
  }
  ....
}

В функцию 'TokenMatch' передаётся строка и её длина, которая в одном месте явно не совпадает.

Ещё два таких места:

V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp aseparser.cpp 1561
V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp aseparser.cpp 1527

Copy-Paste

Следующие подозрительные места я отделил от простых опечаток, которые возникают при наборе текста. Дальше хорошо заметно, как "успешно" был поправлен продублированный код.

V519 The 'pTexture->achFormatHint[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 663, 664. assimp q3bspfileimporter.cpp 664

bool Q3BSPFileImporter::importTextureFromArchive(....)
{
  ....
  pTexture->achFormatHint[ 0 ] = ext[ 0 ];
  pTexture->achFormatHint[ 1 ] = ext[ 1 ];
  pTexture->achFormatHint[ 2 ] = ext[ 2 ];
  pTexture->achFormatHint[ 2 ] = '\0';
  ....
}

Случайно был обнулён последний значащий символ. Про подобные места даже есть отдельная статья: Эффект последней строки.

V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: player.cpuUsage. engine-dedicated%engine-headless%engine-legacy gameserver.cpp 902

void CGameServer::LagProtection()
{
  ....
  const float playerCpuUsage =
    player.isLocal ? player.cpuUsage : player.cpuUsage; // <=
  ....
}

Я думаю, условные конструкции не используют, если выбора нет. Можно предположить, что здесь одну переменную забыли поправить.

V524 It is odd that the body of '-' function is fully equivalent to the body of '+' function. assimp%engine-headless%engine-legacy types.h 183

/** Component-wise addition */
aiColor3D operator+(const aiColor3D& c) const {
  return aiColor3D(r+c.r,g+c.g,b+c.b);
}

/** Component-wise subtraction */
aiColor3D operator-(const aiColor3D& c) const {
  return aiColor3D(r+c.r,g+c.g,b+c.b);
}

Функции сложения и вычитания подозрительно одинаково реализованы. Скорее всего, для вычитания забыли поправить знак.

V524 It is odd that the body of '>' function is fully equivalent to the body of '<' function. assimp 3dshelper.h 470

bool operator < (const aiFloatKey& o) const
  {return mTime < o.mTime;}

bool operator > (const aiFloatKey& o) const
  {return mTime < o.mTime;}

Противоположные по смыслу операторы сравнения выглядят ещё более подозрительно, когда реализованы одинаковым образом.

Форматирование

В этом разделе пойдёт речь о подозрительных местах, связанных с форматированием кода. Реальные ли здесь ошибки - виднее авторам, но стиль программирования явно не самый лучший.

V628 It's possible that the line was commented out improperly, thus altering the program's operation logics. assimp colladaparser.cpp 2281

void ColladaParser::ReadSceneLibrary()
{
  ....
  else if( mReader->getNodeType() == irr::io::EXN_ELEMENT_END)
  {
    if( strcmp( mReader->getNodeName(), "....") == 0)
      //ThrowException( "Expected end of \"....\" element.");

    break;
  }
  ....
}

Раньше в этом месте всегда звался 'break', теперь выход из цикла выполняется только по условию. Возможно, следовало закомментировать и само условие.

V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. sound oggstream.cpp 256

bool COggStream::UpdateBuffers()
{
  ....
  active = DecodeStream(buffer);
  if (active)
    alSourceQueueBuffers(source, 1, &buffer); CheckError("....");
  ....
}

Функция CheckError() не является частью условия, но написано, как будто это так.

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. streflop s_atanf.cpp 90

Simple __atanf(Simple x)
{
  ....
  ix = hx&0x7fffffff;
  if(ix>=0x50800000) {  /* if |x| >= 2^34 */
      if(ix>0x7f800000)
    return x+x;    /* NaN */
      if(hx>0) return  atanhi[3]+atanlo[3];
      else     return -atanhi[3]-atanlo[3];
  } if (ix < 0x3ee00000) {  /* |x| < 0.4375f */            // <=
      if (ix < 0x31000000) {  /* |x| < 2^-29 */
    if(huge+x>one) return x;  /* raise inexact */
      }
      id = -1;
  } else {
    ....
  }
  ....
}

Оператор if расположен на той же строке, что и закрывающаяся скобка от предыдущего if. Возможно, в этом месте пропущено ключевое слово 'else', и программа работает не так, как ожидал программист.

V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. AAI aaibrain.cpp 1138

void AAIBrain::BuildUnitOfMovementType(....)
{
  ....
  if(ai->Getbt()->units_static[unit].cost < ....)
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 3, urgent))
    {
      ai->Getbt()->units_dynamic[unit].requested += 3;
      ai->Getut()->UnitRequested(....);
    }
  }
  else if(ai->Getbt()->units_static[unit].cost < ....)
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 2, urgent))
      ai->Getbt()->units_dynamic[unit].requested += 2;
      ai->Getut()->UnitRequested(....);
  }
  else
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 1, urgent))
      ai->Getbt()->units_dynamic[unit].requested += 1;
      ai->Getut()->UnitRequested(....);
  }
  ....
}

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

Указатели

V571 Recurring check. The 'if (0 == MatFilePtr)' condition was already verified in line 140. assimp ogrematerial.cpp 143

aiMaterial* OgreImporter::LoadMaterial(const std::string MaterialName)
  const
{
  ....
  MatFilePtr=m_CurrentIOHandler->Open(MaterialFileName);
  if(NULL==MatFilePtr)
  {
    //try the default mat Library
    if(NULL==MatFilePtr)
    {
      MatFilePtr=m_CurrentIOHandler->Open(m_MaterialLibFilename);
      ....
    }
  }
  ....
}

Повторяющиеся проверки не являются ошибкой, но в проекте много мест, где проверок действительно не хватает.

V595 The 'model->GetRootPiece()' pointer was utilized before it was verified against nullptr. Check lines: 236, 238. engine-headless%engine-legacy imodelparser.cpp 236

S3DModel* C3DModelLoader::Load3DModel(std::string modelName)
{
  ....
  model->GetRootPiece()->SetCollisionVolume(                // <=
    new CollisionVolume("box", -UpVector, ZeroVector));

  if (model->GetRootPiece() != NULL) {                      // <=
    CreateLists(model->GetRootPiece());
  }
  ....
}

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

Похожие места:

V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 1559, 1564. assimp unzip.c 1559
V595 The 'facCAI' pointer was utilized before it was verified against nullptr. Check lines: 1059, 1064. engineSim commandai.cpp 1059
V595 The 'projectileDrawer' pointer was utilized before it was verified against nullptr. Check lines: 170, 176. engineSim shieldprojectile.cpp 170
V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 2068, 2073. minizip unzip.c 2068

V576 Incorrect format. Consider checking the fifth actual argument of the 'sprintf' function. To print the value of pointer the '%p' should be used. engine-dedicated%engine-headless%engine-legacy seh.cpp 45

void __cdecl
se_translator_function(unsigned int err,
                       struct _EXCEPTION_POINTERS* ep)
{
  char buf[128];
  sprintf(buf,"%s(0x%08x) at 0x%08x",ExceptionName(err), // <=
    errep->ExceptionRecord->ExceptionAddress);           // <=
  CrashHandler::ExceptionHandler(ep);
  throw std::exception(buf);
}

Для печати указателя необходимо использовать спецификатор %p. Текущий код будет корректно работать, пока размер указателя совпадает с размером 'int'.

V643 Unusual pointer arithmetic: ".." + io->getOsSeparator(). The value of the 'char' type is being added to the string pointer. assimp lwsloader.cpp 467

std::string LWSImporter::FindLWOFile(const std::string& in)
{
  ....
  std::string test = ".." + io->getOsSeparator() + tmp;   // <=
  if (io->Exists(test))
    return test;

  test = ".." + io->getOsSeparator() + test;              // <=
  if (io->Exists(test)) {
    return test;
  }
  ....
}

Ожидалось, что будет получена строка "..\tmp", однако в данном случае к указателю на строку ".." будет прибавлено целое значение. Это гарантированно приведет к выходу за границу строкового литерала. Для предотвращения такой ситуации следует избегать подобных арифметических операций со строковыми и символьными переменными.

Корректный вариант кода:

std::string test = std::string("..") + io->getOsSeparator() + tmp;

Операции с памятью

V512 A call of the 'memset' function will lead to underflow of the buffer 'area'. RAI gterrainmap.h 84

#define MAP_AREA_LIST_SIZE 50
struct TerrainMapMobileType
{
  TerrainMapMobileType()
  {
    ....
    memset(area,0,MAP_AREA_LIST_SIZE);       // <=
  };
  
  TerrainMapArea *area[MAP_AREA_LIST_SIZE];  // <=
  ....
};

Неполное обнуление памяти. Объявляется массив из 50 указателей, а обнуляется 50 байт. Размер же массива составляет 50*sizeof(pointer) байт.

Аналогичные места:

V512 A call of the 'memset' function will lead to underflow of the buffer 'BQ'. RAI builder.cpp 67
V512 A call of the 'memset' function will lead to underflow of the buffer 'SL'. RAI unitmanager.cpp 28
V512 A call of the 'memset' function will lead to underflow of the buffer 'Group'. RAI unitmanager.cpp 29
V512 A call of the 'memset' function will lead to underflow of the buffer 'eventList'. RAI rai.cpp 77

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dest' is lost. Consider assigning realloc() to a temporary pointer. assimp blenderloader.cpp 217

void BlenderImporter::InternReadFile( const std::string& pFile, 
  aiScene* pScene, IOSystem* pIOHandler)
{
  ....
  dest = reinterpret_cast<Bytef*>( realloc(dest,total) );
  memcpy(dest + total - have,block,have);
  ....
}

В случае, если не удастся изменить размер блока памяти, функция realloc() вернёт нулевой указатель, а указатель на старую область памяти будет потерян. Необходимо сохранять указатель в буферную переменную и делать соответствующие проверки.

Похожее место:

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dest' is lost. Consider assigning realloc() to a temporary pointer. assimp xglloader.cpp 181

Неопределённое поведение

V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. engine-dedicated%engine-headless%engine-legacy%unitsync cpuid.cpp 176

void CpuId::getMasksIntelLeaf11()
{
  getMasksIntelLeaf11Enumerate();

  // We determined the shifts now compute the masks
  maskVirtual = ~((-1) << shiftCore);
  maskCore = (~((-1) << shiftPackage)) ^ maskVirtual;
  maskPackage = (-1) << shiftPackage;
}

Согласно стандарту языка C++11, сдвиг отрицательного числа приводит к неопределённому поведению.

Заключение

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

Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач.

#Cpp #GameDev