Команда PVS-Studio готує технічний прорив, ну а поки перевіримо Blender

PVS-Studio, Blender, C/C++Статичний аналіз найбільш корисний при регулярних перевірках. Особливо для таких, що активно розвиваються проектів як Blender. Прийшов час перевірити його знову, і дізнатися, які підозрілі місця вдасться знайти на цей раз.

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

Проект вже перевірявся раніше. Результати перевірки версії 2.62 викладені у статті "Перевірка проекту Blender з допомогою PVS-Studio".

З часу минулої перевірки розмір вихідного коду разом з додатковими бібліотеками збільшився до 77 мегабайт. А його обсяг зріс до 2206 KLOC. На момент попередньої повірки розмір проекту становив 68 мегабайт (2105 KLOC).

Розрахувати обсяг коду мені допомогла утиліта SourceMonitor. Ця утиліта вміє аналізувати код на мовах C++, C, C#, VB.NET, Java, Delphi і розраховує різні метрики. Наприклад, вона може визначити цикломатическую складність ваших проектів, а також сформувати докладну статистику для кожного з файлів проекту. І показати результати у вигляді таблиць або діаграм.

Отже, ця стаття розповість про помилки і підозрілі місця, знайдені в версії Blender 2.77 a. Для перевірки використовувався аналізатор PVS-Studio версії 6.05.

Помилки
При активному використанні механізмів копіювання та автоматичного доповнення коду дуже часто можуть виникнути помилки в найменуваннях різних змінних і констант. Такі помилки можуть привести до невірних результатів обчислень або неочікуваної поведінки програми. У проекті Blender аналізатор знайшов одразу кілька прикладів. Розглянемо їх докладно.

Помилка в умови
CurvePoint::CurvePoint(CurvePoint *iA, CurvePoint *iB, float t3)
{
....
if ((iA->getPoint2D() - //<=
iA->getPoint2D()).norm() < 1.0 e-6) { //<=
....
}
....
} 

V501 There are identical sub-expressions to the left and to the right of the '-' operator: iA->getPoint2D() — iA->getPoint2D() curve.cpp 136

Всередині функції CurvePoint відбувається робота з двома близькими по імені об'єктами iA iB. Різні методи цих об'єктів постійно перетинаються в різних операціях у досить довгому дереві умов. В одному з умовних блоків допущена помилка. В результаті відбувається операція віднімання між властивістю одного і того ж об'єкта. Не знаючи особливостей коду, точно сказати, в якій із двох операндів допущена помилка, неможливо. Пропоную два можливих варіанти її виправлення:
if ((iA->getPoint2D()-iB->getPoint2D()).norm()<1.0 e-6)....

або
if ((iB->getPoint2D()-iA->getPoint2D()).norm()<1.0 e-6)....

Наступна помилка теж сховалася всередині умовного оператора.
template < typename MatrixType, int QRPreconditioner>
void JacobiSVD<MatrixType, QRPreconditioner>::allocate (....)
{
....
if(m_cols>m_rows)m_qr_precond_morecols.allocate(*this);
if(m_rows>m_cols)m_qr_precond_morerows.allocate(*this);
if(m_cols!=m_cols)m_scaledMatrix.resize(rows,cols); //<=
}

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: m_cols != m_cols jacobisvd.h 819

У наведеному фрагменті коду відбувається рівняння кількості рядків і стовпців всередині певної матриці. Якщо кількість відповідно не одно, відбувається виділення пам'яті для нових елементів або їх створення. А після, якщо були додані нові осередки, відбувається операція зміни розміру матриці. Але в результаті помилки у вираженні умовного оператора операція не відбудеться ніколи, так як умова m_cols!=m_cols завжди помилково. В даному випадку не має значення, яку з двох частин виразу потрібно змінити, тому пропоную такий варіант:
if(m_cols!=m_rows) m_scaledMatrix.resize(rows,cols)

Ще кілька проблемних місць, виявлених діагностикою V501:
  • V501 There are identical sub-expressions to the left and to the right of the '==' operator: left.rows() == left.rows() numeric.cc 112
  • V501 There are identical sub-expressions to the left and to the right of the '>' operator: (from[0][3]) > (from[0][3]) stereoimbuf.c 120
  • V501 There are identical sub-expressions to the left and to the right of the '>' operator: (from[0][3]) > (from[0][3]) stereoimbuf.c 157
  • V501 There are identical sub-expressions to the left and to the right of the '==' operator: out->y == out->y filter.c 209
