Анализатор PVS-Studio продолжает осваивать платформу Linux. Посмотрим, какие ошибки он смог обнаружить в загрузчике операционных систем Grub.
Статья расскажет о проверке программы-загрузчика для операционных систем семейства Unix - Grub. Grub разработан Erich Boleyn и входит в проект GNU. GRUB является эталонной реализацией загрузчика, соответствующего спецификации Multiboot, и может загрузить любую совместимую с ней операционную систему.
Проект Grub написан на языке С. Он уже проверялся другими анализаторами, в том числе Coverity. В таком проекте достаточно сложно найти непроверенные места. Однако, анализатор PVS-Studio справился с этой задачей и обнаружил несколько интересных ошибок.
Опечатки являются одной из самых распространённых ошибок в программах. От них не застрахованы даже профессиональные разработчики. С опечаток и начнём.
Опечатка в имени константы
typedef enum
{
GRUB_PARSER_STATE_TEXT = 1,
GRUB_PARSER_STATE_ESC,
GRUB_PARSER_STATE_QUOTE,
GRUB_PARSER_STATE_DQUOTE,
....
} grub_parser_state_t;
char * grub_normal_do_completion (....)
{
....
if (*escstr == ' '
&& cmdline_state != GRUB_PARSER_STATE_QUOTE
&& cmdline_state != GRUB_PARSER_STATE_QUOTE) // <=
*(newstr++) = '\\';
....
}Предупреждение: V501 There are identical sub-expressions 'cmdline_state != GRUB_PARSER_STATE_QUOTE' to the left and to the right of the '&&' operator. completion.c 502
Опечатки в именах констант, близких по написанию, - достаточно частое явление. Наверняка и в этом примере, вместо повторного сравнения значения cmdline_state с константой GRUB_PARSER_STATE_QUOTE, требовалось сверить его с константой GRUB_PARSER_STATE_DQUOTE.
Опечатка в имени регистра
struct grub_bios_int_registers
{
grub_uint32_t eax;
grub_uint16_t es;
grub_uint16_t ds;
grub_uint16_t flags;
grub_uint16_t dummy;
grub_uint32_t ebx;
grub_uint32_t ecx;
grub_uint32_t edi;
grub_uint32_t esi;
grub_uint32_t edx;
};
grub_vbe_status_t
grub_vbe_bios_getset_dac_palette_width (....)
{
struct grub_bios_int_registers regs;
regs.eax = 0x4f08;
regs.ebx = (*dac_mask_size & 0xff) >> 8;
regs.ebx = set ? 1 : 0; // <=
....
}Предупреждение: V519 The 'regs.ebx' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 312, 313. vbe.c 313
Структура regs является обёрткой для работы с регистрами памяти. Учитывая близость наименований регистров, очень легко допустить ошибку. Вероятнее всего, во втором случае, вместо ebx должен быть использован какой-то другой регистр. Точно указать, как именно исправить ошибку, не зная особенностей кода, невозможно. Главная цель анализатора - указать на проблемное место, а решить, как именно его следует исправить, разработчик сможет сам. Именно поэтому статический анализ наиболее актуален непосредственно во время разработки проектов.
Бессмысленное присваивание
static void free_subchunk (....)
{
switch (subchu->type)
{
case CHUNK_TYPE_REGION_START:
{
grub_mm_region_t r1, r2, *rp;
....
if (*rp)
{
....
}
else
{
r1->pre_size = pre_size;
r1->size = (r2 - r1) * sizeof (*r2);
for (rp = &grub_mm_base; *rp; rp = &((*rp)->next))
if ((*rp)->size > r1->size)
break;
r1->next = *rp; // <=
*rp = r1->next; // <=
h = (grub_mm_header_t) (r1 + 1);
r1->first = h;
h->next = h;
h->magic = GRUB_MM_FREE_MAGIC;
h->size = (r2 - r1 - 1);
}
....
if (r2)
{
....
r2->size += r1->size;
....
hl2->next = r2->first;
r2->first = r1->first;
hl->next = r2->first;
*rp = (*rp)->next;
....
}
....
}
....
}
....
}Предупреждение: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 338, 339. relocator.c 339
Подобные ошибки встречаются не так часто. Сложно судить, что именно предполагалось написать в данном случае. В коде происходит присваивание полю указателя, хранящегося в переменной *rp. А в следующей строке происходит обратная операция. И уже переменной *rp присваивается указатель r1->next. Подобный код не имеет смысла, так как переменная *rp уже хранит это значение. Рассматривая код, точно определить, есть здесь ошибка или это просто лишняя операция, практически невозможно. Всё же, я считаю, что это ошибка.
Некорректный аргумент memset
static void setup (....)
{
....
struct grub_boot_blocklist *first_block, *block;
....
/* Clean out the blocklists. */
block = first_block;
while (block->len)
{
grub_memset (block, 0, sizeof (block)); // <=
block--;
....
}
....
}Предупреждение: V579 The grub_memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. grub-setup.c 500
Функции для низкоуровневой работы с памятью служат рассадником для опечаток. Во время работы с ними часто неверно рассчитывают размер буфера. Вот и в данном примере вместо размера буфера block функция grub_memset получает в качестве третьего аргумента размер указателя. В результате буфер block будет очищен не полностью. Исправить код надо следующим образом:
grub_memset (block, 0, sizeof (*block));Ещё несколько мест, где встретилась похожая ошибка:
Некорректная очистка памяти
static gcry_err_code_t do_arcfour_setkey (....)
{
byte karr[256];
....
for (i=0; i < 256; i++ )
karr[i] = key[i%keylen];
....
memset( karr, 0, 256 ); // <=
return GPG_ERR_NO_ERROR;
}Предупреждение: V597 The compiler could delete the 'memset' function call, which is used to flush 'karr' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. arcfour.c 108
Использование здесь функции memset для обнуления памяти некорректно. В ситуации, приведённой в этом фрагменте, сразу после вызова функции memset происходит выход из функции. В случае, если буфер более не используется, компилятор может удалить функцию во время сборки. Чтобы этого избежать, необходимо использовать функцию memset_s.
В проекте нашлось ещё несколько предупреждений, связанных с затиранием памяти:
Лишняя операция
Int main (int argc, char *argv[])
{
....
{
FILE *f;
size_t rd;
f = fopen ("/dev/urandom", "rb");
if (!f)
{
memset (pass1, 0, sizeof (pass1));
free (buf);
free (bufhex);
free (salthex);
free (salt);
fclose (f); // <=
....
}
....
fclose (f);
}
....
}Предупреждение: V575 The null pointer is passed into 'fclose' function. Inspect the first argument. grub-mkpasswd-pbkdf2.c 184
В случае неудачного открытия файла происходит очистка временных переменных. Помимо этого, в условный блок затесалась функция fclose, предназначенная для закрытия файла. Проверка в выражении условного блока показывает, что файл открыт не был. Следовательно, закрывать его незачем. Согласно документации, в случае отправки в функцию значения NULL происходит вызов обработчика недопустимых параметров. Дальнейшее поведение программы зависит от настроек обработчика. Приведённый код в любом случае является ошибочным и для его исправления требуется удалить вызов функции fclose внутри условного оператора.
Ещё одно подозрительное место, найденное диагностикой V575:
Неиспользуемое значение
static grub_err_t grub_video_cirrus_setup (....)
{
....
if (CIRRUS_APERTURE_SIZE >= 2 * framebuffer.page_size)
err = grub_video_fb_setup (mode_type, mode_mask,
&framebuffer.mode_info,
framebuffer.ptr,
doublebuf_pageflipping_set_page,
framebuffer.ptr + framebuffer.page_size);
else
err = grub_video_fb_setup (mode_type, mode_mask,
&framebuffer.mode_info,
framebuffer.ptr, 0, 0);
err = grub_video_cirrus_set_palette (0,
GRUB_VIDEO_FBSTD_NUMCOLORS,
grub_video_fbstd_colors);
return err;
}Предупреждение: V519 The 'err' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 448, 460. cirrus.c 460
В сущности, в этом фрагменте нет серьёзной ошибки. Видимо предполагается, что функция grub_video_fb_setup не может вернуть ошибку. Если это так, то зачем заносить её результат в переменную, если сразу после этого значение перезаписывается. Можно предположить, что переменную просто используют для проверки значения при отладке. Но, возможно, тут действительно забыли добавить важную проверку. В любом случае, на это место стоит обратить внимание и скорректировать код.
Ещё одно подозрительное место:
Даже в хорошо проверенных проектах встречаются ошибки. Статический анализ полезен на любом этапе развития проекта. Ну а пока PVS-Studio for Linux готовится к выходу, можно почитать о результатах проверок других проектов.