В очікуванні Linux версії: перевірка коду графічного редактора Inkscape

У цій статті мова піде про перевірку ще одного відомого open source проекту — векторного графічного редактора Inkscape 0.92. Проект розвивається вже понад 12 років і надає безліч можливостей по роботі з різними форматами векторних ілюстрацій. За цей час його кодова база зросла до 600 тисяч рядків, і прийшов час перевірити його за допомогою статичного аналізатора PVS-Studio.


Введення
Inkscape — це багатоплатформовий вільний векторний графічний редактор. Він широко використовується любителями і професіоналами по всьому світу для створення ілюстрацій, іконок, логотипів, діаграм, карт, а також веб-графіки. Inkscape став одним з найбільш популярних редакторів у своїй області. Проект був створений в 2003 році як форк проекту Sodipodi, і досі продовжує розвиватися. Детальніше про Inkscape можна прочитати на офіційному сайті.

Для перевірки використовувалася остання версія Inkscape — 0.92, код якої доступний репозиторії на GitHub і статичний аналізатор PVS-Studio 6.07, який можна завантажити за посилання. Правда, на момент написання статті для скачування доступна тільки PVS-Studio для Windows. Але ситуація незабаром зміниться. І можна вже заздалегідь записатися в добровольці для тестування бета-версії PVS-Studio для Linux. Подробиці можна дізнатися зі статті: "PVS-Studio зізнається в любові до Linux".



Але повернемося до помилок. Хочу зазначити, що в статті були обрані і описані найбільш цікаві повідомлення аналізатора. Для більш ретельної перевірки автори проекту зможуть отримати у нас тимчасовий ключ для PVS-Studio та звіт. Так як публічної PVS-Studio поки немає, для перегляду звіту вони зможуть скористатися інструментом PVS-Studio Standalone, що працюють під Windows. Так, це не зручно. Але прошу всіх потерпіти, залишилося не так довго до щасливого моменту виходу PVS-Studio для Linux.

Результати перевірки
Перевірка покажчика на null після new
Попередження PVS-Studio: V668 There is no sense testing in the 'outputBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 180
bool GzipInputStream::load()
{
....
outputBuf = new unsigned char [OUT_SIZE];
if ( !outputBuf ) { // <=
delete[] srcBuf;
srcBuf = NULL;
return false;
}
....
}

Згідно сучасному стандарту C++, при неможливості виділити пам'ять оператор new генерує виняток std::bad_alloc(), а не повертає nullptr. У разі, якщо систему не вдасться виділити пам'ять, буде викинуто виняток і виконання функції припиниться, отже, програма ніколи не зайде в блок після умови.

У даному випадку це може привести до витоку пам'яті. Найбільш очевидним рішенням проблеми є використання блоку try {....} catch(const std::bad_alloc &) {....}, але набагато краще замість явного звільнення пам'яті використовувати розумні покажчики (smart pointers).

Аналогічні перевірки покажчиків:

  • V668 There is no sense testing in the 'destbuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 397
  • V668 There is no sense testing in the 'srcBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 175
  • V668 There is no sense testing in the 'oldcurve' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sp-lpe-item.cpp 719


Порівняння this з нулем
Попередження PVS-Studio: V704 '!this' expression in conditional statements should be avoided — this expression is always false on newer компілятори, because 'this' pointer can never be NULL. sp-lpe-item.cpp 213

bool SPLPEItem::performPathEffect (....) {
if (!this) {
return false;
}
....
}

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

Ще одна перевірка на рівність this значення nullptr:

  • V704 'this' expression in conditional statements should be avoided — this expression is always true on newer компілятори, because 'this' pointer can never be NULL. sp-paint-server.cpp 42


Небезпечне перевизначення
Попередження PVS-Studio: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 1046, 1051. sp-mesh-array.cpp 1051

void SPMeshNodeArray::create ( ...., Geom::OptRect bbox ) // <=
{
....
if( !bbox ) {
std::cout << "SPMeshNodeArray::create(): bbox empty" 
<< std::endl;
Geom::OptRect bbox = item->geometricBounds(); // <=
}
if( !bbox ) { // <=
std::cout << "ERROR: No bounding box!" 
<< std::endl;
return;
}
....
}

