to the top
close form

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Шаг 1
Поздравляем! У вас есть промокод!

Тип желаемой лицензии:

Шаг 2
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности
close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Проверяем исходный код GIMP с помощью P…

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

15 Авг 2014

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

0273_GIMP_ru/image1.png

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

Популярные статьи по теме
64-битные ошибки: LONG, LONG_PTR и привет из прошлого

Дата: 09 Мар 2023

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

В целом, 64-битные ошибки - дело минувших дней. Мало кто сейчас занимается портированием кода с 32-битной на 64-битную систему. Кому это было нужно, уже портировали свои приложения. Кому не нужно, то…
Приключения капитана Блада: потонет ли Арабелла?

Дата: 14 Фев 2023

Автор: Владислав Столяров

Недавно в сети появилась новость о том, что был открыт исходный код игры "Приключения капитана Блада". Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Потонет ли легендарный к…
Тонкости C++: итак, вы объявили класс…

Дата: 07 Фев 2023

Автор: Сергей Ларин

Во время работы наша команда постоянно сталкивается с некоторыми особенностями языка, которые могут быть неизвестны рядовому C++ программисту. В этой статье мы расскажем о том, как работает, казалось…
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…


Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо