Waiting for the Linux-version: Checking the Code of Inkscape Graphics Editor

• Introduction
• Analysis results
• Testing a pointer for null after new
• Comparing this with zero
• Dangerous parameter redefinition
• Incorrectly commented-out line
• "One-time" loop
• Very odd method
• Lost comma
• Wrong length in strncmp
• Potential division by zero
• Missing else?
• Using a null pointer
• Missing semicolon
• Unused parameter
• Pointer to a nonexistent array
• Incorrect object name in a condition
• Careless use of numeric constants
• Identical expressions
• Identical operations in if and else blocks
• Conclusion
• P.S.

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.

Introduction

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.

Analysis results

Testing a pointer for null after new

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:

• V668 There is no sense in testing 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 in testing 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 in testing 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

Comparing this with zero

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:

• V704 'this' expression in conditional statements should be avoided - this expression is always true on newer compilers, because 'this' pointer can never be NULL. sp-paint-server.cpp 42

Dangerous parameter redefinition

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

Incorrectly commented-out line

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.

"One-time" loop

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.

Very odd method

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();

}

Lost comma

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.

Wrong length in strncmp

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)
....

Potential division by zero

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:

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

Missing else?

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 ) {
      ....
  }
}
....

Using a null pointer

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:

• 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

Missing semicolon

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.

Unused parameter

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:

• 763 Parameter 'widget' is always rewritten in function body before being used. ruler.cpp 923

Pointer to a nonexistent array

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:

• 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

Incorrect object name in a condition

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.

Careless use of numeric constants

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:

• 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

Identical expressions

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:

• V501 There are identical sub-expressions 'Ar.maxExtent() < 0.1' to the left and to the right of the '&&' operator. path-intersection.cpp 364

Identical operations in if and else blocks

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:

• 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

Conclusion

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.

P.S.