За задумом автора, у випадку, коли параметр bbox дорівнює nullptr, для нього має скластися новий об'єкт типу Geom::OptRect, і якщо об'єкт створити не вдалося, то відбувається вихід з методу з повідомленням про помилку.

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

Даний код варто було б написати так:

void SPMeshNodeArray::create ( ...., Geom::OptRect bbox )
{
....
if( !bbox ) {
std::cout << "SPMeshNodeArray::create(): bbox empty" 
<< std::endl;
bbox = item->geometricBounds();
if( !bbox ) {
std::cout << "ERROR: No bounding box!" 
<< std::endl;
return;
}
}
....
}


Неправильно закомментированная рядок
Попередження PVS-Studio: V628 it's possible that the line was commented out improperly, thus altering the program's operation logics. FontFactory.cpp 705

font_instance *font_factory::Face (....)
{
....
if( features[0] != 0 ) // <=
// std::cout << " features: " << std::endl;

for( unsigned k = 0; features[k] != 0; ++k ) {
// dump_tag( &features[k], " feature: ");
++(res->openTypeTables[ extract_tag(&features[k])]);
}
....
}

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

«Одноразовий цикл»
Попередження PVS-Studio: V612 An unconditional 'break' within a loop. text_reassemble.c 417

int TR_kern_gap (....)
{ 
....
while(ptsp && tsp){
....
if(!text32){
....
if(!text32)break;
}
....
if(!ptxt32){
....
if(!ptxt32)break;
}
....
break; // <=
}
....
return(kern);
}

Цей цикл в будь-якому випадку завершиться після першого проходу, оскільки перед оператором break немає ніякого умови. Точно сказати що мав на увазі автор складно. Якщо помилки тут немає, то код все одно краще переписати і замінити while if.

Дуже дивний метод
Попередження PVS-Studio: V571 Recurring check. The 'back == false' condition was already verified in line 388. Path.cpp 389

void
Path::SetBackData (bool nVal)
{
if (back == false) {
if (nVal == true && back == false) {
back = true;
ResetPoints();
} else if (nVal == false && back == true) {
back = false;
ResetPoints();
}
} else {
if (nVal == true && back == false) {
back = true;
ResetPoints();
} else if (nVal == false && back == true) {
back = false;
ResetPoints();
}
}
}

Складно сказати, чому даний метод був написаний таким дивним чином. Блоки if else збігаються, проводиться безліч зайвих перевірок. Якщо навіть логічної помилки тут немає, то даний метод безумовно слід переписати так:

void
Path::SetBackData (bool nVal)
{
back = nVal;
ResetPoints();
}


Втрачена кома
Попередження PVS-Studio: V737 It is possible that ',' is missing comma at the end of the string. drawing-text.cpp 272
void DrawingText::decorateStyle (....)
{
....
int dashes[16]={
8, 7, 6, 5,
4, 3, 2, 1,
-8, -7, -6, -5 // <=
-4, -3, -2, -1
};
....
}

Була пропущена кома, що приводить до того, що масив dashes буде ініціалізованим першим зовсім не такими значеннями, які очікував автор.

Очікувалося:

{ 8, 7, 6, 5,
4, 3, 2, 1,
-8, -7, -6, -5,
-4, -3, -2, -1 } 

Насправді масив буде заповнено так:

{ 8, 7, 6, 5, 
4, 3, 2, 1,
-8, -7, -6, -9,
-3, -2, -1, 0 }

На місце 12-го елемента масиву буде записано число -5 — 4 == -9. А останній елемент (на який не вистачило елементів у списку ініціалізації масиву), згідно стандарту C++, ініціалізований нулем.

Невірна довжина в strncmp
Попередження PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. blend.cpp 85

