Проверяем исходный код GIMP с помощью PVS-Studio

Чтобы проверить GIMP, для начала нужно научиться его компилировать. Это непростая задача, из-за которой поверка несколько раз откладывалась. Однако, проект известный, и интересно оценить качество исходного кода. Поэтому лень была побеждена, и проект проанализирован.

GIMP

Мне не нравится интерфейс GIMP, хотя иногда я использую этот графический редактор. Нет смысла покупать Photoshop только для того, чтобы несколько раз в месяц адаптировать картинку с единорогом для очередной статьи. Программ Paint и GIMP мне более чем достаточно.

Можно сказать, что я не достаточно профессиональный пользователь, чтобы судить об удобстве. Однако, чтобы утверждать, что на стуле неудобно сидеть из-за торчащих гвоздей, вовсе не обязательно быть плотником или экспертом по мебели. Я могу перечислить ряд недоделок в GIMP, которые мне мешают. Например, при открытии файла нельзя вставить полный путь до файла в поле Location, если путь содержит буквы на руссом языке. Таких недочётов много.

Будучи знакомым с корявым интерфейсом GIMP, я ожидал, что в коде я найду много ляпов. Я ошибался. Оказывается, разработчики уже применяют статический анализ в своей работе. Причем, используют тяжёлую артиллерию. Применяется один из самых мощных статических анализаторов — Coverity.

О том, что используется Coverity, я сужу по наличию в проекте такого файла как «coverity.sh». Плюс упоминание про это можно найти в интернете:

Проект Coverity, организованный при поддержке американского правительства и занимающийся поиском ошибок в программах с открытым кодом, сообщает, что в список проверяемых проектов войдут около 100 приложений для работы с графикой, в числе которых популярные Scribus, GIMP, Inkscape, Krita, Blender и многие другие (из публикации в 2007 году).

Результаты проверки

Давайте посмотрим, что можно найти в коде GIMP после Coverity. Анализ был выполнен с помощью PVS-Studio версии 5.18.

Фрагмент 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: V501 There are identical sub-expressions to the left and to the right of the ‘-‘ operator: xp * yq — yq * xp gimpink-blob.c 162

Выражение «xp * yq — yq * xp» очень странное. Значение «xp*yq» вычитается само из себя.

Идентичные проверки можно найти чуть ниже в этом файле. Строки: 195 и 278.

Фрагмент 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: 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

Путаница в приоритетах операторов. К размеру некого объекта нужно прибавить 0 или «array->length». Но приоритет оператора ‘+’ выше, чем оператора ‘?:’. Выражение работает следующим образом:

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

Возможно, программист знал об этом, поэтому использовал скобки. Но тогда, скобки стоят не на своём месте. Правильный вариант:

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

Фрагмент 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: 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

В зависимости от переменной ‘bpc’ в функцию требуется передать флаг «cmsFLAGS_BLACKPOINTCOMPENSATION» или сочетание флагов «cmsFLAGS_BLACKPOINTCOMPENSATION | cmsFLAGS_NOOPTIMIZE».

Приоритет оператора ‘|’ выше, чем приоритет тернарного оператора ‘?:’. В результате, условием для оператора ‘?:’ является выражение «cmsFLAGS_NOOPTIMIZE | bpc». Это условие всегда истинно. В функцию всегда передаётся флаг cmsFLAGS_BLACKPOINTCOMPENSATION.

Правильный вариант:

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

Аналогичную ошибку можно найти здесь: lcms.c 1016.

Фрагмент 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: 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

Два одинаковых условия в последовательности if-elseif-elseif-…

Фрагмент 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: V519 The ‘matrix->coeff[2][1]’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 567, 568. gimptext.c 568

Эффект последней строки. В самом конце, используется неправильный индекс. Должно быть:

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

Фрагмент 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: V523 The ‘then’ statement is equivalent to the ‘else’ statement. warp.c 1366

Подозрительно, что независимо от условия выполняется одно и то же действие.

