# Проверяем исходный код GIMP с помощью PVS\-Studio Чтобы проверить GIMP, для начала нужно научиться его компилировать\. Это непростая задача, из\-за которой поверка несколько раз откладывалась\. Однако, проект известный, и интересно оценить качество исходного кода\. Поэтому лень была побеждена, и проект проанализирован\. ![0273_GIMP_ru/image1.png](https://import.viva64.com/docx/blog/0273_GIMP_ru/image1.png) ## GIMP Мне не нравится интерфейс [GIMP](https://www.gimp.org/), хотя иногда я использую этот графический редактор\. Нет смысла покупать Photoshop только для того, чтобы несколько раз в месяц адаптировать картинку с единорогом для очередной статьи\. Программ Paint и GIMP мне более чем достаточно\. Можно сказать, что я не достаточно профессиональный пользователь, чтобы судить об удобстве\. Однако, чтобы утверждать, что на стуле неудобно сидеть из\-за торчащих гвоздей, вовсе не обязательно быть плотником или экспертом по мебели\. Я могу перечислить ряд недоделок в GIMP, которые мне мешают\. Например, при открытии файла нельзя вставить полный путь до файла в поле Location, если путь содержит буквы на русском языке\. Таких недочётов много\. Будучи знакомым с корявым интерфейсом GIMP, я ожидал, что в коде я найду много ляпов\. Я ошибался\. Оказывается, разработчики уже применяют статический анализ в своей работе\. Причем, используют тяжёлую артиллерию\. Применяется один из самых мощных статических анализаторов \- Coverity\. О том, что используется Coverity, я сужу по упоминаниям в интернете: _Проект Coverity, организованный при поддержке американского правительства и занимающийся поиском ошибок в программах с открытым кодом, сообщает, что в список проверяемых проектов войдут около 100 приложений для работы с графикой, в числе которых популярные Scribus, GIMP, Inkscape, Krita, Blender и многие другие \(из публикации в 2007 году\)\._ ## Результаты проверки Давайте посмотрим, что можно найти в коде GIMP после Coverity\. Анализ был выполнен с помощью [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) версии 5\.18\. **Фрагмент N1\-N3** ```cpp 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** ```cpp 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"\. Но приоритет оператора '\+' выше, чем оператора '?:'\. Выражение работает следующим образом: ```cpp memsize += ((sizeof (GimpArray) + array->static_data) ? 0 : array->length); ``` Возможно, программист знал об этом, поэтому использовал скобки\. Но тогда, скобки стоят не на своём месте\. Правильный вариант: ```cpp memsize += sizeof (GimpArray) + (array->static_data ? 0 : array->length); ``` **Фрагмент N5, N6** ```cpp #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\. Правильный вариант: ```cpp transform = cmsCreateTransform ( src_profile, lcms_format, dest_profile, lcms_format, intent, cmsFLAGS_NOOPTIMIZE | (bpc ? cmsFLAGS_BLACKPOINTCOMPENSATION : 0)); ``` Аналогичную ошибку можно найти здесь: lcms\.c 1016\. **Фрагмент N7** ```cpp 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** ```cpp 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 [Эффект последней строки](https://pvs-studio.ru/ru/blog/posts/cpp/0260/)\. В самом конце, используется неправильный индекс\. Должно быть: ```cpp matrix->coeff[2][0] = 0.0; matrix->coeff[2][1] = 0.0; matrix->coeff[2][2] = 1.0; ``` **Фрагмент N9** ```cpp 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** ```cpp 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** ```cpp 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 Для наглядности поясню на искусственном примере: ```cpp int A = ...; if ( A != 1 || A != 2 || A != 3) ``` Чему бы ни была равна переменная A, условие всегда выполняется\. **Фрагмент N14** ```cpp 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** ```cpp #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** ```cpp 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\. Этот ноль или единица сравнивается с символом '/', что не имеет никакого смысла\. Правильный вариант: ```cpp if (*suffix != '/') ``` **Фрагмент N17** ```cpp 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** ```cpp 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** ```cpp 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'\. По моему наблюдению, такой код можно встретить, когда хотели сбросить определённый бит, но ошиблись\. В этом случае код должен выглядеть так: ```cpp c &= ~(1u << (thisbit ++)); ``` **Фрагмент N20** ```cpp 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\. Правильный вариант: ```cpp *popup_width = gimp_item_get_width (item); *popup_height = gimp_item_get_height (item); ``` **Фрагмент N21** ```cpp 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\. Правильный вариант: ```cpp if (ret_segment_start) *ret_segment_start = NULL; if (ret_segment_end) *ret_segment_end = NULL; ``` **Фрагмент N22\-N40** ```cpp 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\. Спасибо\.