static Inkscape::Filters::FilterBlendMode
sp_feBlend_readmode (....) {
....
switch (value[0]) {
case 'n':
if (strncmp(value, "normal", 6) == 0)
return Inkscape::Filters::BLEND_NORMAL;
break;
case 'm':
....
case 's':
if (strncmp(value, "screen", 6) == 0)
return Inkscape::Filters::BLEND_SCREEN;
if (strncmp(value, "saturation", 6) == 0) // <=
return Inkscape::Filters::BLEND_SATURATION;
break;
case 'd':
....
case 'o':
if (strncmp(value, "overlay", 7) == 0)
return Inkscape::Filters::BLEND_OVERLAY;
break;
case 'c':
....
case 'h':
if (strncmp(value, "hard-light", 7) == 0) // <=
return Inkscape::Filters::BLEND_HARDLIGHT;
....
break;
....
}
}

У функцію strncmp передається невірна довжина рядків «saturation» «hard-light», тому будуть порівнюватися не всі символи, а тільки перші 6 і 7 символів відповідно. Швидше за все, тут проявило себе т. н. Copy-Paste програмування. Ця помилка призведе до помилкових спрацьовувань при додаванні нових елементів в switch-case. Варто було б виправити код:
if (strncmp(value, "saturation", 10) == 0)
....
if (strncmp(value, "hard-light", 10) == 0)


Потенційне ділення на нуль
Попередження PVS-Studio: V609 Divide by zero. Denominator range [0..999]. lpe-fillet-chamfer.cpp 607

Geom::PathVector
LPEFilletChamfer::doEffect_path (....)
{
....
if (....){
....
} else if (type >= 3000 && type < 4000) {
unsigned int chamferSubs = type-3000;
....
double chamfer_stepsTime = 1.0/chamferSubs;
....
}
...
}

У разі, коли змінна type дорівнює 3000, значення змінної chamferSubs складе 0. Відповідно, значення chamfer_stepsTime буде дорівнює 1.0/0 == inf, а це явно не те, чого чекає автор. Щоб уникнути подібної ситуації варто змінити умову в блоці if:

...
else if (type > 3000 && type < 4000)
...

Або ж можна окремо обробляти ситуацію, коли chamferSubs == 0.

Аналогічна ситуація:

  • V609 Divide by zero. Denominator range [0..999]. lpe-fillet-chamfer.cpp 623


Пропущений else?
Попередження PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sp-item.cpp 204

void SPItem::resetEvaluated() 
{
if ( StatusCalculated == _evaluated_status ) {
....
} if ( StatusSet == _evaluated_status ) { // <=
....
}
}

Судячи з форматування коду (оператор if розташований на тій же рядку, що закривається, дужка від попереднього if) і логіці роботи, тут було пропущено ключове слово else:

....
if ( StatusCalculated == _evaluated_status ) {
....
} else if ( StatusSet == _evaluated_status ) {
....
}
}
....


Робота з нульовим покажчиком
Попередження PVS-Studio: V595 The 'priv' pointer was utilized before it was verified against nullptr. Check lines: 154, 160. document.cpp 154

SPDocument::~SPDocument() 
{
priv->destroySignal.emit(); // <=
....
if (oldSignalsConnected) {
priv->selChangeConnection.disconnect(); // <=
priv->desktopActivatedConnection.disconnect(); // <=
} else {
....
}
if (priv) { // <=
....
}
....
}

У нижньому блоці if відбувається перевірка priv NULL, т. к. автор допускає рівність цього покажчика нулю, однак, вище покажчик вже використовується і без жодних перевірок. Щоб виправити цю помилку слід перевірити значення покажчика до того, як його використовувати.

