Сразу предупреждаю, мои вкусы очень специфичны. Красота ошибки в том, что человеку её очень сложно найти. Я не верю, что её можно заметить при обзоре кода. Если только заранее знать, что она есть, и искать её целенаправленно.
Ошибку я нашёл в проекте DPDK. В нём есть и другие ошибки, но про них потом. Они меркнут перед этим алмазом. Только не ждите чего-то эдакого. Ошибка проста до безобразия. Вот только найти её, просматривая код, ой как непросто. Собственно, попробуйте сами.
Для этого сначала изучите список именованных констант:
enum dbg_status { DBG_STATUS_OK, DBG_STATUS_APP_VERSION_NOT_SET, DBG_STATUS_UNSUPPORTED_APP_VERSION, DBG_STATUS_DBG_BLOCK_NOT_RESET, DBG_STATUS_INVALID_ARGS, DBG_STATUS_OUTPUT_ALREADY_SET, DBG_STATUS_INVALID_PCI_BUF_SIZE, DBG_STATUS_PCI_BUF_ALLOC_FAILED, DBG_STATUS_PCI_BUF_NOT_ALLOCATED, DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS, DBG_STATUS_NO_MATCHING_FRAMING_MODE, DBG_STATUS_VFC_READ_ERROR, DBG_STATUS_STORM_ALREADY_ENABLED, DBG_STATUS_STORM_NOT_ENABLED, DBG_STATUS_BLOCK_ALREADY_ENABLED, DBG_STATUS_BLOCK_NOT_ENABLED, DBG_STATUS_NO_INPUT_ENABLED, DBG_STATUS_NO_FILTER_TRIGGER_256B, DBG_STATUS_FILTER_ALREADY_ENABLED, DBG_STATUS_TRIGGER_ALREADY_ENABLED, DBG_STATUS_TRIGGER_NOT_ENABLED, DBG_STATUS_CANT_ADD_CONSTRAINT, DBG_STATUS_TOO_MANY_TRIGGER_STATES, DBG_STATUS_TOO_MANY_CONSTRAINTS, DBG_STATUS_RECORDING_NOT_STARTED, DBG_STATUS_DATA_DIDNT_TRIGGER, DBG_STATUS_NO_DATA_RECORDED, DBG_STATUS_DUMP_BUF_TOO_SMALL, DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED, DBG_STATUS_UNKNOWN_CHIP, DBG_STATUS_VIRT_MEM_ALLOC_FAILED, DBG_STATUS_BLOCK_IN_RESET, DBG_STATUS_INVALID_TRACE_SIGNATURE, DBG_STATUS_INVALID_NVRAM_BUNDLE, DBG_STATUS_NVRAM_GET_IMAGE_FAILED, DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE, DBG_STATUS_NVRAM_READ_FAILED, DBG_STATUS_IDLE_CHK_PARSE_FAILED, DBG_STATUS_MCP_TRACE_BAD_DATA, DBG_STATUS_MCP_TRACE_NO_META, DBG_STATUS_MCP_COULD_NOT_HALT, DBG_STATUS_MCP_COULD_NOT_RESUME, DBG_STATUS_RESERVED0, DBG_STATUS_SEMI_FIFO_NOT_EMPTY, DBG_STATUS_IGU_FIFO_BAD_DATA, DBG_STATUS_MCP_COULD_NOT_MASK_PRTY, DBG_STATUS_FW_ASSERTS_PARSE_FAILED, DBG_STATUS_REG_FIFO_BAD_DATA, DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA, DBG_STATUS_DBG_ARRAY_NOT_SET, DBG_STATUS_RESERVED1, DBG_STATUS_NON_MATCHING_LINES, DBG_STATUS_INSUFFICIENT_HW_IDS, DBG_STATUS_DBG_BUS_IN_USE, DBG_STATUS_INVALID_STORM_DBG_MODE, DBG_STATUS_OTHER_ENGINE_BB_ONLY, DBG_STATUS_FILTER_SINGLE_HW_ID, DBG_STATUS_TRIGGER_SINGLE_HW_ID, DBG_STATUS_MISSING_TRIGGER_STATE_STORM, MAX_DBG_STATUS };
Теперь изучите массив строк:
static const char * const s_status_str[] = { /* DBG_STATUS_OK */ "Operation completed successfully", /* DBG_STATUS_APP_VERSION_NOT_SET */ "Debug application version wasn't set", /* DBG_STATUS_UNSUPPORTED_APP_VERSION */ "Unsupported debug application version", /* DBG_STATUS_DBG_BLOCK_NOT_RESET */ "The debug block wasn't reset since the last recording", /* DBG_STATUS_INVALID_ARGS */ "Invalid arguments", /* DBG_STATUS_OUTPUT_ALREADY_SET */ "The debug output was already set", /* DBG_STATUS_INVALID_PCI_BUF_SIZE */ "Invalid PCI buffer size", /* DBG_STATUS_PCI_BUF_ALLOC_FAILED */ "PCI buffer allocation failed", /* DBG_STATUS_PCI_BUF_NOT_ALLOCATED */ "A PCI buffer wasn't allocated", /* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */ "The filter/trigger constraint dword offsets are not " "enabled for recording", /* DBG_STATUS_VFC_READ_ERROR */ "Error reading from VFC", /* DBG_STATUS_STORM_ALREADY_ENABLED */ "The Storm was already enabled", /* DBG_STATUS_STORM_NOT_ENABLED */ "The specified Storm wasn't enabled", /* DBG_STATUS_BLOCK_ALREADY_ENABLED */ "The block was already enabled", /* DBG_STATUS_BLOCK_NOT_ENABLED */ "The specified block wasn't enabled", /* DBG_STATUS_NO_INPUT_ENABLED */ "No input was enabled for recording", /* DBG_STATUS_NO_FILTER_TRIGGER_256B */ "Filters and triggers are not allowed in E4 256-bit mode", /* DBG_STATUS_FILTER_ALREADY_ENABLED */ "The filter was already enabled", /* DBG_STATUS_TRIGGER_ALREADY_ENABLED */ "The trigger was already enabled", /* DBG_STATUS_TRIGGER_NOT_ENABLED */ "The trigger wasn't enabled", /* DBG_STATUS_CANT_ADD_CONSTRAINT */ "A constraint can be added only after a filter was " "enabled or a trigger state was added", /* DBG_STATUS_TOO_MANY_TRIGGER_STATES */ "Cannot add more than 3 trigger states", /* DBG_STATUS_TOO_MANY_CONSTRAINTS */ "Cannot add more than 4 constraints per filter or trigger state", /* DBG_STATUS_RECORDING_NOT_STARTED */ "The recording wasn't started", /* DBG_STATUS_DATA_DID_NOT_TRIGGER */ "A trigger was configured, but it didn't trigger", /* DBG_STATUS_NO_DATA_RECORDED */ "No data was recorded", /* DBG_STATUS_DUMP_BUF_TOO_SMALL */ "Dump buffer is too small", /* DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED */ "Dumped data is not aligned to chunks", /* DBG_STATUS_UNKNOWN_CHIP */ "Unknown chip", /* DBG_STATUS_VIRT_MEM_ALLOC_FAILED */ "Failed allocating virtual memory", /* DBG_STATUS_BLOCK_IN_RESET */ "The input block is in reset", /* DBG_STATUS_INVALID_TRACE_SIGNATURE */ "Invalid MCP trace signature found in NVRAM", /* DBG_STATUS_INVALID_NVRAM_BUNDLE */ "Invalid bundle ID found in NVRAM", /* DBG_STATUS_NVRAM_GET_IMAGE_FAILED */ "Failed getting NVRAM image", /* DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE */ "NVRAM image is not dword-aligned", /* DBG_STATUS_NVRAM_READ_FAILED */ "Failed reading from NVRAM", /* DBG_STATUS_IDLE_CHK_PARSE_FAILED */ "Idle check parsing failed", /* DBG_STATUS_MCP_TRACE_BAD_DATA */ "MCP Trace data is corrupt", /* DBG_STATUS_MCP_TRACE_NO_META */ "Dump doesn't contain meta data - it must be provided in image file", /* DBG_STATUS_MCP_COULD_NOT_HALT */ "Failed to halt MCP", /* DBG_STATUS_MCP_COULD_NOT_RESUME */ "Failed to resume MCP after halt", /* DBG_STATUS_RESERVED0 */ "", /* DBG_STATUS_SEMI_FIFO_NOT_EMPTY */ "Failed to empty SEMI sync FIFO", /* DBG_STATUS_IGU_FIFO_BAD_DATA */ "IGU FIFO data is corrupt", /* DBG_STATUS_MCP_COULD_NOT_MASK_PRTY */ "MCP failed to mask parities", /* DBG_STATUS_FW_ASSERTS_PARSE_FAILED */ "FW Asserts parsing failed", /* DBG_STATUS_REG_FIFO_BAD_DATA */ "GRC FIFO data is corrupt", /* DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA */ "Protection Override data is corrupt", /* DBG_STATUS_DBG_ARRAY_NOT_SET */ "Debug arrays were not set " "(when using binary files, dbg_set_bin_ptr must be called)", /* DBG_STATUS_RESERVED1 */ "", /* DBG_STATUS_NON_MATCHING_LINES */ "Non-matching debug lines - in E4, all lines must be of " "the same type (either 128b or 256b)", /* DBG_STATUS_INSUFFICIENT_HW_IDS */ "Insufficient HW IDs. Try to record less Storms/blocks", /* DBG_STATUS_DBG_BUS_IN_USE */ "The debug bus is in use", /* DBG_STATUS_INVALID_STORM_DBG_MODE */ "The storm debug mode is not supported in the current chip", /* DBG_STATUS_OTHER_ENGINE_BB_ONLY */ "Other engine is supported only in BB", /* DBG_STATUS_FILTER_SINGLE_HW_ID */ "The configured filter mode requires a single Storm/block input", /* DBG_STATUS_TRIGGER_SINGLE_HW_ID */ "The configured filter mode requires that all the constraints of a " "single trigger state will be defined on a single Storm/block input", /* DBG_STATUS_MISSING_TRIGGER_STATE_STORM */ "When triggering on Storm data, the Storm to trigger on must be specified" };
Последнее — код, который превращает именованную константу в строчку:
const char *qed_dbg_get_status_str(enum dbg_status status) { return (status < MAX_DBG_STATUS) ? s_status_str[status] : "Invalid debug status"; }
Где ошибка?
Нашли?
На самом деле есть намёк, за который можно теоретически зацепиться. В массиве все строки отделяются одной пустой строкой (см. первый комментарий к статье), кроме одного места. Но начнём мы с предупреждения анализатора кода PVS-Studio:
V557 Array overrun is possible. The value of ‘status’ index could reach 58. qede_debug.c 7149
Оно указывает на место, где из массива по индексу извлекается указатель на строку:
return (status < MAX_DBG_STATUS) ? s_status_str[status] : "Invalid debug status";
Вначале я поставил неправильный скучный диагноз. По невнимательности я подумал, что передо мной off-by-one error. Уж очень это похоже на типовую неправильную проверку, которую я встречал много раз в разных проектах.
Суть классического антипаттерна. Есть enum, последний элемент которого используется как количество элементов в нём.
enum Efoo { A, B, C, COUNT };
Константам в enum присваиваются значения, начиная с 0. Соответственно, вспомогательная константа COUNT окажется равна 3, что соответствует количеству основных именованных констант.
Есть какой-то массив, где каждой именованной константе что-то соответствует:
char Cfoo[] = { 'A', 'B', 'C' };
Часто допускают баг, когда проверяют, что индекс не выходит за границу массива:
char Convert(unsigned id) { return (id > COUNT) ? 0 : Cfoo[id]; }
Проверка работает неправильно: если id окажется равен COUNT, то произойдёт выход за границу массива. Для таких ошибок анализатор PVS-Studio выдаёт предупреждение аналогично рассмотренному ранее.
Правильным будет один из следующих вариантов:
return (id >= COUNT) ? 0 : Cfoo[id]; // OK return (id < COUNT) ? Cfoo[id] : 0; // OK
Ничего интересного. Расходимся. В статью можно выписывать только само предупреждение рядом с другими ошибками выхода за границу, но сам баг не разбирать в статье про проверку DPDK.
И тут в последний момент — СТОП!
ЗДЕСЬ ЖЕ ВЕДЬ ПРАВИЛЬНАЯ ПРОВЕРКА НАПИСАНА!
return (status < MAX_DBG_STATUS) ? s_status_str[status] : "Invalid debug status";
Теперь непонятно и интересно! Почему анализатор выдал предупреждение? Ложное срабатывание? Это вряд ли, тут ему негде запутаться.
Засучив рукава, я начинаю внимательно изучать код. Вот оно! Пропущена строка для константы DBG_STATUS_NO_MATCHING_FRAMING_MODE.
В enum:
DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS, DBG_STATUS_NO_MATCHING_FRAMING_MODE, DBG_STATUS_VFC_READ_ERROR,
В массиве:
/* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */ "The filter/trigger constraint dword offsets are not " "enabled for recording", /* DBG_STATUS_VFC_READ_ERROR */ "Error reading from VFC",
Обратите внимание на разделитель, состоящий из двух пустых строчек (см. первый комментарий к статье). Я упоминал об этом артефакте раньше. Здесь что-то пошло не так. Возможно, произошёл какой-то сбой при заполнении массива, возможно, строчку случайно удалили, возможно, неудачный мёрж веток, или что-то ещё.
Впрочем, это сейчас, когда найден баг, две пустые строчки выглядят подозрительно. При обзоре никто не обратит на них внимание. А даже случайно заметив, просто удалят одну из них для красоты. Никто не скажет, «да тут небось что-то пропустили, пойдём проверим» 🙂
В результате этой ошибки:
- возможен выход за границу массива;
- функция возвращает неправильные строки для всех констант, начиная с DBG_STATUS_NO_MATCHING_FRAMING_MODE.
На мой взгляд, это красиво. Спасибо за внимание. Регулярно проверяйте код используя PVS-Studio.
P.S.
Коллега предложил добавить в статью, что подобной ошибки можно избежать, используя проверки этапа компиляции. Примеры кода с static_assert для C и C++. Для C++ получается ловчее с вычислением размеров массивов, но это уже другая история. Пожалуй, везде такие проверки добавлять — перебор. Но вот для таких больших массивов, почему бы и нет.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Most striking error I found with PVS-Studio in 2024.
ссылка на оригинал статьи https://habr.com/ru/articles/853920/
Добавить комментарий