Довгоочікувана перевірка CryEngine V

У травні 2016 року німецька компанія Crytek вирішила опублікувати на Github вихідний код ігрового движка CryEngine V. Ігровий движок написаний на мові C++ і відразу привернув увагу як спільноти open-source розробників, так і команду розробників статичного аналізатора PVS-Studio, виконує перевірку якості коду відкритих проектів. На CryEngine різних версій зроблено багато відмінних ігор від різних ігрових студій, і тепер движок став доступний ще більшого числа розробників. Стаття містить огляд помилок, виявлених з допомогою статичного аналізатора коду.

Введення
CryEngine — ігровий движок, створений німецькою компанією Crytek в 2002 році і спочатку використовуваний у шутері від першої особи Far Cry. На CryEngine різних версій зроблено багато відмінних ігор від різних ігрових студій, які ліцензували движок: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve і багато інших. В березні 2016 року компанія Crytek анонсувала вихід свого нового движка CryEngine V і незабаром опублікувала початковий код на Github.

Для перевірки відкритого вихідного коду використовувався статичний аналізатор PVS-Studio версії 6.05. Це інструмент для виявлення помилок у вихідному коді програм, написаних на мовах С, C++ і C#. Єдиний вірний спосіб використання методології статичного аналізу — регулярна перевірка коду на комп'ютерах розробників build-сервері. Але для демонстрації можливостей аналізатора PVS-Studio ми виконуємо разові перевірки open-source проектів і пишемо оглядові статті з описом виявлених помилок. Втім, якщо проект здався нам цікавим, на пришестя одного-двох років ми можемо знову перевірити його. По суті такі повторні перевірки нічим не відрізняються від разових, так як в коді накопичується багато змін.

Для перевірки вибираються як просто відомі і популярні проекти, так і запропоновані читачами по електронній пошті. Тому серед ігрових движків CryEngine V далеко не перший. Були перевірені наступні механізми:Перевірка Xenko EngineТакож одного разу був перевірений CryEngine 3 SDK.

Особливу увагу хочу приділити перевірці ігрового движка Unreal Engine 4. На прикладі цього проекту ми змогли в подробицях розповісти про правильному застосуванні методології статичного аналізу на реальному проекті: від впровадження аналізатора в проект до відома кількості попереджень до нуля з подальшим контролем за появою помилок на новому коді. Наша робота з проектом Unreal Engine 4 вилилася у співпрацю з компанією Epic Games, в рамках якого команда PVS-Studio виправила всі попередження аналізатора у вихідному коді движка, і була написана спільна стаття про виконану роботу, яка опублікована Unreal Engine Blog посилання на російський переклад). Також компанія Epic Games придбала ліцензію PVS-Studio для самостійного контролю якості коду. До аналогічного співпраці ми готові і з компанією Crytek.

Структура звіту аналізатора
У цій статті я хочу відповісти на кілька поширених питань, пов'язаних з кількістю попереджень та помилкових спрацьовувань. Наприклад, — «Скільки відсотків становлять помилкові спрацьовування?» або «Чому в такому великому проекті так мало помилок?».

Для початку, всі попередження аналізатора PVS-Studio діляться на три рівні важливості: High, Medium Low. Так High рівень містить високо критичні попередження, з найбільшою ймовірністю є реальними помилками, а Low рівень — попередження з низькою критичністю, якого попередження з дуже високою ймовірністю хибно-позитивного спрацювання. Варто пам'ятати, що конкретний код помилки не обов'язково прив'язує її до певного рівня важливості, а розподіл повідомлень за групами сильно залежить від контексту, в якому вони були згенеровані.

У проекті CryEngine V попередження аналізатора загального призначення (General Analysis) розподілені за рівнями важливості наступним чином:
  • High: 576 попереджень;
  • Medium: 814 попереджень,
  • Low: 2942 попередження.
На малюнку 1 зображено розподіл попереджень за рівнями важливості на круговій діаграмі.



Малюнок 1 — Розподіл попереджень за рівнями важливості в процентному відношенні

У статті неможливо вмістити опис всіх попереджень і показати відповідні фрагменти коду. Зазвичай стаття містить 10-40 прикладів помилок з описом. Деякі підозрілі місця коду наводяться просто списком. Більшість попереджень залишаються нерозглянутими. У кращому випадку, після повідомлення розробників, вони запитують повний звіт аналізатора для детального вивчення. Гірка правда полягає в тому, що в більшості перевірених нами проектів, матеріалу для статті більш ніж достатньо після перегляду тільки попереджень рівня High. І ігровий движок CryEngine V — не виняток. На малюнку 2 представлена структура попереджень рівня High.



Малюнок 2 — Опис попереджень High рівня

