Geekly Articles each Day
📜 ⬆️ ⬇️

Waiting for Linux version: checking the code of the graphical editor Inkscape

In this article, we will discuss the verification of another well-known open source project - the vector graphics editor Inkscape 0.92. The project has been developing for more than 12 years and provides many opportunities to work with various formats of vector illustrations. During this time, its codebase has grown to 600 thousand lines, and the time has come to check it with the help of the PVS-Studio static analyzer.


Introduction


Inkscape is a free, cross-platform vector graphics editor. It is widely used by amateurs and professionals around the world to create illustrations, icons, logos, diagrams, maps, as well as web graphics. Inkscape has become one of the most popular editors in its field. The project was created in 2003 as a fork of the Sodipodi project, and still continues to evolve. More information about Inkscape can be found on the official website .

The latest version of Inkscape - 0.92, the code of which is available in the repository on GitHub and the static analyzer PVS-Studio 6.07, which can be downloaded at the link, was used for verification. True, at the time of writing the article, only PVS-Studio for Windows is available for download. But the situation will change soon. And you can already sign up for volunteers in advance to test the PVS-Studio beta version for Linux. Details can be found in the article: " PVS-Studio confesses his love for Linux ".
')


But back to the mistakes. I want to note that in the article the most interesting messages of the analyzer were selected and described. For a more thorough check, the project authors will be able to get a temporary key for PVS-Studio and a report from us. Since there is no public PVS-Studio yet, they can use the PVS-Studio Standalone tool running under Windows to view the report. Yes, it is not convenient. But I ask everyone to suffer, it remains not so long until the happy release of PVS-Studio for Linux.

Test results


Check pointer to null after new


PVS-Studio warning : V668, it’s not a problem. 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; } .... }

According to the modern C ++ standard, if it is impossible to allocate memory, the new operator throws an exception std :: bad_alloc () , and does not return nullptr . If the system fails to allocate memory, an exception will be thrown and the function will stop executing, therefore, the program will never enter the block after the condition.

In this case, it may lead to a memory leak. The most obvious solution to the problem is to use the try {....} catch (const std :: bad_alloc &) {....} block , but it is much better to use smart pointers instead of explicitly freeing memory.

Similar pointer tests:



Comparing this with zero


PVS-Studio warning : V704 '! This' expression can be compiled, because it can never be NULL. sp-lpe-item.cpp 213

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

According to the modern standard C ++, the this pointer can never be null. Often, using this with a null comparison can lead to unexpected errors. Read more about this in the diagnostic description of the V704 .

Another check for this being equal to nullptr :



Dangerous parameter override


PVS-Studio warning : V581 The conditional expressions of the 'if' are agreed 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; } .... }

As conceived by the author, in the case where the bbox parameter is nullptr , a new object of type Geom :: OptRect should be created for it, and if the object could not be created, then the method will exit with an error message.

However, the code does not work as expected by the author. When the bbox parameter is nullptr , inside the first if block, a completely new bbox object is created , which is immediately destroyed when it leaves the block. The result is that the second condition is always executed when the first condition is fulfilled, so every time the bbox parameter is nullptr , the method exits with an error message.

This code should be written like this:

 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; } } .... }


Incorrectly commented out line


PVS-Studio Warning: V628 It’s possible that you’ve been commented out, 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])]); } .... }

Here the author forgot to comment out the line with the condition used for debugging. In this case, lucky and it did not lead to negative consequences. It turned out that the condition in if simply duplicated the condition of the for loop execution at the first iteration, but this is undoubtedly a mistake and can lead to problems in the future.

"One-time cycle"


PVS-Studio warning : 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); }

In any case, this cycle will end after the first pass, since there is no condition before the break statement. Just say what the author meant is difficult. If there is no error, it is still better to rewrite the code and replace the while with if .

Very strange method


PVS-Studio warning : V571 Recurring check. The 'back == false' condition has been 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(); } } }

It is difficult to say why this method was written in such a strange way. The if and else blocks are the same; many unnecessary checks are performed. Even if there is no logical error here, then this method should definitely be rewritten as follows:

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


Lost comma


PVS-Studio warning : V737 It is possible that ',' the comma is missing. 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 }; .... }

A comma was omitted, which causes the dashes array to be initialized at all in ways other than those expected by the author.

Expected:

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

In fact, the array will be filled like this:

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

In the place of the 12th element of the array will be written the number -5 - 4 == -9 . And the last element (for which there were not enough elements in the array initialization list) will be, according to the C ++ standard, initialized to zero.

