Красивый баг


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


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


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


enum dbg_status
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[] = ....
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.

Комментарии (18)


  1. Andrey2008 Автор
    28.10.2024 07:33

    Движок habr для кода умный там, где не надо :)

    Он схлопывает две пустые строчки в одну, поэтому не удаётся показать лишнюю пустую строчку в коде, про которою идёт речь. Она здесь:

    Как вариант можно у нас в блоге посмотреть статью, там как показан так, как надо.


  1. RodionGork
    28.10.2024 07:33

    Ну такое, всё же здесь пресловутая "ошибка в ДНК" скорее, а то что нашли - уже вторичное проявление. Безусловно если код разрабатывать таким образом то без тулов для всевозможных проверок и анализа будет очень больно.

    Если уж константы определили то нужно было коненчо не лениться и строки расставить в соответствии с ними, а не комменты к ним приписывать, т.е. массив инициализировать вот таким немного унылым образом:

    s_status_str[DBG_STATUS_OK] = "Operation completed successfully";
    s_status_str[DBG_STATUS_APP_VERSION_NOT_SET] = "Debug application version wasn't set";
    //...

    я не знаток C/C++ так что может есть уже какая-то более удобная конструкция (можно было вообще ключи текстовые имхо и мапу использовать) - но принцип такой - не надо константы и значения по двум разным файлам разносить и пытаться их вручную с дурацкими комментами матчить. это обязательно "бумкнет" :)


    1. alexxisr
      28.10.2024 07:33

      в джаве сделали удобно - можно в самом enum добавлять произвольные поля и пользоваться им как этакой нерасширяемой мапой.


      1. RodionGork
        28.10.2024 07:33

        да, вариантов много - ну в джаве можно было бы имя константы самой вывести с помощью рефлексии и не возиться со строчками. или вынести строчки в json-файл и выбирать их оттуда (возможно опять же по имени константы)... тут и локализацию легко внедрить.


    1. mentin
      28.10.2024 07:33

      Проблема с такой инициализацией - она выполняется в рантайме, а та - статически компилятором. Начиная с С99 (то есть почти везде) можно и статически, но про это мало кто знает и редко использует.
      https://www.geeksforgeeks.org/designated-initializers-c/


      1. RodionGork
        28.10.2024 07:33

        но про это мало кто знает

        спасибо, огонь :) очевидно "красивая ошибка" настолько стара что ещё в C99 против неё попытались обезопаситься


    1. Mishootk
      28.10.2024 07:33

      я не знаток C/C++ так что может есть уже какая-то более удобная конструкция (можно было вообще ключи текстовые имхо и мапу использовать) - но принцип такой - не надо константы и значения по двум разным файлам разносить и пытаться их вручную с дурацкими комментами матчить. это обязательно "бумкнет" :)

      Вот отправная точка:

      https://habr.com/ru/articles/276763/
      Взял идею, под свои потребности написал почти заново и нарадоваться не могу.


  1. mentin
    28.10.2024 07:33

    На ревью кода конечно такое можно поймать, я бы здесь попросил добавить static_assert что размер второго массива совпадает с MAX_DBG_STATUS.


    1. RodionGork
      28.10.2024 07:33

      порядок перепутать легко все равно... особенно если эти фрагменты часто редактируют разные люди... ну тогда и анализатор не поможет впрочем :)


  1. viordash
    28.10.2024 07:33

    можно использовать в qed_dbg_get_status_str switch на енам dbg_status . В этом случае массив строк s_status_str уже не нужен. А компилятор ругнется если не все кейсы будут обработаны.


  1. COKPOWEHEU
    28.10.2024 07:33

    Да тут сам массив строк выглядит ужасно.

    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",
    

    И все.


    1. datacompboy
      28.10.2024 07:33

      Поможет гарантировать, что к правильным индексами привязаны правильные строки, но не гарантирует, что для всех индексов прописаны строки.

      Причем, если к последней определённой константе строка есть -- пропущенные в середине countof() не поймает.


      1. COKPOWEHEU
        28.10.2024 07:33

        не гарантирует, что для всех индексов прописаны строки.

        А это толком и не отследить. Разве что в рантайме пройтись циклом и проверить длину каждой строки. В любом случае это куда менее опасная ошибка, чем сдвиг вообще всех индексов.

        Пример по сути демонстрирует, что используя плохие практики программирования, можно получить трудноуловимые ошибки. Ну так не используйте плохие практики.


        1. datacompboy
          28.10.2024 07:33

          А это толком и не отследить

          Есть много способов отследить и убедиться в этом. Как правило, игнорируемые по причине того, что код -- одноразовый. Написал и забыл, редактируется настолько редко, что нагораживание защит вокруг дороже обойдётся.

          Лучший способ в данном конкретном случае (и всех аналогичных) -- использовать статический анализатор, который может поймать косвенно (как в статье) по использованию, или попытаться "догадаться" что массивы должны быть 1-в-1 с enum'ом.

          Я удивлён, кстати ( @Andrey2008 ), что PVS-Studio не догадывается что это дескриптор для ENUMа и не проверяет на равенство элементов и порядка констант унутре. А ведь косяк в таких массивах часто получается из-за потерянной запятой.


          1. Andrey2008 Автор
            28.10.2024 07:33

            Хм.. Место для подумать, спасибо.


  1. neon1ks
    28.10.2024 07:33

    Этот код хорошо покрывается юнит тестами.


    1. Andrey2008 Автор
      28.10.2024 07:33

      Но не покрыт :)

      P.S. Про юнит-тесты в DPDK отдельная земетка скоро будет :)


  1. RR_Zz
    28.10.2024 07:33

    Тут прям напрашивается xmacro паттерн

    https://www.geeksforgeeks.org/x-macros-in-c/

    Смотреть Example 2