Проверка Кода Flipper Zero Dolphin На Чистоту С Помощью Pvs-Studio

Flipper Zero — это швейцарский армейский нож с открытым исходным кодом для компьютерных фанатов и пентестеров.

Так получилось, что пути этого проекта и анализатора PVS-Studio пересеклись.

Философский вопрос: стоит ли начинать проверку проекта, зная, что авторы проекта уже исправляют ошибки? Давай попробуем.

Представляем Флиппер Зеро

Поэтому статья отличается от наших классических публикаций.

о проверке открытых проектов .

Флиппер Зеро — проект карманного мультитула для исследования систем контроля доступа: домофонов, радиопультов, шлагбаумов, телевизоров, бесконтактных карт. Он построен на базе микроконтроллера STM32WB55, и все оригинальное.

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

Мне приятно, что наши читатели узнают об этом интересном проекте от его создателей, поэтому предоставляю им слово.

Прошивка Flipper написана на чистом C с небольшими включениями C++.

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

Кроме того, несколько приложений (во Flipper это считается верхним уровнем) написаны на C++, поскольку он позволяет писать код быстрее, а для этих приложений это было очень критично.

И неудивительно, что настал момент, когда наши команды пересеклись и началась переписка.

Кто-то из 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);
   }
 }

   

Используйте функцию «strcpy / strcpy_s», чтобы сохранить нулевой терминал.

subghz_scene_save_name.c 22 Теперь станет понятно, зачем я привожу этот кусок кода.

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

Я смотрю код и обнаруживаю, что он уже исправлен.

Добавлен "+1".

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

Почему бы просто не написать стркпи ?

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

Не успела.

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

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

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] Слева и справа от '||' имеются одинаковые подвыражения 'storage_type_is_not_valid(type_old)'.

оператор.

хранение-обработка.

c 380 О радость! Ошибка на сайте! Опечатка: переменная проверяется дважды тип_старый .

Переменная type_new проверяться не будет. Я конечно понимаю, что странно радоваться ошибкам в программе.

Извините, у меня профессиональная деформация :).

Главное, что для моего творческого порыва еще не все потеряно и я могу смотреть дальше.

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

Да, их немного, но они есть, и я о них напишу.

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

И далее, я напишу один из двух вариантов текста:

1. PVS-Studio пока не используется.

   Мы нашли ошибку и исправили ее самостоятельно.

   Скажу: с помощью PVS-Studio такие ошибки можно найти и исправить быстрее.

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

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

Планируем сделать это в ближайшее время, а также присоединить к другим нашим проектам, например, программатору wifi.

Что касается легкой и быстрой реализации – все уже продумано! Для этого в PVS-Studio есть механизм под названием массовый подавление предупреждений (установить базовую линию).

Вы можете отложить текущий технический долг на потом и работать только над новыми предупреждениями.

Очень краткое описание можно найти здесь.

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

Еще ошибки, которые мне удалось найти

Дополнительный доход

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("X", address[i]); } printf("\r\n"); } delay(100); } furi_hal_power_disable_otg(); onewire.stop(); }

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

• V654 [CWE-834] Условие цикла '!done' всегда истинно.

  ibutton-cli.cpp 253

• V779 [CWE-561, CERT-MSC12-C] Обнаружен недостижимый код. Вполне возможно, что ошибка присутствует. ibutton-cli.cpp 269

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

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

Этот код никогда не получает контроля:

furi_hal_power_disable_otg(); onewire.stop();

В результате логика программы нарушается.

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

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

Пример:

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

}

Примечание.

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

Как так получилось, так получилось, структуру проекта я не знаю.

Просто приведу фрагмент переписки с разработчиками.

Это делает его еще более интересным.

Команда Флиппер Зеро.

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

Я.

Тогда это странная библиотека, поскольку она вызывает аборт и используется во встроенном устройстве.

Например, AUTOSAR (AUTOmotive Open System ARchitecture) это вообще запрещает — В3506 .

Команда Флиппер Зеро.

Этот код является частью теста.

Команда Флиппер Зеро.

Правильно, это библиотека только для заголовков, и нас не особо заботит качество ее тестового кода.

Я.

Соглашаться.

Дальше всё в целом ок, но всё это я оставлю в статье.

Возможно, кто-то еще задумается, случайно ли аборт / Выход в библиотеках, которые они загрузили в свои встроенные устройства.

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

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

Внешний код.

}

Кстати, на мой взгляд, это странное решение.

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

Думаю нужно либо делать полноценную обработку ошибки выделения памяти, либо не делать вид, что проверка существует и удалять утверждать :).

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

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

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

Например, здесь:

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

}

Предупреждение PVS-Studio: V522 [CWE-690, CERT-MEM52-CPP] Возможно, происходит разыменование потенциального нулевого указателя 'sd_data'.

Проверьте строки: 516, 515. Storage-ext.c 516 Есть и другие подобные предупреждения:

