Проверяем код дельфина Flipper Zero на чистоту с помощью PVS-Studio23.12.2021 16:46

Flipper Zero — швейцарский нож для гиков и пентестеров с открытым исходным кодом. Так получилось, что пути этого проекта и анализатора PVS-Studio пересеклись. Философский вопрос: начинать ли проверять проект, зная, что авторы проекта уже исправляют ошибки? Попробуем.

Знакомство с Flipper Zero

Я попросил создателей проекта Flipper Zero принять участие в написании статьи, давая различные пояснения и комментарии в процессе. Поэтому статья отличается от наших классических публикаций про проверку открытых проектов.

Flipper Zero — проект карманного мультитула для исследования систем контроля доступа: домофонов, радиопультов, шлагбаумов, телевизоров, бесконтактных карт. Он построен на основе микроконтроллера STM32WB55, и весь исходный код проекта открыт под лицензией GPL. Впрочем, не буду пытаться своими словами составить описание. Мне приятно, что наши читатели узнают про этот интересный проект от его создателей, поэтому передаю им слово.

Прошивка Флиппера написана на чистом Си с очень небольшими вкраплениями С++.
Нам хотелось, чтобы под Флиппер можно было писать на любом языке, поэтому на низком и среднем уровне (аппаратный HAL и ядро системы, те функции, которыми пользуются приложения) код реализован на чистом Си. Это позволяет привязать эти API к любому языку, поддерживающему C ABI (буквально любой язык).
Помимо этого, несколько приложений (это считается во Флиппере верхним уровнем) написаны на С++, потому что это позволяет писать код быстрее, и для этих приложений это было очень критично.

Многие из разработчиков проекта Flipper Zero читают наши публикации. Некоторые из наших сотрудников в свою очередь интересуются судьбой и развитием проекта. И неудивительно, что настал момент, когда наши команды пересеклись и началась переписка.

Кто-то из Flipper Zero предложил проверить их проект с помощью анализатора PVS-Studio. Почему бы и нет. Тем более что один из моих коллег написал в наш внутренний чатик: «Это очень крутые ребята!». В общем, «надо брать» :).

Другой мой коллега тут же быстро пробежался по проекту и оставил комментарий: «Хотя ошибок, кажется, немного, но кое-что достойное внимания есть». Отлично! Мы всегда рады проверить интересный проект. И нам — возможность показать работу анализатора, и проекту — польза.

Писать или не писать?

Одним из подозрительных мест, выписанных на скорую руку, было:

if(....) { .... }
else
{
  memcpy(subghz->file_name_tmp, subghz->file_name, strlen(subghz->file_name));
  if(scene_manager_get_scene_state(....)== SubghzCustomEventManagerSet) {
    subghz_get_next_name_file(subghz);
  }
}

Предупреждение PVS-Studio: V575 The 'memcpy' function doesn’t copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. subghz_scene_save_name.c 22

Сейчас станет понятно, почему я привожу этот фрагмент кода. Дело в том, что, пока я морально готовился делать полноценный анализ проекта и писать статью, авторы Flipper Zero запросили демонстрационную версию PVS-Studio. И сообщили нам, что, возможно, сами проверят код и даже что-то напишут на эту тему.

И действительно, беру я чуть более свежую версию проекта и не вижу предупреждения, описанного моим коллегой. Иду смотреть код и обнаруживаю, что он уже исправлен. Добавилось »+1».

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

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

В общем, смотрю я на исправленный фрагмент кода и начинаю грустить :(. Не успел. Не писать же про ошибки, которых уже нет… Ведь я ещё не знаю, как исправляли ошибку.

Решаю, на всякий, посмотреть ещё одну ранее выписанную ошибку.

static FS_Error storage_process_common_rename(Storage* app, const char* old,
                                              const char* new)
{
  FS_Error ret = FSE_INTERNAL;
  StorageType type_old = storage_get_type_by_path(old);
  StorageType type_new = storage_get_type_by_path(new);

  if(storage_type_is_not_valid(type_old) || storage_type_is_not_valid(type_old))
  {
    ret = FSE_INVALID_NAME;
  }
  else
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions 'storage_type_is_not_valid (type_old)' to the left and to the right of the '||' operator. storage-processing.c 380

О радость! Ошибка на месте!

Опечатка: два раза проверяется переменная type_old. А переменная type_new не проверятся.

Я, конечно, понимаю, что это странно радоваться ошибкам в программе. Сорри, у меня профессиональная деформация :).

Главное, ещё не всё потеряно для моего творческого порыва, и можно смотреть дальше. И действительно, в новом отчёте есть предупреждения, указывающие на реальные ошибки. Да, их мало, но они есть, и я напишу про них.

Здесь, кстати, возникает вопрос, насколько уже начал использоваться PVS-Studio для проверки проекта? Прошу прокомментировать. И тогда, я напишу один из двух вариантов текста:

PVS-Studio ещё не используется. Ошибку мы нашли и исправили сами. Я скажу: используя PVS-Studio, такие ошибки можно находить и исправлять быстрее.
Ошибка исправлена благодаря PVS-Studio. Я: вот видите, как полезен PVS-Studio.

В общем, в любом случае PVS-Studio — это хорошо :).

PVS-Studio ещё не используется, так как для первичной настройки надо затратить определённое количество времени, в плане настройки мест, где ошибка — не ошибка (например, всё связанное с malloc-ами). Мы планируем этим заняться в скором будущем, и также прикрутить его к остальным нашим проектам, например wifi-программатору.

Ага, значит первый вариант. Хотя из пояснения стало понятно, что это неполноценная ошибка и »+1» добавлено для аккуратности, это можно было сделать заранее.

По поводу лёгкого и быстрого внедрения — уже всё продумано! Для этого в PVS-Studio есть такой механизм, как массовое подавление предупреждений (set the baseline). Можно отложить текущий технический долг на потом и работать только с новыми предупреждениями.

Совсем краткое описание можно посмотреть здесь.

Более развернутое объяснение про то, как подружить анализатор кода и большую кодовую базу: «Как внедрить статический анализатор кода в legacy проект и не демотивировать команду».

Ещё ошибки, которые я успел найти

Как я и обещал, рассмотрим интересные места кода, к которым моё внимание привлёк анализатор PVS-Studio. Заодно предлагаю, не откладывая, скачать бесплатную пробную версию, чтобы проверить свои собственные проекты.

Лишний return

void onewire_cli_search(Cli* cli) {
  ....
  bool done = false;
  ....
  onewire.start();
  furi_hal_power_enable_otg();

  while(!done) {
    if(onewire.search(address, true) != 1) {
      printf("Search finished\r\n");
      onewire.reset_search();
      done = true;
      return;
    } else {
      printf("Found: ");
      for(uint8_t i = 0; i < 8; i++) {
        printf("%02X", address[i]);
      }
    printf("\r\n");
    }
    delay(100);
  }

  furi_hal_power_disable_otg();
  onewire.stop();
}

Анализируемый код содержит с точки зрения PVS-Studio сразу две аномалии:

V654 [CWE-834] The condition '! done' of loop is always true. ibutton-cli.cpp 253
V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. ibutton-cli.cpp 269

И действительно! Во-первых, условие цикла всегда истинно. После того как значение переменной done меняется внутри тела цикла, функция тут же завершает работу, так что это изменение не имеет значения.

Во-вторых, эпилог функции не выполняется. Этот код никогда не получает управление:

furi_hal_power_disable_otg();
onewire.stop();

Как следствие, нарушается логика работы программы.

Проверка указателя, возвращаемого функциями malloc

В проекте достаточно фривольно обращаются с результатом работы функции malloc. Где-то приложение жёстко прекращает работу, если не удалось выделить память. Пример:

void random_permutation(unsigned n)
{
  if (permutation_tab) free(permutation_tab);
  permutation_tab = (unsigned *) malloc(n * sizeof(unsigned));
  if (permutation_tab == NULL) abort();
  ....
}

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

Flipper Zero Team. Это внешняя библиотека.

Я. Тогда это стрёмная библиотека, раз она зовёт abort и используется во встраиваемом устройстве. Например, AUTOSAR (AUTomotive Open System ARchitecture) вообще запрещает подобное — V3506.

Flipper Zero Team. Этот код — часть бенчмарка.

Flipper Zero Team. Верно, это header-only библиотека, и качество кода её тестов нас не особо волнует.

Я. Согласен. Тогда вообще всё OK, но оставлю всё это в статье. Быть может, кто-то другой задумается, а нет ли случайно abort/exit в библиотеках, которые они затянули в свои встраиваемые устройства.

В других местах нулевой указатель интерпретируется более спокойно:

ptr = malloc(sizeof(uint8_t) * BlockSize);
if(ptr == NULL) {
  goto error;
}

Внешний код.

Где-то существует проверка, осуществляемая только в отладочных версиях:

size_t bench_mlib(unsigned n)
{
  string_t *tab = (string_t*) malloc (n * sizeof (string_t));
  assert (tab != 0);
  ....
}

Кстати, на мой взгляд, это странное решение. По факту, пользу от такой проверки может получить только разработчик, но не пользователь. Считаю, нужно или делать полноценную обработку ошибки выделения памяти, или уж не делать вид, что проверка существует, и удалить assert:).

Есть причина, почему здесь выбран такой способ проверки?

Это внешняя библиотека.

Теперь самое интересное. Иногда нет вообще никакой проверки. Выделенная память сразу начинает смело использоваться. Например, здесь:

void storage_ext_init(StorageData* storage) {
  SDData* sd_data = malloc(sizeof(SDData));
  sd_data->fs = &USERFatFS;
  ....
}

Предупреждение PVS-Studio: V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'sd_data'. Check lines: 516, 515. storage-ext.c 516

Есть и другие подобные предупреждения:

V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'app'. Check lines: 8, 7. dialogs.c 8
V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'app'. Check lines: 162, 161. notification-settings-app.c 162
V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'bench_data'. Check lines: 81, 79. storage_settings_scene_benchmark.c 81
V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'app'. Check lines: 18, 16. storage_settings.c 18
V575 [CWE-628, CERT-EXP37-C] The potential null pointer is passed into 'strlen' function. Inspect the first argument. Check lines: 174, 168. storage-test-app.c 174

Примечание. Предвижу, что кто-то напишет, что особого смысла проверять такие указатели нет. Для них сразу предлагаю статью-ответ: «Почему важно проверять, что вернула функция malloc».

И ожидаемый вопрос для авторов проекта: Отсутствие проверок — это просто ошибка или в этом есть определённый расчёт на невозможность такого события?

Если в эмбеддед кончилась память — продолжать работу опасно. Используемый нами аллокатор прекращает всё выполнение кода прошивки при неудачной аллокации и зовёт человека с дебаггером.

Продолжаем тему нулевых указателей

Если судить по устройству функции furi_record_data_get_or_create, она теоретически может вернуть нулевой указатель:

static FuriRecordData* furi_record_data_get_or_create(string_t name_str) {
  furi_assert(furi_record);
  FuriRecordData* record_data =
    FuriRecordDataDict_get(furi_record->records, name_str);
  if(!record_data) {
    FuriRecordData new_record;
    new_record.flags = osEventFlagsNew(NULL);
    ....
  }
  return record_data;
}

Теперь посмотрим, как эта функция используется:

void furi_record_create(const char* name, void* data) {
  ....
  FuriRecordData* record_data = furi_record_data_get_or_create(name_str);
  furi_assert(record_data->data == NULL);
  record_data->data = data;
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476, CERT-EXP34-C] Dereferencing of the null pointer 'record_data' might take place. record.c 65

Указатель, который вернула функция, смело используется без предварительной проверки.

Тут я ошибся. На самом деле, это ложное срабатывание. Авторы указали мне, что я не досмотрел устройство функции furi_record_data_get_or_create. Не стану удалять своё неправильное описание, лучше подробнее разберём этот случай.

Посмотрим функцию целиком:

static FuriRecordData* furi_record_data_get_or_create(string_t name_str) {
  furi_assert(furi_record);
  FuriRecordData* record_data =
    FuriRecordDataDict_get(furi_record->records, name_str);
  if(!record_data) {
    FuriRecordData new_record;
    new_record.flags = osEventFlagsNew(NULL);
    new_record.data = NULL;
    new_record.holders_count = 0;
    FuriRecordDataDict_set_at(furi_record->records, name_str, new_record);
    record_data = FuriRecordDataDict_get(furi_record->records, name_str);
  }
  return record_data;
}

Если сразу удалось получить запись, то её и возвращаем. Если не получили, то создаём и возвращаем. Всё хорошо.

Анализатор же оказался недостаточно сообразительным. Логика вывода такая. Раз есть проверка, то указатель может быть равен NULL. А раз так, функция может вернуть NULL. То, что указатель в любом случае инициируется, анализатор почему-то не учёл.

Выводы: Авторы Flipper Zero оказались ещё большими молодцами. Алгоритм Data-Flow в PVS-Studio следует доработать для подобных случаев.

Продолжим тему нулевых указателей. Есть срабатывание диагностики, построенной на другой логике. Диагностика V595 выдаёт предупреждение, когда вначале указатель разыменовывается, а потом вдруг проверяется. Это очень подозрительно, и с помощью этой диагностики часто выявляется много ошибок. Похвала: к счастью, Flipper Zero не является таким проектом и им/нам не удалось собрать пучок красивых V595:). Но одно полезное срабатывание всё-таки я заметил:

void subghz_scene_receiver_info_on_enter(void* context) {
  ....
  subghz->txrx->protocol_result->to_string(subghz->txrx->protocol_result, text);
  widget_add_string_multiline_element(....);

  string_clear(frequency_str);
  string_clear(modulation_str);
  string_clear(text);

  if(subghz->txrx->protocol_result &&
     subghz->txrx->protocol_result->to_save_file &&
     strcmp(subghz->txrx->protocol_result->name, "KeeLoq")) {
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'subghz→txrx→protocol_result' pointer was utilized before it was verified against nullptr. Check lines: 70, 78. subghz_scene_receiver_info.c 70

Хотя я и рассматриваю в статье различные ошибки, связанные с нулевыми указателями, я должен похвалить авторов проекта за качество кода. Для C кода плотность таких ошибок низкая.

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

Мы использовали MPU, чтобы ловить hard fault по обращению к нулевому адресу. Странно, что у этого подхода нет распространения в embedded. Также у нас написан анализатор потребления и освобождения памяти приложением. К сожалению, без MMU это не реализуется в полной мере, но определённое представление о правильности аллокаций это даёт.
Ещё -Werror.
Но не могу сказать, что у нас не было бессонных ночей дебага коррапченой кучи :).

Кто-то поспешил

bool subghz_get_preset_name(SubGhz* subghz, string_t preset) {
  const char* preset_name;
  switch(subghz->txrx->preset) {
  case FuriHalSubGhzPresetOok270Async:
    preset_name = "FuriHalSubGhzPresetOok270Async";
    break;
  case FuriHalSubGhzPresetOok650Async:
    ....
  case FuriHalSubGhzPreset2FSKDev476Async:
    preset_name = "FuriHalSubGhzPreset2FSKDev476Async";
    break;
      FURI_LOG_E(SUBGHZ_PARSER_TAG, "Unknown preset");   // <=
  default:
  ....
}

Предупреждение PVS-Studio: V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. subghz_i.c 44

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

А как так получилось на самом деле, известно? Понятно, что ошибка не критичная, но всё равно интересно :).

Более того, макрос логирования должен быть в метке default. У нас значительные объемы мержей и недостаток свободных рук в команде — иногда вызывает пропуск таких проблем.

Когда возможно все не правы

Перед нами тот случай, когда с кодом что-то не так, но непонятно, насколько он ошибочен. И непонятно, насколько точен в сообщениях анализатор PVS-Studio.

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

void subghz_cli_command_tx(Cli* cli, string_t args, void* context) {
  uint32_t frequency = 433920000;
  uint32_t key = 0x0074BADE;
  size_t repeat = 10;

  if(string_size(args)) {
    int ret = sscanf(string_get_cstr(args),
                     "%lx %lu %u", &key, &frequency, &repeat);
  ....
}

Предупреждение PVS-Studio: V576 [CWE-628, CERT-FIO47-C] Incorrect format. Consider checking the fifth actual argument of the 'sscanf' function. A pointer to the unsigned int type is expected. subghz_cli.c 105

Обратите внимание на форматную строку, управляющую данными при сканировании:»%lx %lu %u». Она означает, что ожидаются указатели на переменные следующих типов:

%lx — long unsigned int;
%lx — long unsigned int;
%u — unsigned int.

При этом, программа для хранения считанных данных будет использовать переменные типа:

uint32_t;
uint32_t;
size_t.

Я не изучал вопрос, какие размеры данных предполагаются при компиляции проекта Flipper Zero, и затрудняюсь сказать, насколько этот код опасен. Какую правку стоит сделать в любом случае — это однозначно заменить »%u» на »%zu» (см. описание функции sscanf).

Более подробно я смогу прокомментировать код и предупреждение анализатора, если авторы проекта подскажут, какие размеры типов возможны на платформах, на которых собирается проект. Другими словами, интересны возможные модели данных, которые используются при компиляции проекта.

Ошибка. Третья переменная repeat должна иметь тип int32_t.

Тогда опять получается несоответствие. Для сканирования двух первых 32-битных переменных используется управляющий модификатор «l» (long), а для третьей — нет. Плюс несоответствие signed/unsigned.

%lx (long unsigned int) → uint32_t;
%lx (long unsigned int) → uint32_t;
%u (unsigned int) → int32_t;

Подозреваю, что размер типа int совпадает с размером типа long int, а ввести отрицательно число нельзя. Поэтому этот и другой код работают корректно. Тем не менее предлагаю изучить все предупреждения V576 анализатора PVS-Studio и более аккуратно написать управляющие (форматные) строки там, где это необходимо.

Заключение

Статья получилась небольшой, так как проект достаточно качественный, хотя и написан преимущественно на языке C. А что греха таить, код на C в сравнении с C++ более подвержен ошибкам, которые могут выявлять статические анализаторы кода. Доказательств этого утверждения у меня нет, но есть ощущение, подкреплённое проверками мною множества открытых проектов.

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

Наша главная цель при написании прошивки Флиппера — создать удобный и понятный проект, с которым будет приятно работать. Пока он далёк от идеала, но мы усиленно работаем над улучшением читаемости кода и документацией. Мы верим, что силами сообщества получится сделать крутой продукт.

Спасибо за внимание и приходите читать другие наши статьи по тематике embedded и IoT.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio checks the code of Flipper Zero dolphin.