Робота з нульовим покажчиком
Тут помилка в найменуванні призвела до більш серйозної помилки.
int QuantitativeInvisibilityF1D::operator () (....)
{
ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter);
if (ve) {
result = ve->qi();
return 0;
}
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = ve->qi(); //<=
return 0;
}
....
}

V522 Dereferencing of the null pointer 've' might take place. functions1d.cpp 107

Наведена функція досить коротка, але помилки можуть підстерігати нас навіть в простих функціях. З коду видно, що послідовно створюються і перевіряються два об'єкта. Але після перевірки другого об'єкта виникла помилка, та якщо fe успішно створений, то замість нього в результат записується результат роботи функції з першого об'єкта, який згідно з раннім умов не був створений. Це скоріше за все призведе до аварійного завершення програми, якщо виключення не перехопить обробник більш високого рівня.

По всій видимості другий фрагмент коду писався з допомогою Copy-Paste. І випадково в одному місці забули поміняти ім'я змінної ve. Правильний код, швидше за все, мав бути таким:
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = fe->qi();
return 0;
}
Використання нульового вказівника
static ImBuf *accessor_get_ibuf (....)
{
ImBuf *ibuf, *orig_ibuf, *final_ibuf;
....
/* First try to get fully processed image from the cache. */
ibuf = accesscache_get(accessor,
clip_index,
frame,
input_mode,
downscale,
transform_key);
if (ibuf != NULL) {
return ibuf;
}
/* And now we do postprocessing of the original frame. */
orig_ibuf = accessor_get_preprocessed_ibuf(accessor, 
clip_index, 
frame);
if (orig_ibuf == NULL) {
return NULL;
}
....
if (downscale > 0) {
if (final_ibuf == orig_ibuf) {
final_ibuf = IMB_dupImBuf(orig_ibuf);
}
IMB_scaleImBuf(final_ibuf,
ibuf->x / (1 << downscale), //<=
ibuf->y / (1 << downscale)); //<=
}
....
if (input_mode == LIBMV_IMAGE_MODE_RGBA) {
BLI_assert(ibuf->channels == 3 || //<=
ibuf->channels == 4); //<=
}
....
return final_ibuf;
}

Попередження:
  • V522 Dereferencing of the null pointer 'ibuf' might take place. tracking_util.c 765
  • V522 Dereferencing of the null pointer 'ibuf' might take place. tracking_util.c 766
  • V522 Dereferencing of the null pointer 'ibuf' might take place. tracking_util.c 783
У наведеному фрагменті видно, що перевірка змінної ibuf у разі, якщо об'єкт створений, перериває функцію набагато раніше, ніж ця змінна використовується. Можливо на цьому можна було зупинитися і підтвердити факт розіменування вказівника. Однак при більш уважному вивченні коду і коментарів до нього, з'ясовується справжня причина помилки. І насправді тут знову допущена помилка. У місцях, зазначених аналізатором, насправді повинна була використовуватися змінна orig_ibuf замість ibuf.

Неправильний тип змінної
typedef enum eOutlinerIdOpTypes {
OUTLINER_IDOP_INVALID = 0, 
OUTLINER_IDOP_UNLINK,
OUTLINER_IDOP_LOCAL,
....
} eOutlinerIdOpTypes;

typedef enum eOutlinerLibOpTypes {
OL_LIB_INVALID = 0,
OL_LIB_RENAME,
OL_LIB_DELETE,
} eOutlinerLibOpTypes;

static int outliner_lib_operation_exec (....)
{
....
eOutlinerIdOpTypes event; //<=
....
event = RNA_enum_get(op->ptr, "type");
switch (event) {
case OL_LIB_RENAME: //<= 
{
....
}
case OL_LIB_DELETE: //<= 
{
....
}
default:
/* invalid - unhandled */
break;
}
....
}

Попередження:
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. outliner_tools.c 1286
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. outliner_tools.c 1295
У прикладі показано два типи, які є перерахуваннями. І те, що при написанні коду в типі була допущена помилка, цілком очікувано для таких, майже не відрізняються по імені, типів.