Тепер більш докладно про секторах представленої діаграми:
  • Described in the article (6%) — повідомлення аналізатора, наведені в статті з фрагментами коду і описом.
  • Presented list (46%) — повідомлення аналізатора, наведені в статті списком. Такі попередження дуже схожі на який-небудь випадок, який вже докладно описаний у статті, тому просто вказується інформація про попередження.
  • False Positive (8%) — деякий відсоток помилкових спрацьовувань був виписаний для доопрацювання аналізатора в майбутньому.
  • Skipped (40%) — всі інші повідомлення аналізатора. У них входять попередження, які не вдалося вмістити в статті, не є критичними, не дуже цікаві, або зустрілися на ділянках коду, в яких складно розібратися людині не з команди розробників перевіряється проекту. Як показала робота з Unreal Engine 4, на практиці такий код «пахне» і всі попередження все одно виправляються.
Результати перевірки
Прикрий copy-paste


V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z — q2.v.z entitynode.cpp 93
bool
CompareRotation(const Quat& q1, const Quat& q2, float epsilon)
{
return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon)
&& (fabs_tpl(q1.v.y - q2.v.y) <= epsilon)
&& (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <=
&& (fabs_tpl(q1.w - q2.w) <= epsilon);
}

Помилка в одній цифрі, мабуть, одна з найбільш образливих помилок, яку може допустити програміст. У цій функції аналізатор виявив підозріле вираз (q2.v.z — q2.v.z), в якому з великою ймовірністю переплутали змінні q1 q2.

V501 There are identical sub-expressions '(m_eTFSrc == eTF_BC6UH)' to the left and to the right of the '||' operator. texturestreaming.cpp 919
//! Texture formats.
enum ETEX_Format : uint8
{
....
eTF_BC4U, //!< 3Dc+.
eTF_BC4S,
eTF_BC5U, //!< 3Dc.
eTF_BC5S,
eTF_BC6UH,
eTF_BC6SH,
eTF_BC7,
eTF_R9G9B9E5,
....
};

bool CTexture::StreamPrepare(CImageFile* pIM)
{
....
if ((m_eTFSrc == eTF_R9G9B9E5) ||
(m_eTFSrc == eTF_BC6UH)|| // <=
(m_eTFSrc == eTF_BC6UH)) // <=
{
m_cMinColor /= m_cMaxColor.a;
m_cMaxColor /= m_cMaxColor.a;
}
....
}

Інший тип помилок пов'язаний з копіюванням констант. У даному випадку змінна m_eTFSrc два рази порівнюється з константою eTF_BC6UH, на місці якої повинна бути будь-яка інша, наприклад, відрізняє лише однією літерою — константа eTF_BC6SH.

Ще два схожих місця в проекті:
  • V501 There are identical sub-expressions '(td.m_eTF == eTF_BC6UH)' to the left and to the right of the '||' operator. texture.cpp 1214
  • V501 There are identical sub-expressions 'geom_colltype_solid' to the left and to the right of the '|' operator. attachmentmanager.cpp 1004
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266
int SD3DShader::Release(EHWShaderClass eSHClass, int nSize)
{
....
if (eSHClass == eHWSC_Pixel)
return ((ID3D11PixelShader*)pHandle)->Release();
else if (eSHClass == eHWSC_Vertex)
return ((ID3D11VertexShader*)pHandle)->Release();
else if (eSHClass == eHWSC_Geometry) // <=
return ((ID3D11GeometryShader*)pHandle)->Release(); // <=
else if (eSHClass == eHWSC_Geometry) // <=
return ((ID3D11GeometryShader*)pHandle)->Release(); // <=
else if (eSHClass == eHWSC_Hull)
return ((ID3D11HullShader*)pHandle)->Release();
else if (eSHClass == eHWSC_Compute)
return ((ID3D11ComputeShader*)pHandle)->Release();
else if (eSHClass == eHWSC_Domain)
return ((ID3D11DomainShader*)pHandle)->Release()
....
}

Ось приклад ледачого копіювання каскаду умовних операторів, в одному з випадків якого забули внести зміни в код.

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970
void CEnvironmentalWeapon::UpdateDebugOutput() const
{
....
const char* attackStateName = "Ні";
if(m_currentAttackState & // <=
EAttackStateType_EnactingPrimaryattack) // <=
{
attackStateName = "Primary Attack";
}
else if(m_currentAttackState & // <=
EAttackStateType_EnactingPrimaryattack) // <=
{
attackStateName = "Charged Throw";
}
....
}

У попередньому коді була хоч якась ймовірність того, що зайве умова — це результат занадто великої кількості копій коду і одна перевірка залишилася просто зайвою. У цьому фрагменті коду через ідентичних умовних виразів мінлива attackStateName ніколи не прийме значення «Charged Throw».

V519 The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266
void CCryDXGLDeviceContext::
OMGetBlendState (...., FLOAT BlendFactor[4], ....)
{
CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
if ((*ppBlendState) != NULL)
(*ppBlendState)->AddRef();
BlendFactor[0] = m_auBlendFactor[0];
BlendFactor[1] = m_auBlendFactor[1];
BlendFactor[2] = m_auBlendFactor[2]; // <=
BlendFactor[2] = m_auBlendFactor[3]; // <=
*pSampleMask = m_uSampleMask;
}

