Андрей Карпов

15 Авг 2014

Теги:

#Cpp

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

15 Авг 2014

Автор: Андрей Карпов

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. Спасибо.

#Cpp