• V522 [CWE-690, CERT-MEM52-CPP] Возможно, происходит разыменование потенциального нулевого указателя «приложение».

  Проверьте строки: 8, 7. диалоги.

  c 8

• V522 [CWE-690, CERT-MEM52-CPP] Возможно, происходит разыменование потенциального нулевого указателя «приложение».

  Проверьте строки: 162, 161. Notification-settings-app.c 162

• V522 [CWE-690, CERT-MEM52-CPP] Возможно, произошло разыменование потенциального нулевого указателя «bench_data».

  Проверьте строки: 81, 79. Storage_settings_scene_benchmark.c 81

• V522 [CWE-690, CERT-MEM52-CPP] Возможно, происходит разыменование потенциального нулевого указателя «приложение».

  Проверьте строки: 18, 16. Storage_settings.c 18

• V575 [CWE-628, CERT-EXP37-C] Потенциальный нулевой указатель передается в функцию strlen. Проверьте первый аргумент. Проверьте строки: 174, 168. Storage-test-app.c 174

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

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

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

Используемый нами распределитель останавливает все выполнение кода прошивки, если выделение не удалось, и вызывает человека с отладчиком.

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

} 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] Возможно, произошло разыменование нулевого указателя 'record_data'.

запись.

с 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] Указатель 'subghz-> txrx-> protocol_result' использовался до его проверки на nullptr. Проверьте строки: 70, 78. subghz_scene_receiver_info.c 70 Хотя в этой статье я обсуждаю различные ошибки, связанные с нулевыми указателями, я должен поблагодарить авторов проекта за качество кода.

Для кода C плотность таких ошибок невелика.

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

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

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

Также мы написали анализатор потребления и освобождения памяти приложением.

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

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

}

Предупреждение PVS-Studio: V779 [CWE-561, CERT-MSC12-C] Обнаружен недостижимый код. Вполне возможно, что ошибка присутствует. subghz_i.c 44 Оператор перерыв и макрос журналирования, очевидно, следует поменять местами.

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

Знаете ли вы, как это произошло на самом деле? Понятно, что ошибка не критичная, но все равно интересно :).

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

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

Когда все могут ошибаться

И непонятно, насколько точен анализатор 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] Неверный формат. Рассмотрите возможность проверки пятого фактического аргумента функции sscanf. Ожидается указатель на тип unsigned int. subghz_cli.c 105 Обратите внимание на строку формата, которая управляет данными при сканировании: «%lx %lu %u».

Это означает, что ожидаются указатели на переменные следующих типов:

1. %лкс — длинное беззнаковое целое число ;
2. %лкс — длинное беззнаковое целое число ;
3. %у — беззнаковое целое число .

1. uint32_t ;
2. uint32_t ;
3. size_t .

описание функции sscanf ).

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

Ошибка.

Третья переменная повторить должен иметь тип int32_t .

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

1. %lx ( длинное беззнаковое целое число ) -> uint32_t ;
2. %lx ( длинное беззнаковое целое число ) -> uint32_t ;
3. %у ( беззнаковое целое число ) -> int32_t ;

Следовательно, этот и другой код работают корректно.

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

Заключение

И если честно, код на C, по сравнению с C++, более подвержен ошибкам, которые могут быть обнаружены статическими анализаторами кода.

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

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

Кстати, я даю им возможность сказать заключительные слова.

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

Он еще далек от идеала, но мы усердно работаем над улучшением читаемости кода и документации.

Мы верим, что сообщество может сделать крутой продукт.

Если вы хотите поделиться этой статьей с англоязычной аудиторией, воспользуйтесь ссылкой для перевода: Андрей Карпов.

PVS-Studio проверяет код Flipper Zero dolphin .

Теги: #Программирование микроконтроллеров #микроконтроллеры #программирование #открытый исходный код #C++ #si #статический анализ кода #открытый исходный код #pvs-studio #embedded #flipperzero #Flipper Zero dolphin

• Аппаратные Кейлоггеры – Что Это Такое?

  19 Oct, 24

• Почему Wordpress Сегодня Является Самой Популярной Cms

  19 Oct, 24

• Дамп Личных Файлов. Как Я Настраивал Файлообменник На Vps

  19 Oct, 24

• Лен, Жан-Мари

  19 Oct, 24

• И Господь Повелел: «Проходите Собеседование И Принимайте Предложения»

  19 Oct, 24

• Поднимаем Ids/Nms: Микротик И Суриката С Веб-Интерфейсом

  19 Oct, 24

• Иногда Я Не Могу Оценить Комментарии Пользователей

  19 Oct, 24

• Как Открыть Стартап В Берлине [Инфографика]

  19 Oct, 24

• Мы Ваши Электронные Соседи

  19 Oct, 24

• Контейнеризируйте Это! Что Такое Fuel И Для Чего Оно Использует Docker?

  19 Oct, 24