Фактично код працює коректно. Але при цьому він вводить в оману невідповідністю типів. Змінна отримує значення одного перерахування і порівнюється з константами іншого. Для виправлення помилки у цьому випадку достатньо змінити тип зміною event eOutlinerLibOpTypes.

Помилка пріоритетів операцій
static void blf_font_draw_buffer_ex (....)
{
....
cbuf[3] = (unsigned char)((alphatest = ((int)cbuf[3] + 
(int)(a * 255)) < 255) ? alphatest : 255);
....
}

V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. blf_font.c 414

Недотримання пріоритету операцій — одна з найпоширеніших помилок при роботі зі складними виразами. В даному випадку це всього лише помилка, але вона призвела до порушення логіки роботи тернарного оператора. Через неправильно поставленого дужки виникла помилка з пріоритетом операцій. До того ж псується значення змінної alphatest. Замість значення, яке обчислює тернарний оператор, змінної alphatest присвоюється значення bool-типу, отримане в результаті виконання операції порівняння (<). А вже після цього тернарний оператор, що працює зі значенням змінної alphatest, і результат його роботи не зберігається. Для виправлення помилки необхідно змінити вираз наступним чином:
cbuf[3] = (unsigned char)(alphatest = (((int)cbuf[3] +
(int)(a * 255)) < 255) ? alphatest : 255);
Помилка в константі
bool BKE_ffmpeg_alpha_channel_is_supported(RenderData *rd)
{
int codec = rd->ffcodecdata.codec;
if (codec == AV_CODEC_ID_QTRLE)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
....
} 

V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 1672, 1675. writeffmpeg.c 1675

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

Використання однією зміною в зовнішньому і вкладеному циклах
bool BM_face_exists_overlap_subset (...., const int len)
{
int i;
....
for (i = 0; i < len; i++) {
BM_ITER_ELEM (f, &viter, varr[i], BM_FACES_OF_VERT) {
if ((f->len <= len) && (....)) {
BMLoop *l_iter, *l_first;

if (is_init == false) {
is_init = true;
for (i = 0; i < len; i++) { //<=
BM_ELEM_API_FLAG_ENABLE(varr[i], _FLAG_OVERLAP);
}
}
....
}
}
}
} 

V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2204, 2212. bmesh_queries.c 2212

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

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

Подвійна перевірка
static void knife_add_single_cut (....)
{
....
if ((lh1->v && lh2->v) && //<=
(lh1->v->v && lh2->v && lh2->v->v) && //<=
(e_base = BM_edge_exists(lh1->v->v, lh2->v->v)))
{
....
return;
}
....
}

V501 There are identical sub-expressions 'lh2->v' to the left and to the right of the '&&' operator. editmesh_knife.c 781

Це один з варіантів непродуманого умови. Це, звичайно, не помилка, а зайва перевірка, але це не означає, що код не потребує доопрацювання. Умова складається з кількох виразів. При цьому частина другого виразу повністю відповідає перевірці однієї з перемінних у першому і є зайвою. Для виправлення потрібно прибрати зайву перевірку lh2->v з другого виразу. Після цього код стане набагато наочніше.

Інший приклад:
static int edbm_rip_invoke__vert (....)
{
....
if (do_fill) {
if (do_fill) {
....
}
}
....
}

V571 Recurring check. The 'if (do_fill)' condition was already verified in line 751. editmesh_rip.c 752

Ще один варіант логічної помилки. Тут два абсолютно однакових вираження перевіряються всередині зовнішнього і вкладеного умови. Повторна перевірка завжди буде видавати однаковий результат і не має сенсу. Звичайно, даний код ніяк не впливає на роботу програми. Але невідомо, як буде змінюватися код з часом, і зайві перевірки можуть збити з пантелику.

Зайві перевірки зустрічаються далеко не в одному місці проекту. Ось ще кілька місць, виявлених аналізатором:
  • V571 Recurring check. The 'but' condition was already verified in line 9587. interface_handlers.c 9590
  • V571 Recurring check. The '!me->mloopcol' condition was already verified in line 252. paint_vertex.c 253
  • V571 Recurring check. The 'constinv == 0' condition was already verified in line 5256. transform_conversions.c 5257
  • V571 Recurring check. The 'vlr->v4' condition was already verified in line 4174. convertblender.c 4176
  • V571 Recurring check. The 'ibuf == ((void *) 0)' condition was already verified in line 3557. sequencer.c 3559
