# Побочный результат: проверяем CryEngine 3 SDK с помощью PVS\-Studio Мы закончили сравнивать статические анализаторы кода Cppcheck, PVS\-Studio и анализатор встроенный в Visual Studio 2013\. В ходе этого было проверено более 10 открытых проектов\. И про некоторые из них можно написать статьи\. Вот очередная такая статья о результатах проверки проекта CryEngine 3 SDK\. ![0240_CryEngineSDK_ru/image1.png](https://import.viva64.com/docx/blog/0240_CryEngineSDK_ru/image1.png) ## CryEngine 3 SDK Wikipedia: [CryEngine 3 SDK](https://ru.wikipedia.org/wiki/CryEngine_3_SDK) — набор программных инструментов для разработки компьютерных игр на игровом движке CryEngine 3\. CryEngine 3 SDK, как и CryEngine 3, разработан и поддерживается немецкой компанией Crytek\. CryEngine 3 SDK является проприетарным бесплатным \(freeware\) для скачивания, установки и использования средством разработки, с помощью которого любой желающий может разрабатывать компьютерные игры и бесплатно распространять их\. При коммерческом распространении \(продажах\) игр, разработанных на CryEngine 3 SDK, необходимо делать лицензионные отчисления в Crytek\. ## PVS\-Studio Давайте посмотрим, что интересного можно найти в этой библиотеке с помощью анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. PVS\-Studio находит немного больше, если включить 3 уровень предупреждений\. Пример: ```cpp static void GetNameForFile( const char* baseFileName, const uint32 fileIdx, char outputName[512] ) { assert(baseFileName != NULL); sprintf( outputName, "%s_%d", baseFileName, fileIdx ); } ``` V576 Incorrect format\. Consider checking the fourth actual argument of the 'sprintf' function\. The SIGNED integer type argument is expected\. igame\.h 66 Формально, для распечатки беззнаковой переменной 'fileIdx' надо использовать "%u"\. Однако сомнительно, что эта переменная достигнет значения больше, чем INT\_MAX\. Так что на самом деле ошибка никак себя не проявит\. ## Результаты проверки Краткий результат \- надо пользоваться статическими анализаторами кода\. И тогда ошибок в программе станет меньше и мне не о чем будет писать статьи\. ### Двойная проверка ```cpp void CVehicleMovementArcadeWheeled::InternalPhysicsTick(float dt) { .... if (fabsf(m_movementAction.rotateYaw)>0.05f || vel.GetLengthSquared()>0.001f || m_chassis.vel.GetLengthSquared()>0.001f || angVel.GetLengthSquared()>0.001f || angVel.GetLengthSquared()>0.001f) .... } ``` [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'angVel\.GetLengthSquared\(\) \> 0\.001f' to the left and to the right of the '\|\|' operator\. vehiclemovementarcadewheeled\.cpp 3300 Два раза выполняется проверка "angVel\.GetLengthSquared\(\)\>0\.001f"\. Одна проверка лишняя\. Или это опечатка, за\-за которой не проверяется другое значение\. ### Одно и тоже действие при разных условиях **Фрагмент N1\.** ```cpp void CVicinityDependentObjectMover::HandleEvent(....) { .... else if ( strcmp(szEventName, "ForceToTargetPos") == 0 ) { SetState(eObjectRangeMoverState_MovingTo); SetState(eObjectRangeMoverState_Moved); ActivateOutputPortBool( "OnForceToTargetPos" ); } else if ( strcmp(szEventName, "ForceToTargetPos") == 0 ) { SetState(eObjectRangeMoverState_MovingTo); SetState(eObjectRangeMoverState_Moved); ActivateOutputPortBool( "OnForceToTargetPos" ); } .... } ``` [V517](https://pvs-studio.ru/ru/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 255, 261\. vicinitydependentobjectmover\.cpp 255 Мне кажется, что этот код написан с помощью Copy\-Paste\. И ещё мне кажется, что в этом коде после копирования забыли что\-то исправить\. **Фрагмент** **N2\.** Очень странным образом реализована функция ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass\(\)\. Вот это я понимаю ИМЯ\! ```cpp bool CGameRules:: ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass (const IEntityClass* pEntityClass) const { assert(pEntityClass != NULL); if(gEnv->bMultiplayer) { return (pEntityClass == s_pSmartMineClass) || (pEntityClass == s_pTurretClass) || (pEntityClass == s_pC4Explosive); } else { return (pEntityClass == s_pSmartMineClass) || (pEntityClass == s_pTurretClass) || (pEntityClass == s_pC4Explosive); } } ``` [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. gamerules\.cpp 5401 **Аналогичные крайне подозрительные места:** * environmentalweapon\.cpp 964 * persistantstats\.cpp 610 * persistantstats\.cpp 714 * recordingsystem\.cpp 8924 * movementtransitions\.cpp 610 * gamerulescombicaptureobjective\.cpp 1692 * vehiclemovementhelicopter\.cpp 588 ### Неинициализированная ячейка массива ```cpp TDestructionEventId destructionEvents[2]; SDestructibleBodyPart() : hashId(0) , healthRatio(0.0f) , minHealthToDestroyOnDeathRatio(0.0f) { destructionEvents[0] = -1; destructionEvents[0] = -1; } ``` [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'destructionEvents\[0\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 75, 76\. bodydestruction\.h 76 Массив 'destructionEvents' состоит из двух элементов\. В конструкторе массив хотели инициализировать\. Но не получилось\. ### Не там поставили скобку ```cpp bool ShouldRecordEvent(size_t eventID, IActor* pActor=NULL) const; void CActorTelemetry::SubscribeToWeapon(EntityId weaponId) { .... else if(pMgr->ShouldRecordEvent(eSE_Weapon), pOwnerRaw) .... } ``` [V639](https://pvs-studio.ru/ru/docs/warnings/v639/) Consider inspecting the expression for 'ShouldRecordEvent' function call\. It is possible that one of the closing '\)' brackets was positioned incorrectly\. actortelemetry\.cpp 288 Редкая и интересная ошибка\. Не там поставлена закрывающаяся скобка\. Дело в том, что второй аргумент функции ShouldRecordEvent\(\) является необязательным\. Получается, что в начале вызывается функция ShouldRecordEvent\(\)\. Затем, оператор запятая ',' возвращает значение, стоящее с права\. Условие зависит только от переменной 'pOwnerRaw'\. В общем, чёрте что получилось\. ### Забыли написать имя функции ```cpp virtual void ProcessEvent(....) { .... string pMessage = ("%s:", currentSeat->GetSeatName()); .... } ``` [V521](https://pvs-studio.ru/ru/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression '"%s:", currentSeat\-\>GetSeatName\(\)' is correct\. flowvehiclenodes\.cpp 662 В этом выражении переменной pMessage присваивается значение currentSeat\-\>GetSeatName\(\)\. Никакого форматирования не происходит\. Как результат в строке будет отсутствовать двоеточие ':'\. Хоть и мелочь, но ошибка\. Должно было быть: ```cpp string pMessage = string().Format("%s:", currentSeat->GetSeatName()); ``` ### Бессмысленные и беспощадные проверки **Фрагмент N1\.** ```cpp inline bool operator != (const SEfResTexture &m) const { if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 || m_TexFlags != m.m_TexFlags || m_bUTile != m.m_bUTile || m_bVTile != m.m_bVTile || m_Filter != m.m_Filter || m_Ext != m.m_Ext || m_Sampler != m.m_Sampler) return true; return false; } ``` [V549](https://pvs-studio.ru/ru/docs/warnings/v549/) The first argument of 'stricmp' function is equal to the second argument\. ishader\.h 2089 Если не заметили ошибку, то подсказываю\. Строка m\_Name\.c\_str\(\) сравнивается сама с собой\. Должно быть написано: ```cpp stricmp(m_Name.c_str(), m.m_Name.c_str()) ``` **Фрагмент** **N2\.** Теперь логическая ошибка: ```cpp SearchSpotStatus GetStatus() const { return m_status; } SearchSpot* SearchGroup::FindBestSearchSpot(....) { .... if(searchSpot.GetStatus() != Unreachable || searchSpot.GetStatus() != BeingSearchedRightAboutNow) .... } ``` [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. searchmodule\.cpp 469 Проверка в этом коде не имеет смысла\. Приведу аналогию: ```cpp if (A != 1 || A != 2) ``` Условие всегда выполняется\. **Фрагмент N3\.** ```cpp const CCircularBufferTimeline * CCircularBufferStatsContainer::GetTimeline( size_t inTimelineId) const { .... if (inTimelineId >= 0 && (int)inTimelineId < m_numTimelines) { tl = &m_timelines[inTimelineId]; } else { CryWarning(VALIDATOR_MODULE_GAME,VALIDATOR_ERROR, "Statistics event %" PRISIZE_T " is larger than the max registered of %" PRISIZE_T ", event ignored", inTimelineId,m_numTimelines); } .... } ``` [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'inTimelineId \>\= 0' is always true\. Unsigned type value is always \>\= 0\. circularstatsstorage\.cpp 31 **Фрагмент N4\.** ```cpp inline typename CryStringT::size_type CryStringT::rfind( value_type ch,size_type pos ) const { const_str str; if (pos == npos) { .... } else { if (pos == npos) pos = length(); .... } ``` [V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'if \(pos \=\= npos\)' condition was already verified in line 1447\. crystring\.h 1453 Присваивание "pos \= length\(\)" никогда не будет выполнено\. **Аналогичный код здесь:** cryfixedstring\.h 1297 ## Указатели Программисты очень любят проверять указатель на равенство нулю\. Вот если бы они ещё знали, как часто они делают это неправильно\. Проверяют, когда уже поздно\. Приведу один пример, остальное перечислю списком\. ```cpp IScriptTable *p; bool Create( IScriptSystem *pSS, bool bCreateEmpty=false ) { if (p) p->Release(); p = pSS->CreateTable(bCreateEmpty); p->AddRef(); return (p)?true:false; } ``` [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'p' pointer was utilized before it was verified against nullptr\. Check lines: 325, 326\. scripthelpers\.h 325 **Обещанный список из 35 сообщений:** [CryEngineSDK\-595\.txt](https://pvs-studio.com/external-pictures/txt/cryenginesdk-595.txt) ### Неопределённое поведение ```cpp void AddSample( T x ) { m_index = ++m_index % N; .... } ``` [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'm\_index' variable is modified while being used twice between sequence points\. inetwork\.h 2303 ### Циклы "на один раз" ```cpp void CWeapon::AccessoriesChanged(bool initialLoadoutSetup) { .... for (int i = 0; i < numZoommodes; i++) { CIronSight* pZoomMode = .... const SZoomModeParams* pCurrentParams = .... const SZoomModeParams* pNewParams = .... if(pNewParams != pCurrentParams) { pZoomMode->ResetSharedParams(pNewParams); } break; } .... } ``` [V612](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'break' within a loop\. weapon\.cpp 2854 Тело цикла будет выполнять только один раз\. Причина \- безусловный оператор 'break'\. При этом, в цикле нет операторов 'continue'\. **Есть ещё несколько таких подозрительных циклов**: * gunturret\.cpp 1647 * vehiclemovementbase\.cpp 2362 * vehiclemovementbase\.cpp 2382 ### Странные присваивания **Фрагмент N1\.** ```cpp void CPlayerStateGround::OnPrePhysicsUpdate(....) { .... modifiedSlopeNormal.z = modifiedSlopeNormal.z; .... } ``` [V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'modifiedSlopeNormal\.z' variable is assigned to itself\. playerstateground\.cpp 227 **Фрагмент N2\.** ```cpp const SRWIParams& Init(....) { .... objtypes=ent_all; flags=rwi_stop_at_pierceable; org=_org; dir=_dir; objtypes=_objtypes; .... } ``` [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'objtypes' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 2807, 2808\. physinterface\.h 2808 Члену класса 'objtypes' два раза присваивается значение\. **Фрагмент N3\.** ```cpp void SPickAndThrowParams::SThrowParams::SetDefaultValues() { .... maxChargedThrowSpeed = 20.0f; maxChargedThrowSpeed = 15.0f; } ``` [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'maxChargedThrowSpeed' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1284, 1285\. weaponsharedparams\.cpp 1285 **И ещё несколько таких аномальных присваиваний**: * The 'bExecuteCommandLine' variable\. Check lines: 628, 630\. isystem\.h 630 * The 'flags' variable\. Check lines: 2807, 2808\. physinterface\.h 2808 * The 'entTypes' Variable\. Check lines: 2854, 2856\. physinterface\.h 2856 * The 'geomFlagsAny' variable\. Check lines: 2854, 2857\. physinterface\.h 2857 * The 'm\_pLayerEffectParams' variable\. Check lines: 762, 771\. ishader\.h 771 ### Нужно быть аккуратным с именами сущностей ```cpp void CGamePhysicsSettings::Debug(....) const { .... sprintf_s(buf, bufLen, pEntity->GetName()); .... } ``` [V618](https://pvs-studio.ru/ru/docs/warnings/v618/) It's dangerous to call the 'sprintf\_s' function in such a manner, as the line being passed could contain format specification\. The example of the safe code: printf\("%s", str\); gamephysicssettings\.cpp 174 Пожалуй, это не ошибка, но опасный код\. Если вдруг в имени сущности встретится символ '%', то результаты могут быть весьма неожиданными\. ### Одинокий странник ```cpp CPersistantStats::SEnemyTeamMemberInfo *CPersistantStats::GetEnemyTeamMemberInfo(EntityId inEntityId) { .... insertResult.first->second.m_entityId; .... } ``` [V607](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'insertResult\.first\-\>second\.m\_entityId'\. persistantstats\.cpp 4814 Одинокое выражение, которое ничего не делает\. Ошибка? Недописанный код? **Ещё** **одно** **место** **в** **коде:** recordingsystem\.cpp 2671 ### Оператор new ```cpp bool CreateWriteBuffer(uint32 bufferSize) { FreeWriteBuffer(); m_pWriteBuffer = new uint8[bufferSize]; if (m_pWriteBuffer) { m_bufferSize = bufferSize; m_bufferPos = 0; m_allocated = true; return true; } return false; } ``` [V668](https://pvs-studio.ru/ru/docs/warnings/v668/) There is no sense in testing the 'm\_pWriteBuffer' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. crylobbypacket\.h 88 Код устарел\. Теперь при ошибки выделения памяти оператор 'new' генерирует исключение\. **Другие места, ждущие рефакторинга**: * cry\_math\.h 73 * datapatchdownloader\.cpp 106 * datapatchdownloader\.cpp 338 * game\.cpp 1671 * game\.cpp 4478 * persistantstats\.cpp 1235 * sceneblurgameeffect\.cpp 366 * killcamgameeffect\.cpp 369 * downloadmgr\.cpp 1090 * downloadmgr\.cpp 1467 * matchmakingtelemetry\.cpp 69 * matchmakingtelemetry\.cpp 132 * matchmakingtelemetry\.cpp 109 * telemetrycollector\.cpp 1407 * telemetrycollector\.cpp 1470 * telemetrycollector\.cpp 1467 * telemetrycollector\.cpp 1479 * statsrecordingmgr\.cpp 1134 * statsrecordingmgr\.cpp 1144 * statsrecordingmgr\.cpp 1267 * statsrecordingmgr\.cpp 1261 * featuretester\.cpp 876 * menurender3dmodelmgr\.cpp 1373 ## Выводы Особенных выводов у меня нет\. Зато представляю, сколько всего интересного можно найти не в CryEngine 3 SDK, а в самом движке CryEngine 3\. Желаю всем безбажного кода\!