Фрагмент 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: V547 Expression ‘count >= 0’ is always true. Unsigned type value is always >= 0. gimpwire.c 99

Проверка «count >= 0» не имеет смысла, так как переменная ‘count’ является беззнаковой. Возможно, это не серьезная ошибка, но упомянуть о ней стоит.

Аналогичные проверки: gimpwire.c 170; gimpcageconfig.c 428.

Более интересные случаи, найденные с помощью диагностики V547, будут рассмотрены ниже.

Фрагмент 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++;     }   .... }

Предупреждение: V547 Expression is always true. Probably the ‘&&’ operator should be used here. gimppluginprocedure.c 808

Для наглядности поясню на искусственном примере:

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

Чему бы ни была равна переменная A, условие всегда выполняется.

Фрагмент 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: V547 Expression ‘c >= 0’ is always true. Unsigned type value is always >= 0. scheme.c 1654

Все символы будут считаться корректными UTF-8 символами. Переменная ‘c’ имеет беззнаковый тип. Следовательно, условие (c >= 0) всегда истинно.

Фрагмент 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: V547 Expression ‘(thumb_size — size) < 0’ is always false. Unsigned type value is never < 0. file-xmc.c 874

Программа работает не так, как задумывал программист. Предположим, что переменная ‘thumb_size’ равна 10, а переменная ‘size’ равна 25.

На первый взгляд кажется, что результатом вычислений будет значение 15. На самом деле, результат будет равен 0xFFFFFFF1 (4294967281).

Выражение «thumb_size — size» имеет беззнаковый тип. В результате, мы получим число 0xFFFFFFF1u. Макрос ABS в данном случае ничего не делает.

Фрагмент 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: V562 It’s odd to compare 0 or 1 with a value of 47: !* suffix == ‘/’. script-fu-scripts.c 859

Вновь беда с приоритетом операций. Вначале вычисляется выражение "!*suffix". В результате, получаем 0 или 1. Этот ноль или единица сравнивается с символом ‘/’, что не имеет никакого смысла.

Правильный вариант:

if (*suffix != '/')

Фрагмент 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: V570 The ‘gfig_context->current_obj’ variable is assigned to itself. gfig-dialog.c 1623

Переменная копируется сама в себя.

Фрагмент 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));   .... }

Предупреждение PVS_Studio: 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

Оператор «sizeof(string)» вычисляет размер указателя, а не размер буфера.

Фрагмент N19

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

Предупреждение PVS-Studio: 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

Выражение «c |= 0 << (thisbit ++);» не изменяет значение переменной ‘c’.

По моему наблюдению, такой код можно встретить, когда хотели сбросить определённый бит, но ошиблись. В этом случае код должен выглядеть так:

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

Фрагмент 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: V537 Consider reviewing the correctness of ‘popup_width’ item’s usage. gimpitem-preview.c 126

Опечатка или последствие Copy-Paste. Правильный вариант:

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

Фрагмент 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: V581 The conditional expressions of the ‘if’ operators situated alongside each other are identical. Check lines: 1212, 1213. gimpdrawtool.c 1213

Опечатка или последствие Copy-Paste. Правильный вариант:

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

Фрагмент 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: V595 The ‘src’ pointer was utilized before it was verified against nullptr. Check lines: 536, 538. imap_object.c 536

Из условия "(src)? TRUE: FALSE" можно сделать вывод, что указатель ‘src’ может быть равен nullptr.

Однако, выше этот указатель смело разыменовывают в выражении «p = src->list», что является ошибкой.

Есть и другие места, где выдано предупреждение V595. Эти места следует также проверить:

• 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

Заключение

Трудно судить о серьезности найденных ошибок. Мне будет приятно, если благодаря статье будет что-то исправлено.

Хотя, как я говорил, мне не нравится интерфейс GIMP, я очень благодарен разработчикам за их работу. Немало картинок к статьям я сделал именно в GIMP. Спасибо.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking GIMP’s Source Code with PVS-Studio.