Сергей Ларин

26 Авг 2021

Теги:

#Cpp #OS

30 лет ядру Linux: поздравление от PVS-Studio

26 Авг 2021

Автор: Сергей Ларин

Введение
Запуск анализатора
Использование указателя перед проверкой
Опечатка в применении маски
Ошибка в выборе типов
Разыменование нулевого указателя после проверки
Заключение

25 августа 2021 года ядру Linux исполняется 30 лет. За это время ядро пережило множество изменений, так же, как и мы. Сегодня это огромный проект, работающий на миллионах различных устройств. Предыдущую проверку мы делали 5 лет назад, поэтому не можем пропустить такое событие и не заглянуть в код этого эпического проекта.

Введение

В прошлый раз мы выписали 7 интересных ошибок. Примечательно, что сейчас ошибок, заслуживающих рассмотрения в статье, ещё меньше!

На первый взгляд это странно. Размер ядра увеличился. В анализаторе PVS-Studio появились десятки новых диагностических правил. Улучшены внутренние механизмы, анализ потока данных, появился межмодульный анализ и так далее. Как же так получилось, что интересных ошибок почти нет?

Очень просто. Проект стал ещё более качественным! Именно с этим мы и поздравляем Linux в его 30-летие.

Высокое качество связано с тем, что инфраструктура проекта заметно улучшилась. Теперь ядро можно собирать не только GCC, но и Clang – для этого не требуется дополнительных патчей. Дорабатываются другие инструменты статического анализа (появился GCC -fanalyzer, развивается местный анализатор Coccinelle, проект проверяется через Clang Static Analyzer), а также автоматизированные системы проверки кода (kbuild test robot). К тому же за поиск уязвимостей выплачивается Bug Bounty компанией Google.

Однако это не значит, что ошибок нет :). И несколько хороших мы сейчас разберём. "Хорошие и красивые" они, конечно, исключительно в нашем понимании :). Более того, статический анализ следует использовать регулярно, а не вот так наскоками раз в пять лет. Так, действительно, ничего не найдёшь. Подробнее эта мысль рассмотрена в статье "Ошибки, которые не находит статический анализ кода, потому что он не используется".

Но вначале пара слов про запуск анализатора.

Запуск анализатора

Поскольку ядро теперь умеет собираться компилятором Clang, под него в проекте также была заведена инфраструктура, в том числе генератор compile_commands.json, который создаёт файл с JSON Compilation Database из .cmd файлов, генерируемых во время сборки. Так что для его создания придётся скомпилировать ядро. Необязательно использовать компилятор Clang, но рекомендуется производить компиляцию именно им из-за возможных несовместимых флагов у GCC.

Так что сгенерировать полный compile_commands.json для проверки проекта можно таким образом:

make -j$(nproc) allmodconfig # делаем полный конфиг
make -j$(nproc)              # собираем
./scripts/clang-tools/gen_compile_commands.py

После этого можно запускать анализатор:

pvs-studio-analyzer analyze -f compile_commands.json -a 'GA;OP' -j$(nproc) \
                            -e drivers/gpu/drm/nouveau/nouveau_bo5039.c \
                            -e drivers/gpu/drm/nouveau/nv04_fbcon.c

Зачем исключать из анализа эти 2 файла? Они содержат большое количество макросов, которые раскрываются в огромные строчки кода (до 50 тысяч символов на строку), из-за чего анализатор обрабатывает их долго, и их анализ может не пройти.

В недавнем релизе PVS-Studio 7.14 была добавлена возможность межмодульного анализа C/C++ проектов, и мы не могли придумать лучшего момента, чтобы его опробовать. К тому же на такой огромной кодовой базе:

Числа, конечно, внушительные. Суммарный объём проекта – почти 30 млн. строк кода. Вследствие этого изначальная проверка в этом режиме у нас прошла не совсем успешно: на этапе слияния межмодульной информации у нас была "съедена" вся оперативная память, и процесс анализатора был благополучно убит OOM-killer'ом. Мы разобрались в проблеме и придумали, что можно улучшить. Так что в следующем релизе 7.15 появится важная оптимизация, которая это исправит.

Для того, чтобы проверить проект в режиме межмодульного анализа, нужно всего лишь добавить один флаг в команду pvs-studio-analyzer:

pvs-studio-analyzer analyze -f compile_commands.json -a 'GA;OP' -j$(nproc) \
                            --intermodular \
                            -e drivers/gpu/drm/nouveau/nouveau_bo5039.c \
                            -e drivers/gpu/drm/nouveau/nv04_fbcon.c

После анализа мы получаем отчёт на тысячи предупреждений. К сожалению, настроить анализатор, чтобы отсечь ложные срабатывания, не было времени. Хотелось опубликовать статью сразу после дня рождения. Поэтому за час было выписано 4 наиболее интересных случая.

Однако настройка анализатора – это просто. Большинство предупреждений связано с неудачными макросами. Чуть позже мы отфильтруем отчёт и изучим его уже по-настоящему. По итогам постараемся порадовать вас другой более подробной статьёй про найденные ошибки.

Использование указателя перед проверкой

V595 The 'speakup_console[vc->vc_num]' pointer was utilized before it was verified against nullptr. Check lines: 1804, 1822. main.c 1804

static void do_handle_spec(struct vc_data *vc, u_char value, char up_flag)
{
  unsigned long flags;
  int on_off = 2;
  char *label;

  if (!synth || up_flag || spk_killed) 
    return;

  ....

  switch (value) {
  ....
  case KVAL(K_HOLD):
    label = spk_msg_get(MSG_KEYNAME_SCROLLLOCK);
    on_off = vt_get_leds(fg_console, VC_SCROLLOCK);
    if (speakup_console[vc->vc_num])                     // <= проверка
      speakup_console[vc->vc_num]->tty_stopped = on_off;
    break;
  ....
  }

  ....
}

