Проверяем исходный код GIMP с помощью PVS-Studio
Чтобы проверить 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. Спасибо.
0