А третій приклад є явним надлишковим кодом:
static void writedata_do_write (....)
{
if ((wd == NULL) || wd->error || 
(mem == NULL) || memlen < 1) return;
if (wd->error) return;
....
}

V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 331, 332. writefile.c 332

Тут рядок if (wd->error) return; просто зайва, і функція завершиться раніше, ніж буде оброблено це умова. Тому її потрібно просто прибрати.

Протилежні блоки умови
static int select_less_exec (....)
{
....
if ((lastsel==0)&&(bp->hide==0)&&(bp->f1 & SELECT)){
if (lastsel != 0) sel = 1;
else sel = 0;
.... 
} 
....
}

V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 938, 939. editcurve_select.c 938

З фрагмента вірно, що всередині зовнішнього умовного блоку розташовано додаткова умова. Вкладене умова протилежно основного і завжди видає однаковий результат, а змінна sel ніколи не отримає значення 1. Тому досить просто прописати sel = 0 без повторної перевірки. Хоча, можливо помилка повинна бути виправлена зміною однієї з умов. Я не беру участь у розробці проекту і мені складно судити, що тут до чого.

Надлишкові вираження
DerivedMesh *fluidsimModifier_do (....)
{
.... 
if (!fluidmd || (fluidmd && !fluidmd->fss))
return dm;
....
}

V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!fluidmd' and 'fluidmd'. mod_fluidsim_util.c 528

В рамках однієї умови перевіряються протилежні значення однієї і тієї ж змінної. Такі умови часто зустрічаються в різних видах і варіаціях. Вони не наносять шкоди при роботі програми, але даремно ускладнюють код. Цей вираз можна спростити і привести до наступного вигляду:
if (!fluidmd || !fluidmd->fss)) ....

Схожі місця:
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!render_only' and 'render_only'. drawobject.c 4663
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!parent' and 'parent'. kx_scene.cpp 1667
Ще один варіант такої умови:
void ED_transverts_create_from_obedit (....)
{
....
if ((tipsel && rootsel) || (rootsel)) {....}
.... 
}

V686 A pattern was detected: (rootsel) || ((rootsel) && ...). The expression is excessive or contains a logical error. ed_transverts.c 325

