Andrey Karpov

Aug 15 2014

Tags:

#Cpp

Checking GIMP's Source Code with PVS-Studio

Aug 15 2014

Author: Andrey Karpov

GIMP
Analysis results
Conclusion

To check GIMP, we should first find a way to get it compile successfully. This task is far from easy, that's why we had been constantly delaying the check. However, the project is too famous, and we were very interested to find out its quality. So we have conquered our laziness and completed the analysis.

GIMP

I don't like GIMP's interface, though I do use this graphics editor from time to time. It doesn't make sense to purchase Photoshop only for editing the image of our unicorn for another article a few times in a month; Paint and GIMP will do quite well.

I can't call myself a user experienced enough to reasonably judge about convenience. But you don't have to be a carpenter or a furniture expert to tell when nails sticking up from a chair make it uncomfortable to sit on. So I can point out a few defects in GIMP that bother me when working with it. For instance, when opening a file, you cannot paste a complete file path in the Location field if the path contains Russian letters. And there are quite a lot of other similar defects.

Too well familiar with the clumsy GIMP's interface, I expected to find a bunch of bugs in the code. But I was wrong. The project developers appear to have been using static analysis for some time already. And what they use is heavy artillery – one of the most powerful static analyzers, Coverity.

It was mentioned on the Internet:

The Coverity project established with the support of the USA government and focusing on detecting programming errors in open source programs, announces that 100 open source graphics software projects will be included in their SCAN project for source code analysis among which are Scribus, GIMP, Inkscape, Krita, Blender, and many others (from a 2007 publication).

Analysis results

Let's see if we can find anything of interest in GIMP's code after it has been cleaned out by Coverity. Analysis was done by PVS-Studio 5.18.

Fragments No. 1 – No. 3

typedef double gdouble;

GimpBlob *
gimp_blob_square (gdouble xc,
                  gdouble yc,
                  gdouble xp,
                  gdouble yp,
                  gdouble xq,
                  gdouble yq)
{
  GimpBlobPoint points[4];

  /* Make sure we order points ccw */
  if (xp * yq - yq * xp < 0)
  {
    xq = -xq;
    yq = -yq;
  }
  ....
}

PVS-Studio's diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '-' operator: xp * yq - yq * xp gimpink-blob.c 162

The "xp * yq - yq * xp" expression is very strange. The value "xp*yq" is subtracted from itself.

Similar checks can be found a bit further in this file. Look for lines 195 and 278.

Fragment No. 4

gint64 gimp_g_value_get_memsize (GValue *value)
{
  ....
  GimpArray *array = g_value_get_boxed (value);

  if (array)
    memsize += (sizeof (GimpArray) +
                array->static_data ? 0 : array->length);
  ....
}

PVS-Studio's diagnostic message: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. gimp-utils.c 233

There is a mess in operator precedence. 0 or "array->length" must be added to the size of some object. But the '+' operator's priority is higher than that of '?:'. The expression therefore will execute in the following way:

memsize += ((sizeof (GimpArray) + array->static_data) ?
            0 : array->length);

The programmer seems to have known about it, that's why he used parentheses. But then one of them is in a wrong place. The correct code should look as follows:

memsize += sizeof (GimpArray) +
           (array->static_data ? 0 : array->length);

Fragments No. 5, No. 6

#define cmsFLAGS_NOOPTIMIZE 0x0100
#define cmsFLAGS_BLACKPOINTCOMPENSATION 0x2000

static void
lcms_layers_transform_rgb (...., gboolean bpc)
{
  ....
  transform = cmsCreateTransform (
    src_profile,  lcms_format,
    dest_profile, lcms_format,
    intent,
    cmsFLAGS_NOOPTIMIZE |
    bpc ? cmsFLAGS_BLACKPOINTCOMPENSATION : 0);
  ....
}

PVS-Studio's diagnostic message: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. lcms.c 1016

Depending on the 'bpc' variable, the function should receive either the "cmsFLAGS_BLACKPOINTCOMPENSATION" flag or a combination of flags "cmsFLAGS_BLACKPOINTCOMPENSATION | cmsFLAGS_NOOPTIMIZE".