Invalid length in strncmp


PVS-Studio warning : V666 Consider the third argument of the function 'strncmp'. This is not the case. 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; .... } }

An incorrect length of “saturation” and “hard-light” strings is passed to the strncmp function , so not all characters will be compared, but only the first 6 and 7 characters, respectively. Most likely, the so-called Copy-Paste programming . This error will lead to false positives when adding new elements to the switch-case . It would be worth fixing the code:
 if (strncmp(value, "saturation", 10) == 0) .... if (strncmp(value, "hard-light", 10) == 0)


Potential division by zero


PVS-Studio warning : 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; .... } ... }

In the case when the type variable is equal to 3000 , the value of the variable chamferSubs will be 0. Accordingly, the value of chamfer_stepsTime will be 1.0 / 0 == inf , and this is clearly not what the author expects. To avoid this situation, it is worth changing the condition in the if block:

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

Or you can separately handle the situation when chamferSubs == 0 .

Similar situation:



Missed else?


PVS-Studio warning : 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 ) { // <= .... } }

Judging by the formatting of the code (the if statement is located on the same line as the closing bracket from the previous if ) and the logic of the work, the else keyword was omitted here:

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


Work with a null pointer


PVS-Studio warning : V595 The 'priv' pointer was used 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) { // <= .... } .... }

In the lower if block, the priv is checked for NULL , since The author admits the equality of this pointer to zero, however, the above pointer is already used without any checks. To correct this error, you should check the pointer value before using it.

Similar warnings:



Missed semicolon


PVS-Studio warning : 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(); }



After the return, a semicolon (";") is omitted, which is the cause of the problem described in the author's comments. Because, if you comment out the line:

  this->append("");

then we get the construction of the form:

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

Accordingly, the combobox will only be cleared when spfont == NULL .

Unused parameter


PVS-Studio warning : 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"); // <= .... }

In this function, the value of the new_value parameter always changes before it is used. It may be worth removing new_value from the parameter list, since At the moment, the presence of this parameter is absolutely not justified.

Similar situation:


Pointer to a non-existent array


PVS-Studio warning : V507 Pointer to local 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"; .... }

The array n has a lifetime less than the lifetime of the docname pointer to it. This leads to working with an invalid docname pointer. One solution to the problem is to define the array n next to the docname pointer:

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

Similar pointers:


Invalid object name in condition


PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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; } .... }

In the condition preceding _caps_all_petite.get_active () , the object name must be _caps_petite , not _caps_all_petite . The error most likely occurred as a result of Copy-Paste.

Careless use of numeric constants


PVS-Studio warning : 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) { .... } .... }

Such a record is not entirely correct and may lead to a decrease in the accuracy of the calculations. It is better to use the mathematical constant M_SQRT1_2 (the inverse of the square root of 2) declared in the file <math.h> . I think in practice everything works well here, but I wanted to draw attention to such an example of ugly code.

Similar warnings:


Identical expressions


PVS-Studio Warning: Ar.maxExtent () <tol 'of the operator. path-intersection.cpp 313
 void mono_intersect(....) { if(depth > 12 || (Ar.maxExtent() < tol && Ar.maxExtent() < tol)) { .... } .... }

The verification of the condition Ar.maxExtent () <tol is performed twice. Most likely it turned out as a result of making some corrections to the code. You should correct the expression or simply remove the duplicate check.

Similar check:

Same actions in if and else blocks


PVS-Studio warning: 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); } } }

The code in the if and else blocks is the same, so you should look at this place and either fix the logic of the work or delete the duplicate branch.

Similar places:


Conclusion


During the test, a lot of mistakes were made due to inattention. The PVS-Studio static analyzer can effectively detect such errors and thus save time and nerves of the programmer. The main thing is to perform code analysis regularly in order to immediately identify typos and other defects. One-time checks such as this one, although they advertise PVS-Studio well, are ineffective. Consider messages from the static analyzer as extended warnings from the compiler. And with the compiler messages you need to work all the time, not one-time before release. I hope this analogy is close and understandable to the soul of any programmer who is worried about the quality of the code.

I suggest downloading and trying PVS-Studio on my own project.

PS




Our company decided to experiment a bit with Instagram. We do not know whether something will come out of this, but I invite everyone to follow " pvsstudio ".


If you want to share this article with an English-speaking audience, then please use the link to the translation: Egor Bredikhin. Waiting for the Linux version: Checking the Code of the Inkscape Graphics Editor .
Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/307786/

More articles:


All Articles