Checking GIMP source code with PVS-Studio

To test GIMP, first you need to learn how to compile it. This is not an easy task, because of which verification has been postponed several times. However, the project is well-known, and it is interesting to evaluate the quality of the source code. Therefore, laziness was defeated, and the project was analyzed.

Gimp

I do not like the GIMP interface, although sometimes I use this graphic editor. It makes no sense to buy Photoshop only to adapt a picture with a unicorn several times a month for another article. Paint and GIMP programs are more than enough for me.

It can be said that I am not a professional user enough to judge convenience. However, to argue that it is inconvenient to sit on a chair because of protruding nails, it is not at all necessary to be a carpenter or furniture expert. I can list a number of deficiencies in the GIMP that bother me. For example, when opening a file, you cannot insert the full path to the file in the Location field if the path contains letters in Russian. There are many such shortcomings.

Being familiar with the clumsy GIMP interface, I expected that in the code I would find a lot of blunders. I was wrong. It turns out that developers are already using static analysis in their work. Moreover, use heavy artillery. One of the most powerful static analyzers is used - Coverity.
')
The fact that the use of Coverity, I judge by the mention on the Internet:

The Coverity project, organized with the support of the US government and engaged in finding errors in open source programs, reports that the list of checked projects will include about 100 applications for working with graphics, including popular Scribus, GIMP, Inkscape, Krita, Blender and many others (from the publication in 2007).

Test results

Let's see what can be found in the GIMP code after Coverity. The analysis was performed using PVS-Studio version 5.18.

Fragment N1-N3

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 warning: V501 '-' operator: xp * yq - yq * xp gimpink-blob.c 162

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

Identical checks can be found just below in this file. Lines: 195 and 278.

Fragment N4

 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 warning: V502 Perhaps the '?:' Operator works in a different way. The '?:' Operator has a lower limit than the '+' operator. gimp-utils.c 233

Confusion in the priorities of operators. To the size of a certain object you need to add 0 or “array-> length”. But the priority of the operator '+' is higher than the operator '?:'. The expression works as follows:

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

Perhaps the programmer was aware of this, so he used brackets. But then, the brackets are not in place. The correct option is:

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

Fragment N5, N6

 #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 warning: V502 Perhaps the '?:' Operator works in a different way. The '?:' Operator has a lower than the '|' operator. lcms.c 1016

Depending on the variable 'bpc', the function must be passed the flag "cmsFLAGS_BLACKPOINTCOMPENSATION" or the combination of the flags "cmsFLAGS_BLACKPOINTCOMPENSATION | cmsFLAGS_NOOPTIMIZE.

The priority of the operator '|' higher than the priority of the ternary operator '?:'. As a result, the condition for the '?:' Operator is the expression “cmsFLAGS_NOOPTIMIZE | bpc. This condition is always true. The cmsFLAGS_BLACKPOINTCOMPENSATION flag is always passed to the function.

The correct option is:

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

A similar error can be found here: lcms.c 1016.

Fragment N7

 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 warning: V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 602, 688. psd-layer-res-load.c 602

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

Fragment N8

 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 warning: 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 effect of the last line . At the very end, the wrong index is used. It should be:

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

Fragment N9

 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 warning: V523 The 'then' statement is equivalent to the 'else' statement. warp.c 1366

Suspiciously, regardless of the condition, the same action is performed.

Fragment N10, N11, N12

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

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

The “count> = 0” check does not make sense, since the 'count' variable is unsigned. Perhaps this is not a serious mistake, but it is worth mentioning it.

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

More interesting cases found using the V547 diagnostic will be discussed below.

Fragment N13

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

Warning: V547 Expression is always true. Probably the '&&' operator should be used here. gimppluginprocedure.c 808

For clarity, I will explain on an artificial example:

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

Whatever the variable A is, the condition is always satisfied.

Fragment N14

 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 warning: V547 Expression 'c> = 0' is always true. Unsigned type value is always> = 0. Scheme.c 1654

All characters will be considered valid UTF-8 characters. The variable 'c' is of unsigned type. Therefore, the condition (c> = 0) is always true.

Fragment N15

 #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 warning: V547 Expression '(thumb_size - size) <0' is always false. Unsigned type value is never <0. file-xmc.c 874

The program does not work as the programmer intended. Suppose that the variable 'thumb_size' is equal to 10, and the variable 'size' is equal to 25.

At first glance, it seems that the result of the calculation will be the value 15. In fact, the result will be 0xFFFFFFF1 (4294967281).

The expression "thumb_size - size" has an unsigned type. As a result, we get the number 0xFFFFFFF1u. The ABS macro does nothing in this case.

Fragment N16

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

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

Again trouble with the priority of operations. First, the expression "! * Suffix" is evaluated. As a result, we get 0 or 1. This zero or one is compared with the '/' symbol, which makes no sense.

The correct option is:

 if (*suffix != '/') 

Fragment N17

 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 warning: V570 The 'gfig_context-> current_obj' variable is assigned to itself. gfig-dialog.c 1623

The variable is copied to itself.

Fragment N18

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

Warning PVS_Studio: V579 It is possibly a mistake. Inspect the third argument. gimpbrushgenerated-load.c 119

The “sizeof (string)” operator computes the size of the pointer, not the size of the buffer.

Fragment N19

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

PVS-Studio warning: 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 expression “c | = 0 << (thisbit ++);” does not change the value of the variable 'c'.

According to my observation, such code can be found when they wanted to reset a certain bit, but they were mistaken. In this case, the code should look like this:

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

Fragment N20

 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 warning: V537 Consider reviewing the correctness of 'popup_width' item's usage. gimpitem-preview.c 126

A typo or consequence of Copy-Paste. The correct option is:

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

Fragment N21

 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 warning: V581 The conditional expressions of the 'if' are agreed alongside each other are identical. Check lines: 1212, 1213. gimpdrawtool.c 1213

A typo or consequence of Copy-Paste. The correct option is:

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

Fragment N22-N40

 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 warning: V595 The 'src' pointer was used before it was verified against nullptr. Check lines: 536, 538. imap_object.c 536

From the condition "(src)? TRUE: FALSE" it can be concluded that the pointer 'src' can be equal to nullptr.

However, above this pointer is boldly dereferenced in the expression “p = src-> list”, which is an error.

There are other places where V595 warning is issued. These places should also be checked:

• 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 is difficult to judge the seriousness of the errors found. I would be pleased if something was corrected thanks to the article.

Although, as I said, I don’t like the GIMP interface, I’m very grateful to the developers for their work. I made quite a few pictures for the articles in GIMP. Thank.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Checking GIMP's Source Code with PVS-Studio .