Аналогічні попередження:

  • V595 The 'parts' pointer was utilized before it was verified against nullptr. Check lines: 624, 641. sp-offset.cpp 624
  • V595 The '_effects_list' pointer was utilized before it was verified against nullptr. Check lines: 103, 113. effect.cpp 103
  • V595 The 'num' pointer was utilized before it was verified against nullptr. Check lines: 1312, 1315. cr-tknzr.c 1312
  • V595 The 'selector' pointer was utilized before it was verified against nullptr. Check lines: 3463, 3481. cr-parser.c 3463
  • V595 The 'a_this' pointer was utilized before it was verified against nullptr. Check lines: 1552, 1562. cr-sel-eng.c 1552
  • V595 The 'FillData' pointer was utilized before it was verified against nullptr. Check lines: 5898, 5901. upmf.c 5898
  • V595 The 'event_context' pointer was utilized before it was verified against nullptr. Check lines: 1014, 1023. tool-base.cpp 1014
  • V595 The 'event_context' pointer was utilized before it was verified against nullptr. Check lines: 959, 970. tool-base.cpp 959
  • V595 The 'this->repr' pointer was utilized before it was verified against nullptr. Check lines: 662, 665. eraser-tool.cpp 662
  • V595 The 'this->repr' pointer was utilized before it was verified against nullptr. Check lines: 662, 665. eraser-tool.cpp 662
  • V595 The 'modified_connection' pointer was utilized before it was verified against nullptr. Check lines: 1114, 1122. gradient-vector.cpp 1114
  • V595 The 'c' pointer was utilized before it was verified against nullptr. Check lines: 762, 770. freehand-base.cpp 762
  • V595 The 'release_connection' pointer was utilized before it was verified against nullptr. Check lines: 505, 511. gradient-toolbar.cpp 505
  • V595 The 'modified_connection' pointer was utilized before it was verified against nullptr. Check lines: 506, 514. gradient-toolbar.cpp 506


Пропущена крапка з комою
Попередження PVS-Studio: V504 It is highly probable that the semicolon ';' is missing after 'return' keyword. svg-fonts-dialog.cpp 167

void GlyphComboBox::update(SPFont* spfont)
{
if (!spfont) return // <=
//TODO: figure out why do we need to append("")
// before clearing items properly...

//Gtk is refusing to clear the combobox 
//when I comment out this line
this->append(""); 
this->remove_all();
}



Після return пропущена крапка з комою (";"), що і є причиною проблеми, описаної в коментарях автора. Оскільки, якщо закоментувати рядок:

this->append(""); 

то вийде конструкція виду:

if (!spfont) return this->remove_all();

Відповідно, combobox буде очищатися тільки у випадку, коли spfont == NULL.

Невикористаний параметр
Попередження PVS-Studio: V763 Parameter 'new_value' is always rewritten in function body before being used. sp-xmlview-tree.cpp 259

void element_attr_changed (.... const gchar * new_value, ....)
{
NodeData *data = static_cast<NodeData *>(ptr);
gchar *label;

if (data->tree->blocked) return;

if (0 != strcmp (key, "id") &&
0 != strcmp (key, "inkscape:label"))
return;

new_value = repr->attribute("id"); // <=
....
}

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

Аналогічна ситуація:
  • V763 Parameter 'widget' is always rewritten in function body before being used. ruler.cpp 923


Вказівник на неіснуючий масив
Попередження PVS-Studio: V507 Pointer to local array 'n' is stored outside the scope of this array. Such a pointer will become invalid. inkscape.cpp 582

