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

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

    GIMP


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

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

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

    О том, что используется Coverity, я сужу по упоминаниям в интернете:

    Проект 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.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Comments 18

      +15
      g_strlcpy (string, _("Untitled"), sizeof (string));

      Ох, помню я как-то такой баг вылавливал пол-дня. Соль в том, что здесь стоит незаметный символ локализации, который в зависимости от текущей локали подставит перевод строки «Untitled». В результате из-за того, что русские строки занимают в UTF-8 кодировке по два байта на символ, гарантированно возникает переполнение буфера, причём у разработчика, естественно, «всё нормально», а у клиента с русской локалью — сегфолт. Понять, что баг зависит от локали порой очень сложно.

      Хотя конкретно здесь мы максимум получим «обрубок» русского текста, причём возможно между символов.

      P.S. По поводу интерфейса гимпа на хабре была неплохая статья, его можно настроить под себя.
        +5
        Давно ждал эту статью.

        Я сам как-то решил написать небольшую модификацию Gimp, и поэтому разбирал его код в течении недели. Могу смело сказать, исходный код Gimp-а — это неописуемый кошмар, просто ****ц какой-то. Сказывается тот факт, что разработчиков этого редактора мало, но при этом каждый имеет свою «уникальную» стилистику написания кода. Код этого проекта похож на какое-то «спагетти из нескольких видов макарон с несколькими соусами». Но при всём это ужасе этот код работает и довольно не плохо.
        Кстати, модификацию я так не написал — не смог разобраться в зависимостях отдельных модулей.
          +2
          Качество кода и удобство интерфейса не всегда связаны, увы.
          И похоже, что вы своим анализатором очень хорошо дополняете «тяжелую артиллерию» благодаря нахождению мест, которые с точки зрения языка, инициализации, нулевых указателей и переполнения корректны, но при этом все же содержат в себе wtf-фактор вроде c |= 0 << (thisbit ++);
            0
            А свой логотип вы в чём рисовали?
            +4
            >Например, при открытии файла нельзя вставить полный путь до файла в поле Location, если путь содержит буквы на русском языке
            Это точно не глюк GTK под Windows, например?
            У меня в Убунте Гимп 2.8.10, и в нём в меню Open Image вообще нет такой опции — пусть вставлять. Для этого есть специальный пункт Open Location, который русские буквы сожрал и не подавился.
              +3
              Да есть там у вас такая опция. Ctrl+L нажмите — будет строчка. Правда нонче она не появляется если стоять на папке «Recently used» (нет, ну серьёзно: кому оно мешало?), сначала нужно кликнуть по какой-нибудь другой папке, потом нажать Ctrl+L, потом можно вставлять. Русские буквы в Linux'е вставляются исправно, скорее всего какой-то глюк GTK для Windows…
            • UFO just landed and posted this here
                0
                Эх, клевый инструмент вы сделали, очень хочется натравить его на свои проекты, но когда ж будет версия под операционные системы…
                  +1
                  GIMP проверялся под операционной системой, так как под Windows собрать его затруднительно. Напишите нам от имени компании, и мы обсудим варианты сотрудничества. Мы можем заключить контракт на аудит вашего кода или интеграцию PVS-Studio в процесс разработки (адаптацию для конкурентного Linux т.д.). Никаких публичных версий. Только контрактные работы.
                    0
                    У меня масштабы не такие. :) Хотя я очень за то, чтобы заплатить за инструменты аудита, но ментальная нагрузка будет велика при таком серъезном подходе.

                    Если не секрет, почему не выпускаете публичных версий для линукса/мака?
                      +1
                      Если не секрет, почему не выпускаете публичных версий для линукса/мака?


                      Спроса нет.
                        0
                        Вот как. Т.е. все, кто под линуксом и маком — используют clang и вполне им довольны?..
                          +1
                          Не готовы платить почему-то…
                        0
                        Решили рискнуть, вложиться и попробовать: PVS-Studio для Linux.
                          0
                          Ничего себе!

                  Only users with full accounts can post comments. Log in, please.