The '|' operator's priority is higher than that of the ternary operator '?:'. As a result, the '?:' operator has the "cmsFLAGS_NOOPTIMIZE | bpc" expression as its condition. And this condition is always true. The function always receives the cmsFLAGS_BLACKPOINTCOMPENSATION flag.

The correct code should look like this:

transform = cmsCreateTransform (
  src_profile,  lcms_format,
  dest_profile, lcms_format,
  intent,
  cmsFLAGS_NOOPTIMIZE |
  (bpc ? cmsFLAGS_BLACKPOINTCOMPENSATION : 0));

The same error can be found in lcms.c 1016.

Fragment No. 7

static gint load_resource_lrfx (....)
{
  ....
  else if (memcmp (effectname, "oglw", 4) == 0)  <<<===
  ....
  else if (memcmp (effectname, "iglw", 4) == 0)
  ....
  else if (memcmp (effectname, "oglw", 4) == 0)  <<<===
  ....
  else if (memcmp (effectname, "bevl", 4) == 0)
  ....
}

PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 602, 688. psd-layer-res-load.c 602

Two identical conditions in the if-elseif-elseif-... sequence.

Fragment No. 8

void
gimp_text_get_transformation (GimpText    *text,
                              GimpMatrix3 *matrix)
{
  g_return_if_fail (GIMP_IS_TEXT (text));
  g_return_if_fail (matrix != NULL);

  matrix->coeff[0][0] = text->transformation.coeff[0][0];
  matrix->coeff[0][1] = text->transformation.coeff[0][1];
  matrix->coeff[0][2] = text->offset_x;

  matrix->coeff[1][0] = text->transformation.coeff[1][0];
  matrix->coeff[1][1] = text->transformation.coeff[1][1];
  matrix->coeff[1][2] = text->offset_y;

  matrix->coeff[2][0] = 0.0;
  matrix->coeff[2][1] = 0.0;
  matrix->coeff[2][1] = 1.0;     <<<===
}

PVS-Studio's diagnostic message: V519 The 'matrix->coeff[2][1]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 567, 568. gimptext.c 568

The Last Line Effect. In the very end, an incorrect index is used. It should be like this:

matrix->coeff[2][0] = 0.0;
matrix->coeff[2][1] = 0.0;
matrix->coeff[2][2] = 1.0;

Fragment No. 9

static void warp_one (....)
{
  ....
  if (first_time)
    gimp_pixel_rgn_init (&dest_rgn,
                         new, x1, y1, (x2 - x1), (y2 - y1),
                         TRUE, TRUE);
  else
    gimp_pixel_rgn_init (&dest_rgn,
                         new, x1, y1, (x2 - x1), (y2 - y1),
                         TRUE, TRUE);
  ....
}

PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. warp.c 1366

It is very suspicious that one and the same branch is executed regardless of the condition.

Fragments No. 10, No. 11, No. 12

gboolean gimp_wire_read (GIOChannel *channel,
  guint8     *buf,
  gsize       count,
  gpointer    user_data)
{
  g_return_val_if_fail (count >= 0, FALSE);
  ....
}

PVS-Studio's diagnostic message: V547 Expression 'count >= 0' is always true. Unsigned type value is always >= 0. gimpwire.c 99

The "count >= 0" check doesn't make sense as the 'count' variable is unsigned. Perhaps it's not a serious bug but I still should mention it.

Similar checks: gimpwire.c 170; gimpcageconfig.c 428.

Below we will discuss more interesting issues found through the V547 diagnostic.

Fragment No. 13

static GimpPlugInImageType
image_types_parse (const gchar *name,
                   const gchar *image_types)
{
  ....
  while (*image_types &&
         ((*image_types != ' ') ||
          (*image_types != '\t') ||
          (*image_types != ',')))
    {
      image_types++;
    }
  ....
}

PVS-Studio's diagnostic message: V547 Expression is always true. Probably the '&&' operator should be used here. gimppluginprocedure.c 808

To make it clearer, I've made an artificial example:

int A = ...;
if ( A != 1  ||  A != 2  ||  A != 3)

Regardless of the value the A variable takes, the condition is always true.

Fragment No. 14

static gunichar basic_inchar(port *pt) {
  ....
  gunichar c;
  ....
  c = g_utf8_get_char_validated(pt->rep.string.curr, len);

  if (c >= 0)   /* Valid UTF-8 character? */
  {
    len = g_unichar_to_utf8(c, NULL);
    pt->rep.string.curr += len;
    return c;
  }

  /* Look for next valid UTF-8 character in buffer */
  pt->rep.string.curr = g_utf8_find_next_char(
                          pt->rep.string.curr,
                          pt->rep.string.past_the_end);
  ....
}

PVS-Studio's diagnostic message: V547 Expression 'c >= 0' is always true. Unsigned type value is always >= 0. scheme.c 1654

All the characters will be treated as correct UTF-8 characters. The 'c' variable is unsigned, so the (c >= 0) condition is always true.

Fragment No. 15

#define ABS(a)     (((a) < 0) ? -(a) : (a))

static gint32
load_thumbnail (...., gint32 thumb_size, ....)
{
  ....
  guint32 size;
  guint32 diff;
  ....
  diff = ABS(thumb_size - size);
  ....
}

PVS-Studio's diagnostic message: V547 Expression '(thumb_size - size) < 0' is always false. Unsigned type value is never < 0. file-xmc.c 874

The program works differently than the programmer expected. Suppose the 'thumb_size' variable equals 10 and the 'size' variable equals 25.

It may seem at first that the expression will evaluate to 15. But actually the result will be 0xFFFFFFF1 (4294967281).

The "thumb_size - size" expression is unsigned. As a result, we'll get number 0xFFFFFFF1u. The ABS macro doesn't do anything in this case.

Fragment No. 16

static gchar *
script_fu_menu_map (const gchar *menu_path)
{
  ....
  const gchar *suffix = menu_path + strlen (mapping[i].old);
  if (! *suffix == '/')
    continue;
  ....
}

PVS-Studio's diagnostic message: V562 It's odd to compare 0 or 1 with a value of 47: !* suffix == '/'. script-fu-scripts.c 859

Another trouble with operator precedence. First, the "!*suffix" expression is calculated. Its result is either 0 or 1. This number is then compared to the '/' character, which doesn't make any sense at all.

The correct code:

if (*suffix != '/')

Fragment No. 17

static void
save_file_chooser_response (GtkFileChooser *chooser,
                            gint            response_id,
                            GFigObj        *obj)
{
  ....
  gfig_context->current_obj = obj;
  gfig_save_callbk ();
  gfig_context->current_obj = gfig_context->current_obj;  
  ....
}

PVS-Studio's diagnostic message: V570 The 'gfig_context->current_obj' variable is assigned to itself. gfig-dialog.c 1623

The variable is copied into itself.

Fragment No. 18

size g_strlcpy(gchar *dest, const gchar *src, gsize dest_size);

GList * gimp_brush_generated_load (....)
{
  ....
  gchar *string;
  ....
  /* the empty string is not an allowed name */
  if (strlen (string) < 1)
    g_strlcpy (string, _("Untitled"), sizeof (string));
  ....
}

PVS-Studio's diagnostic message: V579 The g_strlcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. gimpbrushgenerated-load.c 119

The "sizeof(string)" operator calculates the pointer size, not the buffer size.

Fragment No. 19

static gboolean save_image (....)
{
  ....
  gint c;
  ....
  if (has_alpha && (data[rowoffset + k + 1] < 128))
    c |= 0 << (thisbit ++);
  else
  ....   
}

PVS-Studio's diagnostic message: V684 A value of the variable 'c' is not modified. Consider inspecting the expression. It is possible that '1' should be present instead of '0'. file-xbm.c 1136

The "c |= 0 << (thisbit ++);" expression doesn't change the 'c' variable.