У коді проекту виявилася така функція, в якій з-за помилки в індексі пропущено заповнення елемента з індексом три: BlendFactor[3]. Це місце було б просто одним з цікавих місць з помилкою, якби аналізатор не виявив ще 2 фрагмента, куди скопіювали код з допущеною помилкою:

V519 The 'm_auBlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905
void CCryDXGLDeviceContext::
OMSetBlendState (....const FLOAT BlendFactor[4], ....)
{
....
m_uSampleMask = SampleMask;
if (BlendFactor == NULL)
{
m_auBlendFactor[0] = 1.0 f;
m_auBlendFactor[1] = 1.0 f;
m_auBlendFactor[2] = 1.0 f; // <=
m_auBlendFactor[2] = 1.0 f; // <=
}
else
{
m_auBlendFactor[0] = BlendFactor[0];
m_auBlendFactor[1] = BlendFactor[1];
m_auBlendFactor[2] = BlendFactor[2]; // <=
m_auBlendFactor[2] = BlendFactor[3]; // <=
}

m_pContext->SetBlendColor(m_auBlendFactor[0],
m_auBlendFactor[1],
m_auBlendFactor[2],
m_auBlendFactor[3]);
m_pContext->SetSampleMask(m_uSampleMask);
....
}

Ось те саме місце, де продовжують пропускати заповнення елемента з індексом '3'. На мить я навіть подумав, що в цьому був якийсь сенс, але ця думка мене швидко покинула, коли наприкінці функції я побачив звернення до всіх чотирьох елементів масиву m_auBlendFactor. Схоже у файлі ccrydxgldevicecontext.cpp просто зробили кілька копій коду з помилкою.

V523 The 'then' statement is equivalent to the 'else' statement. d3dshadows.cpp 1410
void CD3D9Renderer::ConfigShadowTexgen (....)
{
....
if ((pFr->m_Flags & DLF_DIRECTIONAL) ||
(!(pFr->bUseHWShadowMap) && !(pFr->bHWPCFCompare)))
{
//linearized shadows are used for any kind of directional
//lights and for non-hw point lights
m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
}
else
{
//hw point lights sources have non-linear depth for now
m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist);
}
....
}

На закінчення розділу про copy-paste наводжу опис ще однією цікавою помилки. Незалежно від результату виразу, значення m_cEF.m_TempVecs[2][Num] завжди обчислюється по формулі. Судячи із сусіднього кодом цього фрагмента, тут немає помилки в індексі, в цьому умови хочуть заповнити саме елемент з індексом '2'. А ось формулу розрахунку, швидше за все, хотіли використовувати різну, але після копіювання рядка забули змінити код.

Проблеми з ініціалізацією


V546 Member of a class is initialized by itself: 'eConfigMax(eConfigMax)'. particleparams.h 1013
ParticleParams() :
....
fSphericalApproximation(1.f),
fVolumeThickness(1.0 f),
fSoundFXParam(1.f),
eConfigMax(eConfigMax.VeryHigh), // <=
fFadeAtViewCosAngle(0.f)
{}

Аналізатор виявив можливу похибку, коли поле класу ініціалізується з використанням власного значення.

V603 The object was created but it is not being used. If you wish to call constructor, 'this->SRenderingPassInfo::SRenderingPassInfo (....)' should be used. i3dengine.h 2589
SRenderingPassInfo()
: pShadowGenMask(NULL)
, nShadowSide(0)
, nShadowLod(0)
, nShadowFrustumId(0)
, m_bAuxWindow(0)
, m_nRenderStackLevel(0)
, m_eShadowMapRendering(static_cast<uint8>(SHADOW_MAP_NONE))
, m_bCameraUnderWater(0)
, m_nRenderingFlags(0)
, m_fZoomFactor(0.0 f)
, m_pCamera(NULL)
, m_nZoomInProgress(0)
, m_nZoomMode(0)
, m_pJobState(nullptr)
{
threadID nThreadID = 0;
gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID);
m_nThreadID = static_cast<uint8>(nThreadID);
m_nRenderFrameID = gEnv->pRenderer->GetFrameID();
m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false);
}

SRenderingPassInfo(threadID id)
{
SRenderingPassInfo(); // <=
SetThreadID(id);
}

Тут аналізатор виявив неправильне використання конструктора. Програміст, певно, подумав, що виклик конструктора без параметрів всередині іншого конструктора виконає ініціалізацію полів класу, але це не так.

У цьому коді відбудеться наступне: буде створений новий неименованный об'єкт типу SRenderingPassInfo і тут же зруйнований. В результаті поля класу залишаються неинициализированными. Одним із способів виправлення помилки буде написання окремої функції ініціалізації і виклик її з різних конструкторів.

V688 The 'm_cNewGeomMML' local variable possesses the same name as one of the class members, which can result in a confusion. terrain_node.cpp 344
void CTerrainNode::Init (....)
{
....
m_nOriginX = m_nOriginY = 0; // sector origin
m_nLastTimeUsed = 0; // basically last time rendered

uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min ....

m_pLeafData = 0;

m_nTreeLevel = 0;
....
}

Аналізатор виявив збігу імені локальної змінної cNewGeomMML з полем класу. Часто такий код не є помилкою, але тут це виглядає дуже підозріло на тлі ініціалізації інших полів класу.

V575 The 'memset' function processes '0' elements. Inspect the third argument. crythreadutil_win32.h 294
void EnableFloatExceptions (....)
{
....
CONTEXT ctx;
memset(&ctx, sizeof(ctx), 0); // <=
....
}

З допомогою аналізатора була знайдена дуже цікава помилка. При виклику функції memset() переплутали передані аргументи, в результаті чого функцію звуть для заповнення 0 байт пам'яті. Ось як виглядає прототип функції:
void * memset ( void * ptr, int value, size_t num );

Третім аргументом повинен передаватися розмір буфера, а другим — значення, яким необхідно заповнити буфер.

Виправлений варіант:
void EnableFloatExceptions (....)
{
....
CONTEXT ctx;
memset(&ctx, 0, sizeof(ctx));
....
}

V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62
void CBuffer::Execute()
{
....
QuatT * pJointsTemp = static_cast<QuatT*>(
alloca(m_state.m_jointCount * sizeof(QuatT)));
....
}

У коді проекту зустрічаються місця, де за допомогою функції alloca() виділяють пам'ять для масиву об'єктів. У наведеному коді, при такому способі виділення пам'яті у об'єктів класу QuatT не викликатиме ні конструктор, ні деструктор. Такий код може призвести до роботи з неинициализированными змінними та інших помилок.

Весь список підозрілих місць:
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016
  • V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8 f. posealignerc3.cpp 330
ILINE bool InitializePoseAlignerPinger (....)
{
....
chainDesc.offsetMin = Vec3(0.0 f, 0.0 f, bIsMP ? -1.8 f : -1.8 f);
chainDesc.offsetMax = Vec3(0.0 f, 0.0 f, bIsMP ? +0.75 f : +1.f);
....
}

У коді проекту зустрічаються випадки, коли тернарний оператор ?: повертає одне і те ж значення. Можливо, в попередньому випадку так написали для краси коду, але навіщо так зробили в цьому фрагменті?
float predictDelta = inputSpeed < 0.0 f ? 0.1 f : 0.1 f; // <=
float dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;

Всі такі місця, знайдені в проекті:
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8 f. posealignerc3.cpp 313
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151
  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0.1 f. vehiclemovementstdboat.cpp 720
V570 The 'runtimeData.entityId' variable is assigned to itself. behaviortreenodes_ai.cpp 1771
void ExecuteEnterScript(RuntimeData& runtimeData)
{
ExecuteScript(m_enterScriptFunction, runtimeData.entityId);

runtimeData.entityId = runtimeData.entityId; // <=
runtimeData.executeExitScriptIfDestructed = true;
}

Підозріле присвоювання змінної самій собі. Розробникам варто перевірити це місце.

Пріоритети операцій


V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower than the priority '+' operator. gpuparticlefeaturespawn.cpp 79
bool HasDuration() { return m_useDuration; }

void CFeatureSpawnRate::SpawnParticles (....)
{
....
SSpawnData& spawn = pRuntime->GetSpawnData(i);
const float amount = spawn.amount;
const int spawnedBefore = int(spawn.spawned);
const float endTime = spawn.delay +
HasDuration() ? spawn.duration : fHUGE;
....
}

Схоже, в цій функції невірно виконується вимір часу. Пріоритет оператора складання вище, чому тернарного оператора :?, тому до spawn.delay спочатку додається значення 0 або 1, а в змінну endTime завжди записується значення spawn.duration або fHUGE. Це досить поширена помилка. Про цікаві паттернах помилок у пріоритетах операцій, знайдених в базі помилок PVS-Studio, я розповів у статті: Логічні вирази в C/C++. Як помиляються професіонали.

V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. model.cpp 336
enum joint_flags
{
angle0_locked = 1,
....
};

bool CDefaultSkeleton::SetupPhysicalProxies (....)
{
....
for (int j = 0; .... ; j++)
{
// lock axes with 0 limits range
m_arrModelJoints[i]....flags | = (....) * angle0_locked << j;
}
....
}

Ще одна дуже цікава помилка, пов'язана з пріоритетом операцій множення та побітового зсуву. В останнього пріоритет виконання нижче, тому весь вираз на кожній ітерації множиться на одиницю (константа angle0_locked дорівнює одиниці), що виглядає дуже дивно.

Швидше за все хотіли написати так:
m_arrModelJoints[i]....flags|= (....) * (angle0_locked << j);

Список з 35 підозрілих місць з пріоритетом операторів зсуву наведено у файлі CryEngine5_V634.txt.

Невизначений поведінка
Невизначений поведінка — властивість деяких мов програмування в певних ситуаціях видавати результат, що залежить від випадкових факторів на кшталт стану пам'яті або спрацював переривання. Іншими словами, специфікація не визначає поведінку мови в будь-яких можливих ситуаціях. Допускати таку ситуацію в програмі вважається помилкою, навіть якщо на деякому компіляторі програма успішно виконується, вона не буде кроссплатформної і може відмовити на іншій машині, в іншій ОС і навіть на інших налаштуваннях компілятора.



V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25
#ifndef physicalplaceholder_h
#define physicalplaceholder_h
#pragma once
....
const int NO_GRID_REG = -1<<14;
const int GRID_REG_PENDING = NO_GRID_REG+1;
....

Згідно сучасному стандарту C++, зсув вліво від'ємного числа призводить до невизначеної поведінки. Крім цього, в коді CryEngine V є ще кілька таких місць:
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '~(TFragSeqStorage(0))' is negative. udpdatagramsocket.cpp 757
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 324
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 350
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 617
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. tetrlattice.cpp 622
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredrender.cpp 876
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(0xF))' is negative. d3ddeferredshading.cpp 791
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(1 << 0))' is negative. d3dsprites.cpp 1038
V567 Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. operatorqueue.cpp 105
bool COperatorQueue::Prepare (....)
{
++m_current &= 1;
m_ops[m_current].clear();
return true;
}

Аналізатор виявив вираз, що призводить до невизначеної поведінки програми. Змінна неодноразово використовується між двома точками прямування, при цьому її значення змінюється. У результаті неможливо передбачити результат роботи такого виразу.

Ще підозрілі місця:
  • V567 Undefined behavior. The 'itail' variable is modified while being used twice between sequence points. trimesh.cpp 3101
  • V567 Undefined behavior. The 'ihead' variable is modified while being used twice between sequence points. trimesh.cpp 3108
  • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1194
  • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1202
  • V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1220
  • V567 Undefined behavior. The 'm_commandBufferIndex' variable is modified while being used twice between sequence points. xconsole.cpp 180
  • V567 Undefined behavior. The 'm_FrameFenceCursor' variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952
  • V567 Undefined behavior. The 'm_iNextAnimIndex' variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192
Різні помилки в умовах


V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58
bool
operator==(const SComputePipelineStateDescription& other) const
{
return 0 == memcmp(this, &other, sizeof(this)); // <=
}

В операторі рівності допустили помилку у виклику функції memcmp(), передавши в неї розмір покажчика, а не розмір об'єкта. Тепер об'єкти порівнюються з першим кільком байтам.

Виправлений варіант:
memcmp(this, &other, sizeof(*this));

На жаль, у проекті є ще три таких місця, які варто перевірити:
  • V579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286
  • V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145
  • V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34
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. livingentity.cpp 181
CLivingEntity::~CLivingEntity()
{
for(int i=0;i<m_nParts;i++) {
if (!m_parts[i].pPhysGeom | | ....)
delete[] m_parts[i].pMatMapping; m_parts[i].pMatMapping=0;
}
....
}

У коді ігрового движка я помітив дуже багато місць, де розробники пишуть по декілька операторів у сходинку. Часто це навіть не звичайні присвоювання, а цикли, умови, виклики функції, а іноді все це упереміш (малюнок 3).



Малюнок 3 — Погане форматування коду

З таким стилем програмування уникнути помилок на великому обсязі практично неможливо. Так, у розглянутому випадку планували за певних умов звільняти пам'ять з-під масиву об'єктів і обнуляти покажчик. Але з-за неправильного форматування покажчик m_parts[i].pMatMapping обнуляється на кожній ітерації циклу. Які негативні наслідки це може мати, я передбачити не можу, але код однозначно насторожує.

Ще кілька місць з підозрілим форматуванням:
  • 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. physicalworld.cpp 2449
  • 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. articulatedentity.cpp 1723
  • 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. articulatedentity.cpp 1726
V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 538, 540. statobjrend.cpp 540
bool CStatObj::RenderDebugInfo (....)
{
....
ColorB clr(0, 0, 0, 0);
if (nRenderMats == 1)
clr = ColorB(0, 0, 255, 255);
else if (nRenderMats == 2)
clr = ColorB(0, 255, 255, 255);
else if (nRenderMats == 3)
clr = ColorB(0, 255, 0, 255);
else if (nRenderMats == 4)
clr = ColorB(255, 0, 255, 255);
else if (nRenderMats == 5)
clr = ColorB(255, 255, 0, 255);
else if (nRenderMats >= 6) // <=
clr = ColorB(255, 0, 0, 255);
else if (nRenderMats >= 11) // <=
clr = ColorB(255, 255, 255, 255);
....
}

У цьому коді програміст допустив помилку, за якою колір ColorB(255, 255, 255, 255) ніколи не буде обраний. Спочатку значення nRenderMats порівнюються по порядку з числами від 1 до 5, але коли розробник перейшов до роботи з діапазоном значень, то не врахував, що значення більше 11 вже входять в діапазон більше 6, отже, остання умова ніколи не виконується.

Цей каскад умов був повністю скопійований ще в одне місце:
  • V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 338, 340. modelmesh_debugpc.cpp 340
V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 393, 399. xmlcpb_nodelivewriter.cpp 399
enum eNodeConstants
{
....
CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1, // 127
CHILDBLOCKS_MAX_DIST_FOR_16BITS = BIT(6) - 1, // 63
....
};

void CNodeLiveWriter::Compact()
{
....
if (dist <= CHILDBLOCKS_MAX_DIST_FOR_8BITS) // dist <= 127
{
uint8 byteDist = dist;
writeBuffer.AddData(&byteDist, sizeof(byteDist));
isChildBlockSaved = true;
}
else if (dist <= CHILDBLOCKS_MAX_DIST_FOR_16BITS) // dist <= 63
{
uint8 byteHigh = CHILDBLOCKS_USING_MORE_THAN_8BITS | ....);
uint8 byteLow = dist & 255;
writeBuffer.AddData(&byteHigh, sizeof(byteHigh));
writeBuffer.AddData(&byteLow, sizeof(byteLow));
isChildBlockSaved = true;
}
....
}

Схожу помилку в умові допустили і в цьому фрагменті коду, тільки тепер управління не отримує досить великий фрагмент коду. Значення констант CHILDBLOCKS_MAX_DIST_FOR_8BITS CHILDBLOCKS_MAX_DIST_FOR_16BITS мають такі значення, що друга умова ніколи не буде істинним.

V547 Expression 'pszScript[iSrcBufPos] != '==" is always true. The value range of char type: [-128, 127]. luadbg.cpp 716
bool CLUADbg::LoadFile(const char* pszFile, bool bForceReload)
{
FILE* hFile = NULL;
char* pszScript = NULL, * pszFormattedScript = NULL;
....
while (pszScript[iSrcBufPos] != '' &&
....
pszScript[iSrcBufPos] != '=' &&
pszScript[iSrcBufPos] != '==' && // <=
pszScript[iSrcBufPos] != '*' &&
pszScript[iSrcBufPos] != '+' &&
pszScript[iSrcBufPos] != '/' &&
pszScript[iSrcBufPos] != '~' &&
pszScript[iSrcBufPos] != '"')
{}
....
}

У великому умовному вираженні є підвираз, яке завжди істинно. Літерал '==' буде мати тип int і дорівнюватиме 15677. Масив pszScript складається з елементів типу char. Значення типу char не може бути одно 15677. Саме тому вираз pszScript[iSrcBufPos] != '==' завжди істинно.

V734 An excessive expression. Examine the substrings "_ddn" and "_ddna". texture.cpp 4212
void CTexture::PrepareLowResSystemCopy(byte* pTexData, ....)
{
....
// make sure we skip non diffuse textures
if (strstr(GetName(), "_ddn") // <=
|| strstr(GetName(), "_ddna") // <=
|| strstr(GetName(), "_mask")
|| strstr(GetName(), "_spec.")
|| strstr(GetName(), "_gloss")
|| strstr(GetName(), "_displ")
|| strstr(GetName(), "characters")
|| strstr(GetName(), "$")
)
return;
....
}

Функція strstr() шукає перше входження зазначеної підрядка в іншому рядку і повертає вказівник на перше входження підрядка або порожній покажчик. Першою шукається підрядок "_ddn", а потім "_ddna", це означає, що умова виконується, якщо буде знайдено більш коротка підрядок. Можливо, код працює не так, як планував програміст. Ну або принаймні вираз є надлишковим і його можна спростити, видаливши зайву перевірку.

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. goalop_crysis2.cpp 3779
void COPCrysis2FlightFireWeapons::ParseParam (....)
{
....
else if (!paused &&
(m_State == eFP_PAUSED) && // <=
(m_State != eFP_PAUSED_OVERRIDE)) // <=
....
}

Умовний вираз функції ParseParam() написано таким чином, що результат не залежить від подвираженія (m_State != eFP_PAUSED_OVERRIDE).

Розглянемо спрощений приклад:
if ( err == code1 && err != code2)
{
....
}

Результат всього умовного виразу не залежить від результату подвираженія (err != code2), що добре видно при побудові таблиці істинності для даного прикладу (малюнок 4)



Малюнок 4 — Таблиця істинності для логічного виразу
Порівняння беззнакових чисел з нулем


В перевіряються проектах часто зустрічаються порівняння беззнакових чисел з нулем, результат яких завжди істина або брехня. Такий код не завжди містить серйозну помилку. Часто це результат зайвої обережності, або перевірка залишається після зміни типу змінної зі знакового на беззнаковий. У будь-якому випадку такі порівняння варто уважно перевірити.

V547 Expression 'm_socket < 0' is always false. Unsigned type value is never < 0. servicenetwork.cpp 585
typedef SOCKET CRYSOCKET;
// Internal socket data
CRYSOCKET m_socket;

bool CServiceNetworkConnection::TryReconnect()
{
....
// Create new socket if needed
if (m_socket == 0)
{
m_socket = CrySock::socketinet();
if (m_socket < 0)
{
....
return false;
}
}
....
}

Детально хочу зупинитися на типі SOCKET, який може бути знаковим і беззнаковим на різних платформах, тому для роботи з цим типом настійно рекомендується використовувати спеціальні макроси і константи, визначені в стандартних заголовних файлах.

В кросплатформених проектах часто зустрічаються порівняння зі значеннями 0 або -1, які призводять до некоректної інтерпретації коду помилки. Проект CryEngine V не є винятком, хоча подекуди присутні правильні перевірки, наприклад:
if (m_socket == CRY_INVALID_SOCKET)

але в багатьох місцях за кодом використовуються різні способи перевірки.

47 місць, де беззнакові змінні підозріло порівнюються з нулем, винесені в файл CryEngine5_V547.txt. Розробникам бажано перевірити зазначені місця.

Небезпечні покажчики


Діагностичне правило V595 знаходить в коді розіменування покажчиків, перевірка валідності яких виконується нижче за кодом, тобто вже після використання покажчика. На практиці ця діагностика знаходить дуже круті помилки. Невелика кількість помилкових спрацьовувань виникає з-за того, що перевірка покажчика виконується опосередковано, тобто через одну або кілька інших змінних, але погодьтеся, що і для людини розібратися в такому коді буде вельми непросто. Я наведу три приклади спрацьовування цієї діагностики, які викликають особливе здивування, як працює такий код, решта можна подивитись у файлі CryEngine5_V595.txt.

Приклад 1

V595 The 'm_pPartManager' pointer was utilized before it was verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441
void C3DEngine::RenderInternal (....)
{
....
m_pPartManager->GetLightProfileCounts().ResetFrameTicks();
if (passInfo.IsGeneralPass() && m_pPartManager)
m_pPartManager->Update();
....
}

Розіменування і перевірка покажчика m_pPartManager.

Приклад 2

V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477
bool CGameSerialize::LoadLevel (....)
{
....
// can quick-load
if (!gEnv->p3DEngine->RestoreTerrainFromDisk())
return false;

if (gEnv->p3DEngine)
{
gEnv->p3DEngine->ResetPostEffects();
}
....
}

Розіменування і перевірка покажчика gEnv->p3DEngine.

Приклад 3

V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158
void FaceChannel::CleanupKeys (....)
{

CFacialAnimChannelInterpolator backupSpline(*pSpline);

// Create the key entries array.
int numKeys = (pSpline ? pSpline->num_keys() : 0);
....
}

Розіменування і перевірка покажчика pSpline.

Різні попередження


V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. mergedmeshrendernode.cpp 999
static inline void ExtractSphereSet (....)
{
....
switch (statusPos.pGeom->GetType())
{
if (false)
{
case GEOM_CAPSULE:
statusPos.pGeom->GetPrimitive(0, &cylinder);
}
if (false)
{
case GEOM_CYLINDER:
statusPos.pGeom->GetPrimitive(0, &cylinder);
}
for (int i = 0; i < 2 && ....; ++i)
{
....
}
break;
....
}

Мабуть, цей фрагмент коду найдивніший з усіх, що зустрічалися в проекті CryEngine V. Вибір мітки case не залежить від умовного оператора if, навіть якщо там if (false). В операторі switch виконується безумовний перехід до мітці, якщо вона задовольняє висловом switch. Без використання оператора break, за допомогою такого коду можна «обходити» виконання непотрібних операторів, але знову ж таки, супроводжувати такий неочевидний код буде легко не всім розробникам. І чому при переході до мітками GEOM_CAPSULE GEOM_CYLINDER виконується один і той же код?

V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143
typedef CryStringT<char> string;
// The actual fragment name.
string m_fragName;
//! cast to C string.
const value_type* c_str() const { return m_str; }
const value_type* data() const { return m_str; };

void LogError(const char* format, ...) const
{ .... }

void QueueAction(const UpdateContext& context)
{
....
ErrorReporter(*this, context).LogError("....'%s'", m_fragName);
....
}

Якщо в описі функції неможливо вказати число і типи усіх допустимих параметрів, то список формальних параметрів завершується эллипсисом (...), що означає: «та, можливо, ще кілька аргументів». В якості фактичного параметра для эллипсиса можуть виступати тільки POD (Plain Old Data) типи. Якщо эллипсис функції як параметр передається об'єкт класу, то практично завжди свідчить про наявність помилок в програмі. Замість вказівника на рядок в стек потрапляє вміст об'єкта. Такий код призведе до формування в буфері «абракадабри» або до аварійного завершення програми. У коді CryEngine V використовується свій клас рядка, і у нього вже є відповідний метод c_str().

Виправлений варіант:
LogError("....'%s'", m_fragName.c_str();

Ще кілька підозрілих місць:
  • V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339
  • V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648
  • V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324
  • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333
  • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864
  • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931
  • V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727
V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314
int CTriMesh::Slice (....)
{
....
bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef();
for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <=
pmd0->next = pmd;
....
}

Дуже підозрілий фрагмент коду. Після циклу for поставили крапку з комою, хоча форматування коду означає про наявність тіла циклу.

V535 The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490
void CPhysicalWorld::SimulateExplosion (....)
{
....
for(j=0;j<pmd->nIslands;j++) // <= line 3447
{
....
for(j=0;j<pcontacts[ncont].nborderpt;j++) // <= line 3490
{
....
}

Код проекту сповнений і іншим небезпечним кодом, наприклад, використанням одного лічильника у вкладеному і зовнішньому циклах. Великі файли з вихідним кодом містять заплутане форматування та зміна одних змінних в різних місцях — без статичного аналізу тут не обійтися!

Ще кілька підозрілих циклів:
  • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683
  • V535 The variable 'i1' is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576
  • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316
  • V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303
V539 Consider inspecting iterators which are being passed as arguments to function 'erase'. frameprofilerender.cpp 1090
float CFrameProfileSystem::RenderPeaks()
{
....
std::vector<SPeakRecord>& rPeaks = m_peaks;

// Go through all peaks.
for (int i = 0; i < (int)rPeaks.size(); i++)
{
....
if (age > fHotToColdTime)
{
rPeaks.erase(m_peaks.begin() + i); // <=
i--;
}
....
}

Аналізатор порахував, що в функцію, що виконує дію з контейнером, передається ітератор від іншого контейнера. У цьому фрагменті коду це не так і помилки немає. Змінна rPeaks є посиланням на m_peaks. Однак такий код може вводити в оману не тільки аналізатор коду, але і людей, які будуть супроводжувати код. Не варто так писати.

V713 The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. actiongame.cpp 4235
int CActionGame::OnCollisionImmediate(const EventPhys* pEvent)
{
....
else if (pMat->GetBreakability() == 2 &&
pCollision->idmat[0] != pCollision->idmat[1] &&
(energy = pMat->GetBreakEnergy()) > 0 &&
pCollision->mass[0] * 2 > energy &&
....
pMat->GetHitpoints() <= FtoI(min(1E6f, hitenergy / energy)) &&
pCollision) // <=
return 0;
....
}

Оператор if містить досить велику умовний вираз, в якому скрізь виконується звернення до покажчика pCollision. Помилка полягає в тому, що на рівність нулю покажчик pCollision перевіряється самим останнім, тобто вже після багаторазового разыменовывания.

V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 274
typedef std::shared_ptr<....> CDeviceGraphicsCommandListPtr;

CDeviceGraphicsCommandListPtr
CDeviceObjectFactory::GetCoreGraphicsCommandList() const
{
return m_pCoreCommandList;
}

void CRenderItemDrawer::DrawCompiledRenderItems (....) const
{
....
{
auto& RESTRICT_REFERENCE commandList = *CCryDeviceWrapper::
GetObjectFactory().GetCoreGraphicsCommandList();

passContext....->PrepareRenderPassForUse(commandList);
}
....
}

В змінну commandList зберігається посилання на значення, яке зберігає розумний покажчик. При руйнуванні об'єкта розумним покажчиком посилання стає недійсною.

Ще кілька таких місць:
  • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384
  • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368
  • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485
  • V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553
Висновок
Виправлення помилок під час написання коду майже нічого не коштує, на відміну від тих, які знаходяться на етапі роботи тестувальників, а помилки у випущеному продукті несуть вже колосальні витрати. Незалежно від використовуваного аналізатора, сама методологія статичного аналізу коду вже давно показала себе як дуже ефективний спосіб контролю якості коду програмного продукту в цілому.

Співпраця з Epic Games продемонструвало користь від впровадження статичного аналізатора Unreal Engine 4. Ми допомогли розробникам у всіх питаннях інтеграції аналізатора і навіть виправили знайдені помилки, щоб вони могли регулярно перевіряти новий код проекту. До аналогічного співпраці ми готові і з компанією Crytek.

Пропоную всім бажаючим спробувати PVS-Studio на своїх C/C++/C# проектах.

Розробники CryEngineV заздалегідь були повідомлені про перевірку проекту, тому деякі помилки можуть бути виправлені.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Svyatoslav Razmyslov. Long-Awaited Check of CryEngine V .

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.