void
Application::crash_handler (int /*signum*/)
{
....
if (doc->isModifiedSinceSave()) {
const gchar *docname;
....
if (docname) {
....
if (*d=='.' && d>docname && dots==2) {
char n[64];
size_t len = MIN (d - docname, 63);
memcpy (n, docname, len);
n[len] = '\0';
docname = n;
}
}
if (!docname || !*docname) docname = "emergency";
....
}

Масив n має час життя менше, ніж час життя вказівника docname на нього. Це призводить до роботи з недійсним покажчиком docname. Одним з рішень проблеми визначення масиву n поруч з покажчиком docname:

....
if (doc->isModifiedSinceSave()) {
const gchar *docname;
char n[64];
....

Аналогічні покажчики:
  • V507 Pointer to local array 'in_buffer' is stored outside the scope of this array. Such a pointer will become invalid. inkjar.cpp 371
  • V507 Pointer to local array 'out_buffer' is stored outside the scope of this array. Such a pointer will become invalid. inkjar.cpp 375


Невірне ім'я об'єкта в умові
Попередження PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 640, 643. font-variants.cpp 640

void
FontVariants::fill_css( SPCSSAttr *css ) 
{
....
if( _caps_normal.get_active() ) {
css_string = "normal";
caps_new = SP_CSS_FONT_VARIANT_CAPS_NORMAL;
} else if( _caps_small.get_active() ) {
....
} else if( _caps_all_small.get_active() ) {
....
} else if( _caps_all_petite.get_active() ) { // <=
css_string = "petite"; // <=
caps_new = SP_CSS_FONT_VARIANT_CAPS_PETITE;
} else if( _caps_all_petite.get_active() ) { // <=
css_string = "all-petite"; // <=
caps_new = SP_CSS_FONT_VARIANT_CAPS_ALL_PETITE;
} 
....
}

В умови, що йде перед _caps_all_petite.get_active(), ім'я об'єкта має бути _caps_petite, а не _caps_all_petite. Помилка швидше за все сталася в результаті Copy-Paste.

Неакуратне використання числових констант
Попередження PVS-Studio: V624 The constant 0.707107 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT1_2 constant from <math.h>. PathOutline.cpp 1198

void
Path::OutlineJoin (....)
{
....
if (fabs(c2) > 0.707107) {
....
}
....
}

Такий запис не зовсім коректна і може призвести до зменшення точності обчислень. Краще використовувати математичні константи M_SQRT1_2 (the inverse of the square root of 2), оголошену у файлі <math.h>. Думаю, на практиці тут все працює добре, але захотілося звернути увагу і на такий приклад некрасивого коду.

Аналогічні попередження:
  • V624 The constant 1.414213562 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT2 constant from <math.h>. verbs.cpp 1848
  • V624 The constant 3.14159 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. odf.cpp 1568
  • V624 The constant 1.414213562 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT2 constant from <math.h>. inkscape-preferences.cpp 1334


Ідентичні вираження
Попередження PVS-Studio: There are identical sub-expressions 'Ar.maxExtent() < tol' to the left and to the right of the '&&' operator. path-intersection.cpp 313
void mono_intersect (....)
{
if(depth > 12 || (Ar.maxExtent() < tol && Ar.maxExtent() < tol)) 
{
....
}
....
}

Перевірка умови Ar.maxExtent() < tol виконується двічі. Швидше за все це вийшло в результаті внесення будь-яких виправлень у код. Слід виправити вираз або просто прибрати дублюючу перевірку.

Аналогічна перевірка:
  • V501 There are identical sub-expressions 'Ar.maxExtent() < 0.1' to the left and to the right of the '&&' operator. path-intersection.cpp 364


Однакові дії в блоках if та else
Попередження PVS-Studio: The 'then' statement is equivalent to the 'else' statement. ShapeRaster.cpp 1825

void Shape::AvanceEdge (....)
{
....
if ( swrData[no].sens ) { 
if ( swrData[no].curX < swrData[no].lastX ) {
line->AddBord(swrData[no].curX,
swrData[no].lastX,
false);
} else if ( swrData[no].curX > swrData[no].lastX ) { 
line->AddBord(swrData[no].lastX,
swrData[no].curX,
false);
}
} else {
if ( swrData[no].curX < swrData[no].lastX ) {
line->AddBord(swrData[no].curX,
swrData[no].lastX,
false);
} else if ( swrData[no].curX > swrData[no].lastX ) {
line->AddBord(swrData[no].lastX,
swrData[no].curX,
false);
}
}
}

Код в блоках if else однаковий, тому варто переглянути це місце і виправити логіку роботи або видалити дублюючу гілку.

Аналогічні місця:
  • V523 The 'then' statement is equivalent to the 'else' statement. ShapeRaster.cpp 1795
  • V523 The 'then' statement is equivalent to the 'else' statement. PathCutting.cpp 1323
  • V523 The 'then' statement is equivalent to the 'else' statement. ShapeSweep.cpp 2340


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

Пропоную завантажити і спробувати PVS-Studio на своєму власному проекті.

p.s.


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


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Egor Bredikhin.Waiting for the Linux-version: Checking the Code of Inkscape Graphics Editor.
Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.
Джерело: Хабрахабр

0 коментарів

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