Форум программистов, компьютерный форум, киберфорум
el_programmer
Форум программистов и сисадминов Киберфорум > Блоги > PVS-Studio
Войти
Регистрация
Восстановить пароль
Карта форума Блоги Социальные группы Поиск Сообщения за день Заказать работу  
PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.

PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.

Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.
Рейтинг: 5.00. Голосов: 1.

Ищем аномалии в X-Ray Engine

Запись от el_programmer размещена 20.06.2016 в 15:18
Обновил(-а) tezaurismosis 20.06.2016 в 15:44 (Рекламные ссылки удалены)
Автор: Павел Беликов

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 выявила большое количество как лишнего или подозрительного кода, так и однозначно ошибочных и опасных моментов. Стоит отметить, что статический анализатор отлично помогает выявлять ошибки на раннем этапе разработки, что значительно облегчает жизнь программисту и освобождает время на создание новых версий приложения.
Миниатюры
Нажмите на изображение для увеличения Название: image1.png Просмотров: 459 Размер: 218.8 Кб ID: 3895   Нажмите на изображение для увеличения Название: image2.png Просмотров: 814 Размер: 185.7 Кб ID: 3896   Нажмите на изображение для увеличения Название: image3.png Просмотров: 691 Размер: 312.8 Кб ID: 3897  

Нажмите на изображение для увеличения Название: image4.png Просмотров: 679 Размер: 120.4 Кб ID: 3898   Нажмите на изображение для увеличения Название: image5.png Просмотров: 638 Размер: 27.5 Кб ID: 3899  
Размещено в Без категории
Показов 4217 Комментарии 3
« Проверка проекта OpenJDK с помощью PVS-Studio     Главная страница
Всего комментариев 3
Комментарии
  1. Старый комментарий
    Аватар для Avazart
    Цитата:
    В конце метода пропущен return *this. По стандарту подобный код приведёт к неопределённому поведению.
    MSVC не даст такое скомпилировать.
    Запись от Avazart размещена 20.06.2016 в 16:21 Avazart вне форума
  2. Старый комментарий
    Аватар для BANO
    что-то только что на хабре эту статью прочёл
    Запись от BANO размещена 20.06.2016 в 21:26 BANO вне форума
  3. Старый комментарий
    Аватар для vvm28
    PVS-Studio - хороший продукт.
    Однако рост цен на PVS-Studio из года в год впечатляет:
    $800;
    €1600;
    €3500;
    €5250.
    Запись от vvm28 размещена 18.01.2019 в 14:23 vvm28 вне форума
 
КиберФорум - форум программистов, компьютерный форум, программирование
Поддержать - Реклама - Обратная связь
Powered by vBulletin
Copyright ©2000 - 2024, CyberForum.ru