Самая красивая ошибка, которую я нашёл с помощью PVS-Studio в 2024 году

28 Окт 2024

Автор: Андрей Карпов

Сразу предупреждаю, мои вкусы очень специфичны. Красота ошибки в том, что человеку её очень сложно найти. Я не верю, что её можно заметить при обзоре кода. Если только заранее знать, что она есть, и искать её целенаправленно.

Ошибку я нашёл в проекте DPDK. В нём есть и другие ошибки, но про них потом. Они меркнут перед этим алмазом. Только не ждите чего-то эдакого. Ошибка проста до безобразия. Вот только найти её, просматривая код, ой как непросто. Собственно, попробуйте сами.

Для этого сначала изучите список именованных констант:

enum dbg_status open icon

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[] = .... open icon

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++ получается ловчее с вычислением размеров массивов, но это уже другая история. Пожалуй, везде такие проверки добавлять — перебор. Но вот для таких больших массивов, почему бы и нет.

#Cpp