Анализатор ругается на то, что указатель speakup_console[vc->vc_num] разыменовывается до проверки. При беглом взгляде на этот код может показаться, что срабатывание ложное. Однако разыменование действительно присутствует.

И знаете, где оно? :) Внутри макроса spk_killed. Да-да, это не переменная, как может показаться на первый взгляд:

#define spk_killed (speakup_console[vc->vc_num]->shut_up & 0x40)

Скорее всего, тот, кто менял этот код, ожидал, что разыменований не происходит и сделал проверку, т.к. где-то происходит передача нулевого указателя. Так что такие макросы, которые выглядят как переменные и не являются константами, усложняют сопровождение кода и делают его более уязвимым к ошибкам. Макросы – это зло.

Опечатка в применении маски

V519 The 'data' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6208, 6209. cik.c 6209

static void cik_enable_uvd_mgcg(struct radeon_device *rdev,
        bool enable)
{
  u32 orig, data;

  if (enable && (rdev->cg_flags & RADEON_CG_SUPPORT_UVD_MGCG)) {
    data = RREG32_UVD_CTX(UVD_CGC_MEM_CTRL);
    data = 0xfff;                              // <=
    WREG32_UVD_CTX(UVD_CGC_MEM_CTRL, data);

    orig = data = RREG32(UVD_CGC_CTRL);
    data |= DCM;
    if (orig != data)
      WREG32(UVD_CGC_CTRL, data);
  } else {
    data = RREG32_UVD_CTX(UVD_CGC_MEM_CTRL);
    data &= ~0xfff;                            // <=
    WREG32_UVD_CTX(UVD_CGC_MEM_CTRL, data);

    orig = data = RREG32(UVD_CGC_CTRL);
    data &= ~DCM;
    if (orig != data)
      WREG32(UVD_CGC_CTRL, data);
  }
}

Пример взят из кода драйвера для видеокарт Radeon. Судя по тому, как значение 0xfff используется в else-ветке, можно предположить, что это на самом деле битовая маска. В то время как в then-ветке значение, полученное строкой выше, перезаписывается без применения маски. Вероятно, правильный код должен был быть таким:

data = RREG32_UVD_CTX(UVD_CGC_MEM_CTRL);
data &= 0xfff; 
WREG32_UVD_CTX(UVD_CGC_MEM_CTRL, data);

Ошибка в выборе типов

V610 Undefined behavior. Check the shift operator '>>='. The right operand ('bitpos % 64' = [0..63]) is greater than or equal to the length in bits of the promoted left operand. master.c 354

// bitsperlong.h
#ifdef CONFIG_64BIT
#define BITS_PER_LONG 64
#else
#define BITS_PER_LONG 32
#endif /* CONFIG_64BIT */

// bits.h
/*
 * Create a contiguous bitmask starting at bit position @l and ending at
 * position @h. For example
 * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
 */
#define __GENMASK(h, l) ....

// master.h
#define I2C_MAX_ADDR      GENMASK(6, 0)

// master.c
static enum i3c_addr_slot_status
i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
{
  int status, bitpos = addr * 2;                   // <=

  if (addr > I2C_MAX_ADDR)
    return I3C_ADDR_SLOT_RSVD;

  status = bus->addrslots[bitpos / BITS_PER_LONG];
  status >>= bitpos % BITS_PER_LONG;               // <=

  return status & I3C_ADDR_SLOT_STATUS_MASK;
}

Обратите внимание, что макрос BITS_PER_LONG может иметь значение 64.

В коде содержится неопределенное поведение:

после проверки переменная addr может быть в диапазоне [0..127]
если формальный параметр addr >= 16, то переменная status будет сдвинута вправо на число бит больше, чем вмещает тип int (32 бита).

Вероятно, автор хотел сократить количество строк и сделал объявление переменной bitpos рядом с переменной status. Однако он не учёл, что int имеет 32-битный размер на 64-битных платформах, в отличии от типа long.

Для исправления следует объявить переменную status с типом long.

Разыменование нулевого указателя после проверки

V522 Dereferencing of the null pointer 'item' might take place. mlxreg-hotplug.c 294

static void
mlxreg_hotplug_work_helper(struct mlxreg_hotplug_priv_data *priv,
         struct mlxreg_core_item *item)
{
  struct mlxreg_core_data *data;
  unsigned long asserted;
  u32 regval, bit;
  int ret;

  /*
   * Validate if item related to received signal type is valid.
   * It should never happen, excepted the situation when some
   * piece of hardware is broken. In such situation just produce
   * error message and return. Caller must continue to handle the
   * signals from other devices if any.
   */
  if (unlikely(!item)) {
    dev_err(priv->dev, "False signal: at offset:mask 0x%02x:0x%02x.\n",
      item->reg, item->mask);

    return;
  }

  // ....
}

Здесь допущена классическая ошибка: если указатель нулевой, то необходимо вывести сообщение об ошибке. Однако этот же указатель используется при формировании сообщения. Конечно, ошибки подобного рода выявляются достаточно легко на этапе тестирования. Но конкретно в этом случае есть нюанс – судя по комментарию, это может произойти, если "какая-либо часть железки сломана". В любом случае код написан некорректно и его стоит исправить.

Заключение

Проект Linux стал для нас хорошим испытанием – на нём мы смогли проверить новую возможность межмодульного анализа. Само ядро Linux – важный для всего мира проект. За его качество борются разные люди и организации. Мы очень рады, что оно развивается. А также развиваемся и мы! Недавно мы открыли наш "запас" картинок, демонстрирующий начало и развитие дружбы нашего анализатора с Tux'ом. Вот они:

Единорог N81: