Операционные системы — это программное обеспечение, для которого качество кода имеет решающее значение. На этот раз анализатор PVS-Studio проверил MuditaOS. Итак, давайте посмотрим, что нашел статический анализатор в этой ОС с открытым исходным кодом.
О проекте
MuditaOS — операционная система на базе FreeRTOS, проверенная PVS-Studio некоторое время назад. Что мы нашли? Ознакомьтесь с этой статьей! MuditaOS работает на устройствах Mudita, включая телефон, будильник и часы. Исходный код написан на C и C++. Так. Почему бы нам не взглянуть? Насколько хороши эти будильники на самом деле? :)
Мы следовали инструкциям из официального репозитория и собрали проект под Ubuntu 20.04. Мы проверили отладочную версию будильника Mudita Bell. В конце 2021 года будильник стоил 60 долларов США. Вот как это выглядело:
Поскольку проект регулярно обновляется, я заморозил его на версии 8cc1f77.
Предупреждения анализатора
Предупреждения N1–N3
Прежде чем перейти к ошибкам, расскажу об одном забавном случае. Недавно я читал лекцию в Тульском государственном университете о неопределенном поведении. Вот что я написал на слайде bio:
Это требует небольшого пояснения. При анализе кода анализатор PVS-Studio строит абстрактное синтаксическое дерево, представляющее код проекта. Это один из промежуточных этапов анализа. Узлы дерева представляют различные языковые конструкции. Последние расположены в соответствии с иерархией наследования. От узла к узлу языковые конструкции преобразуются посредством приведения типов.
Когда я только начинал работать с PVS-Studio, у меня несколько раз падал анализатор (во время пробных прогонов), потому что я был слишком уверен, что знаю тип узла, к которому я приводил узел базового типа.
Сегодня я докажу вам, что, как и я, разработчики MuditaOS не слишком любят проверять результаты приведения типов. Посмотрим, о чем предупреждает анализатор:
V595 [CERT-EXP12-C] Указатель результат использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 81, 82. AudioModel.cpp 81
void AudioModel::play(....)
{
....
auto cb = [_callback = callback, this](auto response)
{
auto result = dynamic_cast
<service::AudioStartPlaybackResponse *>(response);
lastPlayedToken = result->token;
if (result == nullptr)
{
....
}
....
};
....
}В этом фрагменте кода разработчик использует dynamic_cast для приведения типов. Результатом этой операции является потенциально нулевой указатель, который позже разыменовывается. Затем этот указатель проверяется на nullptr.
Исправить этот код легко. Сначала проверьте указатель result на значение null. Тогда используйте его.
Ниже приведены два случая, которые еще более интересны:
V757 [CERT-EXP12-C] Возможно, некорректная переменная сравнивается с nullptr после преобразования типа с помощью dynamic_cast. Проверить строки: 214, 214. CallLogDetailsWindow.cpp 214
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data)
{
....
if (auto switchData = dynamic_cast
<calllog::CallLogSwitchData *>(data); data != nullptr)
{
....
}
....
}Здесь разработчик использует dynamic_cast для приведения указателя к базовому классу, к указателю на производный. Затем приводимый указатель проверяется на nullptr. Однако, скорее всего, разработчик намеревался проверить результат приведения на наличие nullptr. Если это действительно опечатка, код можно исправить следующим образом:
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data)
{
....
if (auto switchData = dynamic_cast<calllog::CallLogSwitchData *>(data))
{
....
}
....
}Возможно, этот фикс не всем нравится, но мы считаем его коротким и удобным — мы инициализируем и проверяем указатель за одну операцию — поэтому везде используем такой подход.
Примечание. Это отличается от случая, когда существующая переменная присваивается внутри условия. Код ниже считается плохой практикой:
int x = ...;
if (x = foo())Непонятно, пытались ли они написать сравнение, но допустили опечатку или действительно намеревались присвоить и проверить переменную одновременно. Большинство компиляторов и анализаторов предупреждают о таком коде — и это правильно. Код опасный и непонятный. Однако совсем другое дело, когда кто-то создает новую переменную, как показано в примере. Там кто-то попытался создать новую переменную и инициализировать ее определенным значением. Вы не сможете выполнить там операцию ==, как бы сильно вам этого ни хотелось.
Вернемся к коду проекта. Ниже приведен один похожий случай:
V757 [CERT-EXP12-C] Возможно, неверная переменная сравнивается с nullptr после преобразования типа с использованием dynamic_cast. Проверьте строки: 47, 47. PhoneNameWindow.cpp 47
void PhoneNameWindow::onBeforeShow(ShowMode /*mode*/, SwitchData *data)
{
if (const auto newData = dynamic_cast<PhoneNameData *>(data);
data != nullptr)
{
....
}
}Правильный код выглядит так:
void PhoneNameWindow::onBeforeShow(ShowMode /*mode*/, SwitchData *data)
{
if (const auto newData = dynamic_cast<PhoneNameData *>(data))
{
....
}
}Обратите внимание, что упрощение таких проверок — одна из наших рекомендаций по рефакторингу кода, которую мы рассмотрели в этом видео. Загляните, если еще не видели! Коротко и можно узнать что-то новое :)
Предупреждение N4
V522 [CERT-EXP34-C] Может иметь место разыменование нулевого указателя document. TextBlockCursor.cpp 332
auto BlockCursor::begin() -> std::list<TextBlock>::iterator
{
return document == nullptr
? document->blocks.end() : document->blocks.begin();
}Этот фрагмент кода заслуживает отдельного фейспалма. Давайте разберемся, что здесь происходит. Разработчик явно проверяет указатель document на наличие nullptr. Затем указатель разыменовывается в обеих ветвях тернарного оператора. Код корректен только в том случае, если разработчик стремился сломать программу.
Предупреждение N5
V517 [CERT-MSC01-C] Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 1053, 1056. avdtp_util.c 1053
static uint16_t avdtp_signaling_setup_media_codec_mpeg_audio_config_event(....)
{
uint8_t channel_mode_bitmap = ....;
....
if (....)
{
....
}
else if (channel_mode_bitmap & 0x02)
{
num_channels = 2;
channel_mode = AVDTP_CHANNEL_MODE_STEREO;
}
else if (channel_mode_bitmap & 0x02)
{
num_channels = 2;
channel_mode = AVDTP_CHANNEL_MODE_JOINT_STEREO;
}
....
}Здесь мы видим классический скопированный код. Есть два способа понять и исправить этот код: либо вторая ветка должна содержать другую проверку, либо вторая проверка избыточна и ее нужно убрать. Поскольку две ветки содержат разную логику, я предполагаю, что здесь применим первый вариант. В любом случае, я рекомендую разработчикам MuditaOS взглянуть на этот фрагмент кода.
Предупреждения N6, N7
- V571 Периодическая проверка. Условие if (activeInput) уже проверено в строке 249. ServiceAudio.cpp 250
- V547 Выражение activeInput всегда истинно. ServiceAudio.cpp 250
std::optional<AudioMux::Input *> AudioMux::GetActiveInput();....auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage> { .... if (const auto activeInput = audioMux.GetActiveInput(); activeInput) { if (activeInput) { retCode = activeInput.value()->audio->SetOutputVolume(clampedValue); } } .... }
Давайте исследовать. Тип activeinput — это объект std::Optional из указателя на AudioMax::input. Вложенный оператор if содержит вызов функции-члена value. Функция гарантированно вернет указатель и не вызовет исключения. После этого результат разыменовывается.
Однако функция может возвращать либо действительный, либо нулевой указатель. План вложенного оператора if, вероятно, заключался в проверке этого указателя. Хм, мне также нравится заключать указатели и логические значения в std::Optional! И потом каждый раз переживать одно и то же :).
Фиксированный код:
std::optional<AudioMux::Input *> AudioMux::GetActiveInput();....auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage> { .... if (const auto activeInput = audioMux.GetActiveInput(); activeInput) { if (*activeInput) { retCode = (*activeInput)->audio->SetOutputVolume(clampedValue); } } .... }
Предупреждение N8–N11
V668 [CERT-MEM52-CPP] Нет смысла тестировать указатель pcBuffer на null, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. syscalls_stdio.cpp 384
int _iosys_fprintf(FILE *__restrict __stream,
const char *__restrict __format, ...)
{
constexpr auto buf_len = 4096;
char *pcBuffer;
....
pcBuffer = new char[buf_len];
if (pcBuffer == NULL)
{
....
}
}Здесь значение указателя, которое возвращает оператор new (насколько я могу судить, не перегруженный), сравнивается с NULL. Однако если оператору new не удается выделить память, то в соответствии со стандартом языка генерируется исключение std::bad_alloc(). Следовательно, проверка указателя на null не имеет смысла.
Тем более в коде операционной системы, функционирующей в режиме реального времени. Скорее всего, в случаях, когда память не может быть выделена, программа рухнет и последующий код будет просто недоступен.
Проверка может иметь место, если используется перегрузка nothrow для new:
int _iosys_fprintf(FILE *__restrict __stream,
const char *__restrict __format, ...)
{
constexpr auto buf_len = 4096;
char *pcBuffer;
....
pcBuffer = new (std::nothrow) char[buf_len];
if (pcBuffer == NULL)
{
....
}
}Анализатор обнаружил еще несколько таких случаев.
- V668 [CERT-MEM52-CPP] Нет смысла тестировать указатель fontData на null, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. FontManager.cpp 56
- V668 [CERT-MEM52-CPP] Проверять указатель data на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. ImageManager.cpp 85
- V668 [CERT-MEM52-CPP] Проверять указатель data на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. ImageManager.cpp 131
Предупреждение N12
V509 [CERT-DCL57-CPP] Функция noexcept = вызывает функцию setName, которая потенциально может вызвать исключение. Подумайте о том, чтобы обернуть его в блок try..catch. Device.cpp 48
struct Device { static constexpr auto NameBufferSize = 240; .... void setName(const std::string &name) { if (name.size() > NameBufferSize) { throw std::runtime_error("Requested name is bigger than buffer size"); } strcpy(this->name.data(), name.c_str()); } .... }....Devicei &Devicei::operator=(Devicei &&d) noexcept { setName(d.name.data()); }
Здесь анализатор обнаружил, что функция, помеченная как noexcept, вызывает функцию, которая генерирует исключение. Если в теле функции nothrow возникает исключение, функция nothrow вызывает std::terminate, и программа аварийно завершает работу.
Может иметь смысл обернуть функцию setName в блок function-try и обработать исключительную ситуацию там — или вместо генерации исключения можно использовать что-то другое.
Предупреждения N13–N18
Анализатор обнаружил множество фрагментов кода, содержащих бессмысленные проверки. Давайте рассмотрим некоторые из них, а остальные оставим разработчикам:
V547 Выражение snoozeCount == 0 всегда верно. NotificationProvider.cpp 117
void NotificationProvider::handleSnooze(unsigned snoozeCount) { if (snoozeCount > 0) { notifications[NotificationType::AlarmSnooze] = std::make_shared<notifications::AlarmSnoozeNotification>(snoozeCount); } else if (snoozeCount == 0) { notifications.erase(NotificationType::AlarmSnooze); }send(); }
Как видно из кода, переменная snoozeCount имеет беззнаковый тип и, следовательно, не может быть меньше нуля. Так что вторая проверка лишняя. Код станет более кратким, если мы заменим else if на безусловный else:
void NotificationProvider::handleSnooze(unsigned snoozeCount) { if (snoozeCount > 0) { notifications[NotificationType::AlarmSnooze] = std::make_shared<notifications::AlarmSnoozeNotification>(snoozeCount); } else { notifications.erase(NotificationType::AlarmSnooze); }send(); }
Анализатор также выдал предупреждение на этот фрагмент кода:
V547 Выражение currentState == ButtonState::Off всегда истинно. КнопкаOnOff.cpp 33
enum class ButtonState : bool
{
Off,
On
};
....
void ButtonOnOff::switchState(const ButtonState newButtonState)
{
currentState = newButtonState;
if (currentState == ButtonState::On)
{
....
}
else if (currentState == ButtonState::Off)
{
....
}
}Это предупреждение интересно, потому что обычно разработчики могут просто его подавить. Давайте разберемся, что здесь происходит: у нас есть enum с базовым типом bool и двумя состояниями, которые мы проверяем.
Все мы знаем, что разработчики часто расширяют перечисления и добавляют новые значения. Со временем это перечисление может получить больше состояний, и общее количество может превысить два. Тогда анализатор перестал бы предупреждать об этом фрагменте кода.
Однако я хотел бы обратить ваше внимание на то, что это состояние кнопки. На нее можно нажать — или нет — но я сомневаюсь, что авторы планируют в ближайшее время изобрести кнопку Шредингера и добавить третье состояние. Вы можете использовать тот же подход для исправления этого кода — замените else if безусловным else.
void ButtonOnOff::switchState(const ButtonState newButtonState)
{
currentState = newButtonState;
if (currentState == ButtonState::On)
{
....
}
else
{
....
}
}Вот еще несколько V547, на которые стоит обратить внимание:
- V547 Выражение status != 0x00 всегда ложно. AVRCP.cpp 68
- V547 Выражение ‘stream_endpoint-›close_stream == 1’ всегда ложно. avdtp.c 1223
- V547 Выражение ‘stream_endpoint-›abort_stream == 1’ всегда ложно. avdtp.c 1256
- V547 Выражение what == info_type::start_sector всегда верно. disk_manager.cpp 340
Предупреждение N19
V609 [CERT-EXP37-C] Разделить на ноль. Функция qfilter_CalculateCoeffs обрабатывает значение 0. Проверьте третий аргумент. Проверьте строки: Equalizer.cpp:26, unittest_equalizer.cpp:91. Эквалайзер.cpp 26
// Equalizer.cpp QFilterCoefficients qfilter_CalculateCoeffs( FilterType filter, float frequency, uint32_t samplerate, float Q, float gain) { constexpr auto qMinValue = .1f; constexpr auto qMaxValue = 10.f; constexpr auto frequencyMinValue = 0.f;if (frequency < frequencyMinValue && filter != FilterType::FilterNone) { throw std::invalid_argument("Negative frequency provided"); } if ((Q < qMinValue || Q > qMaxValue) && filter != FilterType::FilterNone) { throw std::invalid_argument("Q out of range"); } .... float omega = 2 * M_PI * frequency / samplerate; .... } .... // unittest_equalizer.cpp const auto filterNone = qfilter_CalculateCoeffs(FilterType::FilterNone, 0, 0, 0, 0);
Да, модульный тест был тем, что вызвало здесь анализатор. Тем не менее, я думаю, что этот случай интересен и может быть хорошим примером. Это очень странная операция, и наш межмодульный анализ ее обнаружил.
Кстати, межмодульный анализ — это большая новинка в анализаторе PVS-Studio. Дополнительные сведения об этой функции см. в этой статье.
Но вернемся к предупреждению. Здесь разработчик, написавший тест, скорее всего, не заглянул внутрь функции qfilter_CalculateCoeffs. Результат деления на 0 следующий:
- для целых — неопределённое поведение, после которого нет смысла что-либо тестировать, так как всё может случиться;
- для действительных чисел — значение ±Inf, если рассматриваемый тип поддерживает арифметику с числами с плавающей запятой, согласно IEC 559 / IEEE 754, в противном случае это поведение undefined, как и для целых чисел.
Здесь у нас есть число с плавающей запятой. Поэтому при делении на 0 мы, скорее всего, получим бесконечность. Результат, вероятно, не обрадует автора кода. Нажмите здесь, чтобы узнать больше по этой теме.
В итоге мы видим, что тест содержит явно опасный код, препятствующий корректному тестированию продукта.
Предупреждения N20–N21
V617 Рассмотрим осмотр состояния. Аргумент purefs::fs::inotify_flags::close_write побитовой операции | содержит ненулевое значение. Инотифихандлер.cpp 76
V617 Рассмотрим осмотр состояния. Аргумент purefs::fs::inotify_flags::del побитовой операции | содержит ненулевое значение. Инотифихандлер.cpp 79
namespace purefs::fs { enum class inotify_flags : unsigned { attrib = 0x01, close_write = 0x02, close_nowrite = 0x04, del = 0x08, move_src = 0x10, move_dst = 0x20, open = 0x40, dmodify = 0x80, }; .... }sys::MessagePointer InotifyHandler::handleInotifyMessage (purefs::fs::message::inotify *inotify) { .... if (inotify->flags && (purefs::fs::inotify_flags::close_write | purefs::fs::inotify_flags::move_dst)) { .... } else if (inotify->flags && ( purefs::fs::inotify_flags::del | purefs::fs::inotify_flags::move_src)) { .... } .... }
Этот случай выглядит как классический шаблон, когда разработчик хочет убедиться, что один из флагов установлен в inotify-›flags. В первом случае это close_write или move_dst, во втором — del или move_src соответственно.
Давайте подумаем, как мы можем это осуществить. Для этого сначала нам нужно соединить константы с помощью операции | — именно это и сделал разработчик. Затем убедитесь, что один из них установлен в flags с помощью операции &.
Этот фрагмент кода выглядит странно и вряд ли корректен. Второй операнд оператора && всегда истинен.
Скорее всего, разработчик перепутал логические && и побитовые &. Правильный код выглядит следующим образом:
sys::MessagePointer InotifyHandler::handleInotifyMessage
(purefs::fs::message::inotify *inotify)
{
....
if (inotify->flags
& (purefs::fs::inotify_flags::close_write
| purefs::fs::inotify_flags::move_dst))
{
....
}
else if (inotify->flags
& ( purefs::fs::inotify_flags::del
| purefs::fs::inotify_flags::move_src))
{
....
}
....
}Заключение
В этой статье я описал лишь часть всех предупреждений GA, которые PVS-Studio нашла в этом проекте. На самом деле их больше. Так же стоит отметить, что это еще не конец — дальше я напишу о том интересном, что нашел анализатор PVS-Studio в MuditaOS. У нас будет как минимум еще одна статья, в которой мы будем постоянно искать ответ на один простой вопрос — «Будет ли ваш будильник звенеть в конце концов?»
Мы также рекомендуем разработчикам MuditaOS самостоятельно запустить анализатор PVS-Studio для своего проекта и осмотреть проблемные места. Это бесплатно для проектов с открытым исходным кодом.
