PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.
Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.
Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.
Ищем аномалии в X-Ray Engine
Запись от el_programmer размещена 20.06.2016 в 15:18
Обновил(-а) tezaurismosis 20.06.2016 в 15:44 (Рекламные ссылки удалены)
Обновил(-а) tezaurismosis 20.06.2016 в 15:44 (Рекламные ссылки удалены)
Метки bugs, gamedev, open source, programming, xray
Автор: Павел Беликов X-Ray Engine - игровой движок, который используется в играх серии S.T.A.L.K.E.R. 16 сентября 2014 года его исходный код был выложен в открытый доступ, и с тех пор его развитием занимаются фанаты. Большой размер проекта, огромное количество багов в играх - всё это располагает к отличной демонстрации возможностей статического анализатора кода PVS-Studio. [ATTACH]3895[/ATTACH] [size=5]Вступление[/size] X-Ray был создан украинской компанией GSC GameWorld для игры S.T.A.L.K.E.R.: Тень Чернобыля. Движок включает рендер с поддержкой DirectX 8.1/9.0c/10/10.1/11, физический и звуковой движки, мультиплеер и систему искусственного интеллекта A-Life. Впоследствии компания создавала движок версии 2.0 для своей новой игры, но разработка была прекращена и исходные коды утекли в сеть. Проект вместе со всеми его зависимостями легко собирается в Visual Studio 2015. Для проверки использовался исходный код движка версии 1.6 из [url=https://github.com/OpenXRay/xray-16]репозитория на GitHub[/url] и статический анализатор кода PVS-Studio 6.04. [size=5]Copy-paste[/size] Для начала рассмотрим ошибки, связанные с копированием кода. Сценарий их возникновения в разных случаях обычно похож: скопировали код, поменяли часть переменных, а несколько - забыли. Такие ошибки могут быстро распространяться по кодовой базе, и без статического анализатора их очень легко пропустить. [ATTACH]3896[/ATTACH] [CPP]MxMatrix& MxQuadric::homogeneous(MxMatrix& H) const { .... unsigned int i, j; for(i=0; i<A.dim(); i++) for(j=0; j<A.dim(); i++) H(i,j) = A(i,j); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76 Анализатор обнаружил, что во вложенном цикле [i]for[/i] инкрементируется переменная [i]i[/i], а проверяется переменная [i]j[/i], что приводит к бесконечному циклу. Скорее всего, при копировании её просто забыли поменять. [CPP]void CBaseMonster::settings_read(CInifile const * ini, LPCSTR section, SMonsterSettings &data) { .... if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_base"), "%f,%f,%f", &data.m_attack_effector.ppi.color_base.r, &data.m_attack_effector.ppi.color_base.g, &data.m_attack_effector.ppi.color_base.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_gray"), "%f,%f,%f", &data.m_attack_effector.ppi.color_gray.r, &data.m_attack_effector.ppi.color_gray.g, &data.m_attack_effector.ppi.color_gray.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_add"), "%f,%f,%f", &data.m_attack_effector.ppi.color_add.r, &data.m_attack_effector.ppi.color_add.g, &data.m_attack_effector.ppi.color_add.b); .... } [/CPP] [b]Предупреждения PVS-Studio[/b]: [list][*]V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447[*]V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449[/list] В данном фрагменте используются подряд несколько одинаковых условных выражений. Очевидно, что необходимо заменить [i]color_base [/i]на[i] color_gray [/i]и[i] color_add [/i]в соответствии с кодом в теле [i]if[/i] ветви[i].[/i] [CPP]/* process a single statement */ static void ProcessStatement(char *buff, int len) { .... if (strncmp(buff,"\\pauthr",8) == 0) { ProcessPlayerAuth(buff, len); } else if (strncmp(buff,"\\getpidr",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpidr",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpdr",8) == 0) { ProcessGetData(buff, len); } else if (strncmp(buff,"\\setpdr",8) == 0) { ProcessSetData(buff, len); } } [/CPP] [b]Предупреждение PVS-Studio[/b]: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1502, 1505. gstats.c 1502 Как и в предыдущем примере, здесь используются два одинаковых условия ([i]strncmp(buff,"\\getpidr",9) == 0[/i]). Сложно сказать наверняка, является ли это ошибкой или просто недостижимым кодом, но на это точно стоит обратить внимание. Возможно, что здесь должны быть блоки с [i]getpidr[/i]/[i]setpidr[/i] по аналогии с [i]getpdr[/i]/[i]setpdr[/i]. [CPP] class RGBAMipMappedCubeMap { .... size_t height() const { return cubeFaces[0].height(); } size_t width() const { return cubeFaces[0].height(); } .... }; [/CPP] [b]Предупреждение PVS-Studio[/b]: V524 It is odd that the body of 'width' function is fully equivalent to the body of 'height' function. tpixel.h 1090 Методы [i]height() [/i]и [i]width() [/i]имеют одинаковое тело. Учитывая, что вычисляются размеры граней куба, возможно, ошибки здесь нет. Но лучше переписать метод [i]width() [/i]следующим образом: [CPP]size_t width() const { return cubeFaces[0].width(); } [/CPP] [size=5]Неправильное использование C++[/size] C++ - замечательный язык, который предоставляет программисту много возможностей... отстрелить себе ногу особо жестоким образом. Неопределённое поведение, утечки памяти и, конечно же, опечатки - об ошибках такого рода пойдёт речь в текущем разделе. [ATTACH]3897[/ATTACH] [CPP]template <class T> struct _matrix33 { public: typedef _matrix33<T>Self; typedef Self& SelfRef; .... IC SelfRef sMTxV(Tvector& R, float s1, const Tvector& V1) const { R.x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z); R.y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z); R.z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z); } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V591 Non-void function should return a value. _matrix33.h 435 В конце метода пропущен [i]return *this[/i]. По стандарту подобный код приведёт к неопределённому поведению. Так как возвращаемое значение является ссылкой, это, скорее всего, приведёт к падению программы при попытке обратиться к возвращаемому значению. [CPP]ETOOLS_API int __stdcall ogg_enc(....) { .... FILE *in, *out = NULL; .... input_format *format; .... in = fopen(in_fn, "rb"); if(in == NULL) return 0; format = open_audio_file(in, &enc_opts); if(!format){ fclose(in); return 0; }; out = fopen(out_fn, "wb"); if(out == NULL){ fclose(out); return 0; } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V575 The null pointer is passed into 'fclose' function. Inspect the first argument. ogg_enc.cpp 47 Довольно интересный пример. Анализатор обнаружил, что аргумент в вызове [i]fclose [/i]равен [i]nullptr[/i], что делает вызов функции бессмысленным. Можно предположить, что должны были закрыть поток [i]in.[/i] [CPP]void NVI_Image::ABGR8_To_ARGB8() { // swaps RGB for all pixels assert(IsDataValid()); assert(GetBytesPerPixel() == 4); UINT hxw = GetNumPixels(); for (UINT i = 0; i < hxw; i++) { DWORD col; GetPixel_ARGB8(&col, i); DWORD a = (col >> 24) && 0x000000FF; DWORD b = (col >> 16) && 0x000000FF; DWORD g = (col >> 8) && 0x000000FF; DWORD r = (col >> 0) && 0x000000FF; col = (a << 24) | (r << 16) | (g << 8) | b; SetPixel_ARGB8(i, col); } } [/CPP] [b]Предупреждения PVS-Studio:[/b] [list][*]V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 170[*]V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 171[*]V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 172[*]V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 173[/list] В данном участке кода перепутаны логические и битовые операции. Результат будет не таким, какого ожидал программист: [i]col[/i] будет всегда равен 0x01010101 независимо от входных данных. Правильный вариант: [CPP]DWORD a = (col >> 24) & 0x000000FF; DWORD b = (col >> 16) & 0x000000FF; DWORD g = (col >> 8) & 0x000000FF; DWORD r = (col >> 0) & 0x000000FF; [/CPP] Ещё один пример странного кода: [CPP]VertexCache::VertexCache() { VertexCache(16); } [/CPP] [b]Предупреждение PVS-Studio[/b]: V603 The object was created but it is not being used. If you wish to call constructor, 'this->VertexCache::VertexCache(....)' should be used. vertexcache.cpp 6 Вместо вызова одного конструктора из другого для инициализации экземпляра будет создан и тут же уничтожен новый объект типа [i]VertexCache[/i]. В результате члены создаваемого объекта останутся непроинициализированными. [CPP]BOOL CActor::net_Spawn(CSE_Abstract* DC) { .... m_States.empty(); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V530 The return value of function 'empty' is required to be utilized. actor_network.cpp 657 Анализатор предупреждает, что возвращаемое функцией значение не используется. Похоже, что программист перепутал методы [i]empty()[/i] и [i]clear()[/i]: [i]empty()[/i] не очищает массив, а проверяет, является ли он пустым. Такие ошибки нередко встречаются в различных проектах. Проблема в том, что имя [i]empty()[/i] не очевидно: некоторые воспринимают его как действие - удаление. Для того, чтобы подобной неоднозначности не возникало лучше добавлять глаголы has, is к началу метода: действительно, [i]isEmpty()[/i] с [i]clear()[/i] сложно перепутать. Похожее предупреждение: V530 The return value of function 'unique' is required to be utilized. uidragdroplistex.cpp 780 [CPP]size_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs, char *buffer, size_t capacity, size_t lineCapacity) { memset(buffer, capacity*lineCapacity, 0); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V575 The 'memset' function processes '0' elements. Inspect the third argument. xrdebug.cpp 104 При вызове [i]memset[/i] аргументы перепутали местами и в итоге буфер не обнуляется, как изначально задумывалось. Подобная ошибка может жить в проекте очень долго, так как её очень трудно обнаружить. В таких местах на выручку программисту приходит статический анализатор. Корректное использование [i]memset[/i]: [CPP]memset(buffer, 0, capacity*lineCapacity); [/CPP] Следующая ошибка связана с неправильно составленным логическим выражением. [CPP]void configs_dumper::dumper_thread(void* my_ptr) { .... DWORD wait_result = WaitForSingleObject( this_ptr->m_make_start_event, INFINITE); while ( wait_result != WAIT_ABANDONED) || (wait_result != WAIT_FAILED)) .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V547 Expression is always true. Probably the '&&' operator should be used here. configs_dumper.cpp 262 Выражения вида 'x != a || x != b' всегда являются истинным. Вероятнее всего вместо оператора || подразумевался оператор &&. Подробнее об ошибках в логических выражениях можно прочитать в статье "Логические выражения в C/C++. Как ошибаются профессионалы". [CPP]void SBoneProtections::reload(const shared_str& bone_sect, IKinematics* kinematics) { .... CInifile::Sect &protections = pSettings->r_section(bone_sect); for (CInifile::SectCIt i=protections.Data.begin(); protections.Data.end() != i; ++i) { string256 buffer; BoneProtection BP; .... BP.BonePassBullet = (BOOL) ( atoi( _GetItem(i->second.c_str(), 2, buffer) )>0.5f); .... } } [/CPP] [b]Предупреждение PVS-Studio[/b]: V674 The '0.5f' literal of the 'float' type is compared to a value of the 'int' type. boneprotections.cpp 54 Анализатор обнаружил сравнение целочисленного значения с вещественной константой. Возможно, что здесь по аналогии должна была использоваться функция [i]atof[/i], а не [i]atoi[/i], в другом случае стоит переписать это сравнение, чтобы оно не выглядело подозрительно. Однако сказать наверняка, является ли этот пример ошибочным или нет, может только разработчик, писавший его. [CPP]class IGameObject : public virtual IFactoryObject, public virtual ISpatial, public virtual ISheduled, public virtual IRenderable, public virtual ICollidable { public: .... virtual u16 ID() const = 0; .... } BOOL CBulletManager::test_callback( const collide::ray_defs& rd, IGameObject* object, LPVOID params) { bullet_test_callback_data* pData = (bullet_test_callback_data*)params; SBullet* bullet = pData->pBullet; if( (object->ID() == bullet->parent_id) && (bullet->fly_dist<parent_ignore_distance) && (!bullet->flags.ricochet_was)) return FALSE; BOOL bRes = TRUE; if (object){ .... } return bRes; } [/CPP] [b]Предупреждение PVS-Studio[/b]: V595 The 'object' pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42 Проверка указателя [i]object [/i]на равенство [i]nullptr [/i]идёт после того, как разыменовали [i]object->ID()[/i]. В случае, когда [i]object [/i]равен nullptr, это приведёт к падению программы. [CPP]#ifdef _EDITOR BOOL WINAPI DllEntryPoint(....) #else BOOL WINAPI DllMain(....) #endif { switch (ul_reason_for_call) { .... case DLL_THREAD_ATTACH: if (!strstr(GetCommandLine(), "-editor")) CoInitializeEx(NULL, COINIT_MULTITHREADED); timeBeginPeriod(1); break; .... } return TRUE; } [/CPP] [b]Предупреждение PVS-Studio[/b]: V718 The 'CoInitializeEx' function should not be called from 'DllMain' function. xrcore.cpp 205 В теле [i]DllMain[/i] нельзя использовать часть WinAPI функций, включая [i]CoInitializeEx. [/i]Убедиться в этом можно, прочитав [url=https://msdn.microsoft.com/en-us/library/windows/desktop/ms695279%28v=vs.85%29.aspx]документацию на MSDN[/url]. Нельзя дать какой-то однозначный совет, как стоит переписать эту функцию, но стоит понимать, что такая ситуация опасна, так как она может привести к взаимной блокировке потоков или аварийному завершению. [size=5]Ошибки в приоритетах[/size] [CPP]int sgetI1( unsigned char **bp ) { int i; if ( flen == FLEN_ERROR ) return 0; i = **bp; if ( i > 127 ) i -= 256; flen += 1; *bp++; return i; } [/CPP] [b]Предупреждение PVS-Studio[/b]: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 316 Ошибка связана с использованием инкремента. Для наглядности перепишем данное выражение, расставив скобки: [CPP]*(bp++); [/CPP] То есть произойдёт сдвиг не содержимого по адресу [i]bp,[/i] а самого указателя, что в данном контексте бессмысленно. Ниже по коду есть фрагменты вида [i]*bp += N[/i], из-за чего я и сделал вывод, что это ошибка. Избежать подобной ошибки помогла бы расстановка скобок, что сделало бы порядок вычислений более понятным. Также неплохой методикой является использование [i]const[/i] для аргументов, которые не должны меняться. Аналогичные предупреждения: [list][*]V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 354[*]V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwob.c 80[/list] [CPP]void CHitMemoryManager::load (IReader &packet) { .... if (!spawn_callback || !spawn_callback->m_object_callback) if(!g_dedicated_server) Level().client_spawn_manager().add( delayed_object.m_object_id,m_object->ID(),callback); #ifdef DEBUG else { if (spawn_callback && spawn_callback->m_object_callback) { VERIFY(spawn_callback->m_object_callback == callback); } } #endif // DEBUG } [/CPP] [b]Предупреждение PVS-Studio[/b]: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. hit_memory_manager.cpp 368 В этом фрагменте ветвь [i]else[/i] относится ко второму [i]if[/i] из-за своей право-ассоциативности, что не совпадает с форматированием кода. К счастью, данный случай не отражается на работе программы, тем не менее, он может усложнить процесс отладки и тестирования. Рекомендация проста - в более-менее сложных ветвлениях расставляйте фигурные скобки. [CPP]void HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM& hud_snd, const Fvector& position, const IGameObject* parent, bool b_hud_mode, bool looped, u8 index) { .... hud_snd.m_activeSnd->snd.set_volume( hud_snd.m_activeSnd->volume * b_hud_mode?psHUDSoundVolume:1.0f); } [/CPP] [b]Предупреждение PVS-Studio[/b]: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '*' operator. hudsound.cpp 108 У тернарного условного оператора приоритет ниже, чем у умножения, поэтому порядок операций будет следующим: [CPP](hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0f [/CPP] Очевидно, что правильный код должен выглядеть так: [CPP]hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f) [/CPP] Выражения, содержащие тернарный оператор, несколько [i]if-else[/i] ветвей или операции И/ИЛИ, - это те случаи, когда лучше поставить лишние скобки. Аналогичные предупреждения: [list][*]V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. uihudstateswnd.cpp 487[*]V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. uicellcustomitems.cpp 106[/list] [size=5]Лишние сравнения[/size] [CPP]void CDestroyablePhysicsObject::OnChangeVisual() { if (m_pPhysicsShell){ if(m_pPhysicsShell)m_pPhysicsShell->Deactivate(); .... } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V571 Recurring check. The 'if (m_pPhysicsShell)' condition was already verified in line 32. destroyablephysicsobject.cpp 33 В данном примере дважды проверяется [i]m_pPhysicsShell[/i]. Скорее всего, вторая проверка лишняя. [CPP]void CSE_ALifeItemPDA::STATE_Read(NET_Packet &tNetPacket, u16 size) { .... if (m_wVersion > 89) if ( (m_wVersion > 89)&&(m_wVersion < 98) ) { .... }else{ .... } } [/CPP] [b]Предупреждение PVS-Studio[/b]: V571 Recurring check. The 'm_wVersion > 89' condition was already verified in line 987. xrserver_objects_alife_items.cpp 989 Очень странный код. То ли здесь забыли выражение после [i]if (m_wVersion > 89)[/i], то ли целую серию [i]else-if[/i]. Данный метод требует более подробного рассмотрения разработчиком проекта. [CPP]void ELogCallback(void *context, LPCSTR txt) { .... bool bDlg = ('#'==txt[0])||((0!=txt[1])&&('#'==txt[1])); if (bDlg){ int mt = ('!'==txt[0])||((0!=txt[1])&&('!'==txt[1]))?1:0; .... } } [/CPP] [b]Предупреждения PVS-Studio[/b]: [list][*]V590 Consider inspecting the '(0 != txt[1]) && ('#' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 29[*]V590 Consider inspecting the '(0 != txt[1]) && ('!' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 31[/list] В выражениях инициализации переменных [i]bDlg [/i]и [i]mt [/i]проверка [i](0 != txt[1]) [/i]является избыточной. Если её опустить, выражения станут читаться значительно легче: [CPP]bool bDlg = ('#'==txt[0])||('#'==txt[1]); int mt = ('!'==txt[0])||('!'==txt[1])?1:0; [/CPP] [size=5]Ошибки в типах данных[/size] [ATTACH]3898[/ATTACH] [CPP]float CRenderTarget::im_noise_time; CRenderTarget::CRenderTarget() { .... param_blur = 0.f; param_gray = 0.f; param_noise = 0.f; param_duality_h = 0.f; param_duality_v = 0.f; param_noise_fps = 25.f; param_noise_scale = 1.f; im_noise_time = 1/100; im_noise_shift_w = 0; im_noise_shift_h = 0; .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V636 The '1 / 100' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gl_rendertarget.cpp 245 Значение выражения 1/100 равно 0, так как выполняется операция целочисленного деления. Чтобы получить значение 0.01f, нужно использовать вещественный литерал, переписав выражение: 1/100.0f. Хотя возможно, что данное поведение было предусмотрено автором, и ошибки здесь нет. [CPP] CSpaceRestriction::merge(....) const { .... LPSTR S = xr_alloc<char>(acc_length); for ( ; I != E; ++I) temp = strconcat(sizeof(S),S,*temp,",",*(*I)->name()); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V579 The strconcat function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the first argument. space_restriction.cpp 201 Функция [i]strconcat[/i], в качестве первого параметра принимает длину буфера. Буфер [i]S[/i] объявлен, как [i]LPSTR[/i], то есть как указатель на строку. [i]sizeof(S)[/i] будет равен размеру указателя в байтах, то есть [i]sizeof(char *)[/i], а не количеству символов в строке. Для вычисления длины следует использовать [i]strlen(S)[/i]. [CPP]class XRCDB_API MODEL { .... u32 status; // 0=ready, 1=init, 2=building .... } void MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, build_callback* bc, void* bcp) { .... BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp }; thread_spawn(build_thread,"CDB-construction",0,&P); while (S_INIT == status) Sleep(5); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. xrcdb.cpp 100 Компилятор может убрать проверку [i]S_INIT == status[/i] в качестве оптимизации, так как переменная [i]status[/i] не модифицируется в цикле. Для того, чтобы избежать подобного поведения, нужно использовать [i]volatile[/i] переменные или типы синхронизации данных между потоками. Аналогичные предупреждения: [list][*]V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23[*]V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232[/list] [CPP]void CAI_Rat::UpdateCL() { .... if (!Useful()) { inherited::UpdateCL (); Exec_Look (Device.fTimeDelta); CMonsterSquad *squad = monster_squad().get_squad(this); if (squad && ((squad->GetLeader() != this && !squad->GetLeader()->g_Alive()) || squad->get_index(this) == u32(-1))) squad->SetLeader(this); .... } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V547 Expression 'squad->get_index(this) == u32(- 1)' is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480 Для того, чтобы понять, почему это выражение всегда ложно, вычислим значения отдельных операндов. u32(-1) равен 0xFFFFFFFF или 4294967295. Тип, возвращаемый методом [i]squad->get_index(....)[/i], - u8, следовательно его максимальное значение - 0xFF или 255, что строго меньше, чем u32(-1). Соответственно, значением такого сравнения всегда будет [i]false[/i]. Данный код легко исправить, поменяв тип данных на u8: [CPP]squad->get_index(this) == u8(-1) [/CPP] Та же диагностика срабатывает и для избыточных сравнений беззнаковых переменных: [CPP]namespace ALife { typedef u64 _TIME_ID; } ALife::_TIME_ID CScriptActionCondition::m_tLifeTime; IC bool CScriptEntityAction::CheckIfTimeOver() { return((m_tActionCondition.m_tLifeTime >= 0) && ((m_tActionCondition.m_tStartTime + m_tActionCondition.m_tLifeTime) < Device.dwTimeGlobal)); } [/CPP] [b]Предупреждение PVS-Studio[/b]: V547 Expression 'm_tActionCondition.m_tLifeTime >= 0' is always true. Unsigned type value is always >= 0. script_entity_action_inline.h 115 Переменная [i]m_tLifeTime [/i]является беззнаковой, соответственно она всегда больше или равна нулю. Является ли это лишней проверкой или же здесь скрыта ошибка в логике, судить разработчику. Аналогичное предупреждение: V547 Expression 'm_tActionCondition.m_tLifeTime < 0' is always false. Unsigned type value is never < 0. script_entity_action_inline.h 143 [CPP]ObjectFactory::ServerObjectBaseClass * CObjectItemScript::server_object (LPCSTR section) const { ObjectFactory::ServerObjectBaseClass *object = nullptr; try { object = m_server_creator(section); } catch(std::exception e) { Msg("Exception [%s] raised while creating server object from " "section [%s]", e.what(),section); return (0); } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39 Функция [i]std::exception::what() [/i]является виртуальной и может быть переопределена в наследуемых классах. В данном примере исключение ловится по значению, следовательно, экземпляр класса будет скопирован и вся информация о полиморфном типе будет потеряна. Обращаться к [i]what()[/i] в таком случае бессмысленно. Исключение стоит перехватывать по ссылке: [CPP] catch(const std::exception& e) { [/CPP] [size=5]Разное[/size] [CPP]void compute_cover_value (....) { .... float value [8]; .... if (value[0] < .999f) { value[0] = value[0]; } .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V570 The 'value[0]' variable is assigned to itself. compiler_cover.cpp 260 Переменная [i]value[0][/i]присваивается сама себе. Зачем это делать - непонятно. Возможно, ей должно было быть присвоено другое значение. [CPP]void CActor::g_SetSprintAnimation(u32 mstate_rl, MotionID &head, MotionID &torso, MotionID &legs) { SActorSprintState& sprint = m_anims->m_sprint; bool jump = (mstate_rl&mcFall) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding2) || (mstate_rl&mcJump); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V501 There are identical sub-expressions '(mstate_rl & mcLanding)' to the left and to the right of the '||' operator. actoranimation.cpp 290 Вероятнее всего здесь просто лишняя проверка [i]mstate_rl & mcLanding[/i], но часто подобные предупреждения сигнализируют об ошибке в логике и нерассмотренных значениях enum. Аналогичные предупреждения: [list][*]V501 There are identical sub-expressions 'HudItemData()' to the left and to the right of the '&&' operator. huditem.cpp 338[*]V501 There are identical sub-expressions 'list_idx == e_outfit' to the left and to the right of the '||' operator. uimptradewnd_misc.cpp 392[*]V501 There are identical sub-expressions '(D3DFMT_UNKNOWN == fTarget)' to the left and to the right of the '||' operator. hw.cpp 312[/list] [CPP]RELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS() { .... spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58 Анализатор обнаружил, что одной переменной присваиваются подряд два значения. В данном случае похоже, что это просто мёртвый код и его стоит удалить. [CPP]void safe_verify(....) { .... printf("FATAL ERROR (%s): failed to verify data\n"); .... } [/CPP] [b]Предупреждение PVS-Studio[/b]: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 2. Present: 1. entry_point.cpp 41 В функцию [i]printf[/i] передаётся недостаточное количество аргументов: формат '%s' указывает на то, что должен быть передан указатель на строку. Такая ситуация может привести к ошибке доступа к памяти и экстренному завершению программы. [size=5]Заключение[/size] [ATTACH]3899[/ATTACH] Проверка исходного кода X-Ray Engine выявила большое количество как лишнего или подозрительного кода, так и однозначно ошибочных и опасных моментов. Стоит отметить, что статический анализатор отлично помогает выявлять ошибки на раннем этапе разработки, что значительно облегчает жизнь программисту и освобождает время на создание новых версий приложения. |
Всего комментариев 3
Комментарии
-
Запись от Avazart размещена 20.06.2016 в 16:21 -
Запись от BANO размещена 20.06.2016 в 21:26 -
Запись от vvm28 размещена 18.01.2019 в 14:23