Проверяем исходный код 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 0×0100 #define cmsFLAGS_BLACKPOINTCOMPENSATION 0×2000
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) Программа работает не так, как задумывал программист. Предположим, что переменная '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.
