Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you haven't received our response, please do the following:
check your Spam/Junk folder and click the
"Not Spam" button for our message.
This way, you won't miss messages from our team in the future.
In this article, I talk about the analysis results for another popular open-source project, vector graphics editor Inkscape 0.92. The project has been developing for over 12 years now and provides a large number of features to work with various vector-image formats. Over this time, its code base has grown up to 600 thousand lines of code, and now is the right time to check it with PVS-Studio static analyzer.
Inkscape is a cross-platform open-source vector graphics editor. It is widely used both by amateur and professional designers all over the world to create images, icons, logos, charts, maps, and web-graphics. Inkscape has become one of the most popular tools in this field. The project was founded in 2003 as a fork of Sodipodi project and is still developing. See the official website for more information about Inkscape.
For this analysis, we used the latest version of Inkscape, 0.92, whose source codes can be downloaded from the GitHub repository, and static analyzer PVS-Studio 6.07, which can be downloaded here. Note that at the moment of writing this article, only the Windows-version of PVS-Studio is available. The situation, however, will improve soon, and you can already apply for beta testing of the Linux-version. For details see the article "PVS-Studio confesses its love for Linux".
Well, let's get back to errors. Note that I picked only the most interesting warnings to discuss in this article. To do a more thorough analysis on their own, the project authors should contact us so that we could send them a temporary PVS-Studio key and the analysis report. Since there is no public version of PVS-Studio yet, they can use the PVS-Studio Standalone tool, which runs under Windows, to view the report. True, it's inconvenient, but please be patient: the big day of the Linux-version's release is coming soon.
PVS-Studio diagnostic message: V668 There is no sense in testing 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;
}
....
}As specified by the modern C++ standard, when memory can't be allocated, the new operator throws a std::bad_alloc() exception instead of returning nullptr. If the system fails to allocate storage, an exception is thrown and the function stops executing. The program, therefore, will never enter the block of code following the condition.
In this particular case, the error may result in a memory leak. The most obvious solution is to use the try {....} catch(const std::bad_alloc &) {....} block, but a much better way is to use smart pointers instead of releasing the storage explicitly.
Other similar pointer checks:
PVS-Studio diagnostic message: V704 '!this' expression in conditional statements should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL. sp-lpe-item.cpp 213
bool SPLPEItem::performPathEffect(....) {
if (!this) {
return false;
}
....
}As specified by the modern C++ standard, the this pointer can never be null. Comparing this with zero often results in unexpected errors. For details see the description of diagnostic V704.
Another case of testing this for nullptr:
PVS-Studio diagnostic message: 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;
}
....
}The programmer wants a new object of type Geom::OptRect to be created for the bbox parameter when this parameter equals nullptr; if the object can't be created, method execution should terminate with an error message.
However, this code behaves quite differently from what the author expected. When the bbox parameter equals nullptr, a completely new bbox object is created inside the first if block and immediately destroyed on leaving that block. As a result, the second condition executes every time the first one executes, so each time the bbox parameter equals nullptr, the method terminates and an error message is issued.
That code fragment should be rewritten in the following way:
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 diagnostic message: 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])]);
}
....
}The programmer forgot to comment out the line with the condition, which was used for debugging. The mistake is fortunately harmless. It's just that the condition in the if statement simply replicates the condition of the for loop at the first iteration, but it's surely an error that may become dangerous later.
PVS-Studio diagnostic message: 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);
}This loop will terminate after the first iteration in any case, as there is no condition before the break statement. I'm not sure what the author really wanted this code to do. If there is no error here, it's still better to rewrite the code and replace while with if.
PVS-Studio diagnostic message: 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();
}
}
}It's hard to say why this method is written in such a strange way: the if and else blocks are identical and there are lots of unnecessary checks. Even if there is no logical error here, this method definitely should be rewritten:
void
Path::SetBackData (bool nVal){
back = nVal;
ResetPoints();}
PVS-Studio diagnostic message: V737 It is possible that ',' comma is missing 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
};
....
}A comma is missing, which results in initializing the dashes array to wrong values.
Expected values:
{ 8, 7, 6, 5,
4, 3, 2, 1,
-8, -7, -6, -5,
-4, -3, -2, -1 }Actual values:
{ 8, 7, 6, 5,
4, 3, 2, 1,
-8, -7, -6, -9,
-3, -2, -1, 0 }The 12-th element will be initialized to the value -5 - 4 == -9, while the last element (which has no associated value in the array initialization list) will be initialized to zero, as specified by the C++ standard.
PVS-Studio diagnostic message: 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;
....
}
}The strncmp function receives wrong lengths of strings "saturation" and "hard-light". As a result, only the first 6 and 7 characters, respectively, will be compared. This error must result from using the so called Copy-Paste programming and will cause false positives when adding new elements to switch-case. The code needs fixing:
....
if (strncmp(value, "saturation", 10) == 0)
....
if (strncmp(value, "hard-light", 10) == 0)
....PVS-Studio diagnostic message: 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;
....
}
...
}When the type variable equals 3000, the value of the chamferSubs variable will be 0. Therefore, the value of chamfer_stepsTime will be 1.0/0 == inf, which is obviously not what the programmer expected. To fix it, the condition in the if block should be changed:
...
else if (type > 3000 && type < 4000)
...Another way is to separately handle the situation when chamferSubs == 0.
Another similar issue:
PVS-Studio diagnostic message: 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 ) { // <=
....
}
}As the code formatting (the second if statement occupies the same line as the closing brace of the previous if statement) and logic suggest, the else keyword is missing:
....
if ( StatusCalculated == _evaluated_status ) {
....
} else if ( StatusSet == _evaluated_status ) {
....
}
}
....PVS-Studio diagnostic message: 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) { // <=
....
}
....
}In the lowermost if block, the priv pointer is tested for NULL as the programmer assumes that it can be NULL. However, this pointer was already used without any checks a bit earlier. This error needs to be fixed by checking the pointer before using it.
Other similar errors:
PVS-Studio diagnostic message: 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();
}
A semicolon (";") is missing after return, which is actually the cause of the problem mentioned in the comments. It happens because commenting out the line:
this->append("");results in the following construct:
if (!spfont) return this->remove_all();Therefore, the combobox will be cleared only when spfont == NULL.
PVS-Studio diagnostic message: 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"); // <=
....
}The value of the new_value parameter always changes before it is used. Perhaps this parameter should be removed from the parameter list since having it there doesn't make sense for now.
Another similar issue:
PVS-Studio diagnostic message: 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";
....
}The n array's lifetime is less than that of the docname pointer, which points to that array. This issue results in working with invalid pointer docname. One of the possible solutions is to define the n array near the docname pointer:
....
if (doc->isModifiedSinceSave()) {
const gchar *docname;
char n[64];
....Other similar errors:
PVS-Studio diagnostic message: 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;
}
....
}In the condition preceding _caps_all_petite.get_active(), the _caps_petite object name should be used instead of _caps_all_petite. This code looks like it was written using Copy-Paste.
PVS-Studio diagnostic message: 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) {
....
}
....
}This format is not quite correct and may result in loss of computational accuracy. 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 guess this code works well enough actually, but I thought I should mention it as an example of untidy code.
Other similar defects:
PVS-Studio diagnostic message: V501 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))
{
....
}
....
}The check of the Ar.maxExtent() < tol condition executes twice, which seems to be a result of some modifications made in the code. The authors need either to fix the expression or simply remove the duplicate check.
Another similar check:
PVS-Studio diagnostic message: V523 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 if and else blocks contain the same code, so the authors need to examine this fragment and either fix the logic or remove the duplicate branch.
Other similar issues:
This analysis revealed a lot of programmer mistakes caused by lack of attention. PVS-Studio static analyzer is very good at detecting such errors, which helps save programmers' time and nerves. The most important thing about static analysis is that it should be done regularly, so that the tool can catch typos and other defects as soon as they appear. One-time checks, like this one, are good for promoting PVS-Studio but aren't really effective. Think of static analyzer warnings as extended compiler warnings, and compiler warnings are something you want to deal with all the time, not just once before a release. I hope any programmer who cares about their code's quality can relate to this analogy.
Welcome to download PVS-Studio and try it with your own projects.
0
0
0
0