# Что скрывает загрузчик GNU? Ищем ошибки в Grub Анализатор PVS\-Studio продолжает осваивать платформу Linux\. Посмотрим, какие ошибки он смог обнаружить в загрузчике операционных систем Grub\. ![0432_Grub_ru/image1.png](https://import.viva64.com/docx/blog/0432_Grub_ru/image1.png) ## Введение Статья расскажет о проверке программы\-загрузчика для операционных систем семейства Unix \- Grub\. Grub разработан Erich Boleyn и входит в проект GNU\. GRUB является эталонной реализацией загрузчика, соответствующего [спецификации Multiboot](https://ru.wikipedia.org/wiki/Multiboot_Specification), и может загрузить любую совместимую с ней операционную систему\. Проект Grub написан на языке С\. Он уже проверялся другими анализаторами, в том числе Coverity\. В таком проекте достаточно сложно найти непроверенные места\. Однако, анализатор PVS\-Studio справился с этой задачей и обнаружил несколько интересных ошибок\. ## Результаты проверки Опечатки являются одной из самых распространённых ошибок в программах\. От них не застрахованы даже профессиональные разработчики\. С опечаток и начнём\. **Опечатка в имени константы** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_\. **Опечатка в имени регистра** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'regs\.ebx' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 312, 313\. vbe\.c 313 Структура _regs_ является обёрткой для работы с регистрами памяти\. Учитывая близость наименований регистров, очень легко допустить ошибку\. Вероятнее всего, во втором случае, вместо _ebx_ должен быть использован какой\-то другой регистр\. Точно указать, как именно исправить ошибку, не зная особенностей кода, невозможно\. Главная цель анализатора \- указать на проблемное место, а решить, как именно его следует исправить, разработчик сможет сам\. Именно поэтому статический анализ наиболее актуален непосредственно во время разработки проектов\. **Бессмысленное присваивание** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 Функции для низкоуровневой работы с памятью [служат](https://pvs-studio.ru/ru/blog/posts/cpp/0360/) рассадником для опечаток\. Во время работы с ними часто неверно рассчитывают размер буфера\. Вот и в данном примере вместо размера буфера _block_ функция _grub\_memset_ получает в качестве третьего аргумента размер указателя\. В результате буфер _block_ будет очищен не полностью\. Исправить код надо следующим образом: ```cpp grub_memset (block, 0, sizeof (*block)); ``` Ещё несколько мест, где встретилась похожая ошибка: * V579 The grub\_memset function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. mmap\.c 148 * V579 The grub\_memset function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. mmap\.c 165 **Некорректная очистка памяти** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_\. В проекте нашлось ещё несколько предупреждений, связанных с затиранием памяти: * V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 209 * V597 The compiler could delete the 'memset' function call, which is used to flush 'bufhex' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 210 * V597 The compiler could delete the 'memset' function call, which is used to flush 'salt' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 213 * V597 The compiler could delete the 'memset' function call, which is used to flush 'salthex' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 214 * V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 231 * V597 The compiler could delete the 'memset' function call, which is used to flush 'bufhex' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 232 * V597 The compiler could delete the 'memset' function call, which is used to flush 'salt' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 235 * V597 The compiler could delete the 'memset' function call, which is used to flush 'salthex' object\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 236 * V597 The compiler could delete the 'memset' function call, which is used to flush 'pass2' buffer\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 166 * V597 The compiler could delete the 'memset' function call, which is used to flush 'pass1' buffer\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. grub\-mkpasswd\-pbkdf2\.c 205 **Лишняя операция** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'fclose' function\. Inspect the first argument\. grub\-mkpasswd\-pbkdf2\.c 184 В случае неудачного открытия файла происходит очистка временных переменных\. Помимо этого, в условный блок затесалась функция _fclose_, предназначенная для закрытия файла\. Проверка в выражении условного блока показывает, что файл открыт не был\. Следовательно, закрывать его незачем\. Согласно документации, в случае отправки в функцию значения _NULL_ происходит вызов обработчика недопустимых параметров\. Дальнейшее поведение программы зависит от настроек обработчика\. Приведённый код в любом случае является ошибочным и для его исправления требуется удалить вызов функции _fclose_ внутри условного оператора\. Ещё одно подозрительное место, найденное диагностикой V575: * V575 The null pointer is passed into 'free' function\. Inspect the first argument\. grub\-setup\.c 1187 **Неиспользуемое значение** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_ не может вернуть ошибку\. Если это так, то зачем заносить её результат в переменную, если сразу после этого значение перезаписывается\. Можно предположить, что переменную просто используют для проверки значения при отладке\. Но, возможно, тут действительно забыли добавить важную проверку\. В любом случае, на это место стоит обратить внимание и скорректировать код\. Ещё одно подозрительное место: * V519 The 'err' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 368, 380\. bochs\.c 380 ## Заключение Даже в хорошо проверенных проектах встречаются ошибки\. Статический анализ полезен на любом этапе развития проекта\. Ну а пока PVS\-Studio for Linux готовится к выходу, можно почитать о результатах проверок [других проектов](https://pvs-studio.ru/ru/blog/inspections/)\.