Як і в прикладі вище, всередині одного виразу перевіряється одна і та ж змінна двічі. Такий вираз не є помилковим, але явно перевантажена зайвою перевіркою. Для додання кодом більшої компактності і наочності його потрібно спростити.
if ((tipsel || rootsel) {....}

Подібні помилки зустрілися і в інших місцях проекту.
  • V686 A pattern was detected: (!py_b_len) || ((!py_b_len) && ...). The expression is excessive or contains a logical error. aud_pyapi.cpp 864
  • V686 A pattern was detected: (xn == 0.0 f) || ((xn == 0.0 f) && ...). The expression is excessive or contains a logical error. renderdatabase.c 993
  • V686 A pattern was detected: (xn == 0.0 f) || ((xn == 0.0 f) && ...). The expression is excessive or contains a logical error. renderdatabase.c 1115
Повторне призначення
static bool find_prev_next_keyframes (....)
{
....
do {
aknext = (ActKeyColumn *)BLI_dlrbTree_search_next(
&keys, compare_ak_cfraPtr, &cfranext);
if (aknext) {
if (CFRA == (int)aknext->cfra) {
cfranext = aknext->cfra; //<-
}
else {
if (++nextcount == U. view_frame_keyframes)
donenext = true;
}
cfranext = aknext->cfra; //<- 
}
} while ((aknext != NULL) && (donenext == false));
.... 
}

V519 The 'cfranext' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 447, 454. anim_draw.c 454

Присвоєння всередині умовних блоків не має сенсу, так як його значення все одно присвоюється заново в кінці циклу без будь-яких умов. Зробити висновок, що зайвий рядок розташована саме зверху допомагає цикл, що знаходиться в коді слідом за наведеним фрагментом. Він відрізняється тільки prev змінними і відсутністю цього рядка в умові. До того ж, якщо припустити, що зайвий рядок знизу і умова CFRA == (int)aknext->cfra виявиться помилковим, то цикл перетвориться в нескінченний. Цей фрагмент явно потребує корегування, але як саме його змінити знають тільки розробники проекту.

Зайві або невикористані змінні
Подібних фрагментів з инициализированными, але не використовуються в результаті змінними, зустрілось дуже багато. Деякі з них відносяться до прикладів логічних помилок і зайвих перевірок, про таких фрагментах вже багато разів говорилося вище. Зустрічаються також константи, які цілком можливо повинні були зміняться в межах функцій. Але в результаті залишилися лише у вигляді перевірки, завжди повертає однаковий результат. Приклад такого фрагмента:
static int rule_avoid_collision (....)
{
....
int n, сусідів = 0, nearest = 0; //<=
....
if (ptn && nearest==0) //<=
MEM_freeN(ptn);

return ret; 
} 

V560 A part of conditional expression is always true: nearest == 0. boids.c 361

Інші фрагменти я наведу просто списком. Можливо, багато з них є суперечливими, але на них варто звернути увагу.
  • V560 A part of conditional expression is always true: edit == 0. particle.c 3781
  • V560 A part of conditional expression is always true: !error. pointcache.c 154
  • V560 A part of conditional expression is always true: !error. pointcache.c 2742
  • V560 A part of conditional expression is always false: col. drawobject.c 7803
  • V560 A part of conditional expression is always false: !canvas_verts. dynamicpaint.c 4636
  • V560 A part of conditional expression is always true: (!leaf). octree.cpp 2513
  • V560 A part of conditional expression is always true: (!leaf). octree.cpp 2710
  • V560 A part of conditional expression is always false: (1 == i). basicstrokeshaders.cpp 67
  • V560 A part of conditional expression is always true: (0 == i). basicstrokeshaders.cpp 69
  • V560 A part of conditional expression is always false: (1 == i). basicstrokeshaders.cpp 84
  • V560 A part of conditional expression is always true: (0 == i). basicstrokeshaders.cpp 86
  • V560 A part of conditional expression is always false: (1 == i). basicstrokeshaders.cpp 155
  • V560 A part of conditional expression is always true: (0 == i). basicstrokeshaders.cpp 157
  • V560 A part of conditional expression is always true: (!radmod). solver_control.cpp 557
  • V560 A part of conditional expression is always true: done != 1. context.c 301
  • V560 A part of conditional expression is always true: is_tablet == false. ghost_systemwin32.cpp 665
  • V560 A part of conditional expression is always true: mesh >= 0. kx_gameobject.cpp 976
Зайва очищення списку
int TileManager::gen_tiles(bool sliced)
{
....
state.tiles.clear(); //<=
....
int tile_index = 0;

state.tiles.clear();
state.tiles.resize(num);
....
}

V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 149, 156. tile.cpp 156

У даному випадку це може бути просто зайвий рядок. Можливо, раніше між двома очищеннями списку розташовувався ще якийсь код, але в даному випадку це ще один зайвий фрагмент, який не несе користі і його треба видалити, щоб не захаращувати код. Цей рядок може бути й наслідком того, що в ній повинен був очищатися якийсь інший об'єкт, який не видно при побіжному огляді коду. У такому разі фрагмент буде явною помилкою, яка може призвести до невідомих наслідків для програми.

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

Інтрига
Команда PVS-Studio зараз активно працює над новим напрямком. А я прикриваю тили, заповнюючи інформаційне поле статтями про повторній перевірці деяких відкритих проектів. Що це за напрям? Не можу сказати. Я залишу тільки картинку, яку кожен може трактувати на свій лад.

<img src=«habrastorage.org/getpro/habr/post_images/6c4/d9d/519/6c4d9d519eb91b73daf8125733e75b45.png» alt=«Picture » 2"/>

Висновок
Аналізатор вказав досить багато проблемних місць у проекті. Втім, в Blender іноді використовується дивний стиль коду, і не можна точно трактувати такі фрагменти як помилки. Я вважаю, небезпечні помилки часто виникають з-за помилок. Саме з їх перебуванням добре справляється аналізатор PVS-Studio. Але помилки, відображені у статті, це виключно думку автора, яке досить суб'єктивно. І для повної оцінки можливостей аналізатора його варто завантажити та спробувати самостійно.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Alexander Chibisov. PVS-Studio team is about to produce a technical breakthrough, but for now let's recheck Blender.

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

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

0 коментарів

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