I've noticed that code like that is very likely to be found when the programmer wanted to zero out a certain bit but made a mistake. Then the code should look as follows:

c &= ~(1u << (thisbit ++));

Fragment No. 20

gboolean gimp_item_get_popup_size (....,
    gint *popup_width, gint *popup_height)
{
  ....
  if (scaling_up)
  {
    *popup_width = gimp_item_get_width  (item);
    *popup_width = gimp_item_get_height (item);
  }
  ....
}

PVS-Studio's diagnostic message: V537 Consider reviewing the correctness of 'popup_width' item's usage. gimpitem-preview.c 126

This is a typo or a consequence of the Copy-Paste technique. The correct code:

*popup_width = gimp_item_get_width (item);
*popup_height = gimp_item_get_height (item);

Fragment No. 21

gboolean gimp_draw_tool_on_vectors_curve (....,
  GimpAnchor       **ret_segment_start,
  GimpAnchor       **ret_segment_end,
  ....)
{
  ....
  if (ret_segment_start) *ret_segment_start = NULL;
  if (ret_segment_start) *ret_segment_end   = NULL;
  ....
}

PVS-Studio's diagnostic message: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 1212, 1213. gimpdrawtool.c 1213

This is a typo or a consequence of the Copy-Paste technique. The correct code:

if (ret_segment_start) *ret_segment_start = NULL;
if (ret_segment_end) *ret_segment_end = NULL;

Fragments No. 22 – No. 40

ObjectList_t*
object_list_append_list(ObjectList_t *des, ObjectList_t *src)
{
   GList *p;
   for (p = src->list; p; p = p->next)
      object_list_append(des, object_clone((Object_t*) p->data));
   object_list_set_changed(des, (src) ? TRUE : FALSE);
   return des;
}

PVS-Studio's diagnostic message: V595 The 'src' pointer was utilized before it was verified against nullptr. Check lines: 536, 538. imap_object.c 536

You may conclude from the "(src) ? TRUE : FALSE" condition that the 'src' pointer may be equal to nullptr.

However, this pointer is bravely dereferenced in the "p = src->list" expression a bit earlier, which is an error.

There are other fragments that triggered the V595 warning as well. They also need checking:

The 'l' pointer. Check lines: 262, 265. gimpimage-item-list.c 262
The 'quantobj' pointer. Check lines: 965, 971. gimpimage-convert-type.c 965
The 'slist' pointer. Check lines: 683, 685. gimpfont.c 683
The 'dock_window->p->context' pointer. Check lines: 487, 504. gimpdockwindow.c 487
The 'layer_renderer' pointer. Check lines: 1245, 1275. gimplayertreeview.c 1245
The 'shell->display' pointer. Check lines: 574, 588. gimpdisplayshell-dnd.c 574
The 'ops' pointer. Check lines: 265, 267. gimpgegltool.c 265
The 'dialog' pointer. Check lines: 234, 249. file-save-dialog.c 234
The 'shell' pointer. Check lines: 738, 763. view-actions.c 738
The 'fname' pointer. Check lines: 1426, 1437. scheme.c 1426
The 'sgip->table' pointer. Check lines: 148, 161. sgi-lib.c 148
The 'sgip->length' pointer. Check lines: 154, 167. sgi-lib.c 154
The 'pixels' pointer. Check lines: 1482, 1508. psd-load.c 1482
The 'img_a->alpha_names' pointer. Check lines: 1735, 1741. psd-load.c 1735
The 'brush' pointer. Check lines: 432, 451. brush.c 432
The 'curve_list->data' pointer. Check lines: 126, 129. curve.c 126
The 'outline_list->data' pointer. Check lines: 183, 187. pxl-outline.c 183
The 'id_ptr' pointer. Check lines: 896, 898. sample-colorize.c 896

Conclusion

It's not easy to tell how critical the bugs found in this project are. But I will be pleased if some of them will be fixed thanks to this article.

Although I told you in the beginning that I don't like GIMP's interface, I'm still thankful to its authors for their project. Quite a number of images for my articles were made in GIMP. Thank you.

#Cpp