Операционные системы – это тот вид софта, для которого качество исходного кода особенно критично. В этот раз под объективы анализатора PVS-Studio попала MuditaOS. Так давайте же посмотрим, что нам удалось найти в данной операционной системе с открытым исходным кодом.
MuditaOS – это операционная система, которая основана на FreeRTOS (статья о проверке). Данный продукт работает на устройствах Mudita, среди которых есть телефон, будильники и часы. Исходный код программы написан на языках С и С++. Ну что, давайте попробуем проверить этот проект? Насколько будильники окажутся хороши? :)
Сборка проекта происходила под Ubuntu 20.04 по инструкции из официального репозитория. Проверялась отладочная версия для будильника Mudita Bell. По информации на конец 2021 года, он стоит 60$ и выглядит так:
Так как проект регулярно обновляется, для проверки я зафиксировал его версией 8cc1f77.
Перед тем как перейти к ошибкам, я хочу рассказать один забавный случай. Недавно мне довелось выступать с лекцией в ТулГУ про неопределённое поведение. Там, в слайде о себе, я представился так:
Тут нужно немного пояснить: в анализаторе PVS-Studio одно из промежуточных представлений кода – абстрактное синтаксическое дерево. В нём разные языковые конструкции реализованы в виде узлов и имеют иерархию наследования. Преобразования между узлами как раз происходят при помощи кастов.
В начале работы в PVS-Studio я несколько раз валил анализатор (при пробных запусках), полагаясь на то, что я без сомнений знаю точный тип узла, к которому преобразую узел базового типа.
Сейчас я докажу вам, что разработчики MuditaOS тоже не очень любят проверять результат операции по преобразованию типов. Давайте посмотрим предупреждения анализатора:
V595 [CERT-EXP12-C] The 'result' pointer was utilized before it was verified against nullptr. Check lines: 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 перед его использованием.
Также удалось обнаружить пару более интересных случаев:
V757 [CERT-EXP12-C] It is possible that an incorrect variable is compared with nullptr after type conversion using 'dynamic_cast'. Check lines: 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] It is possible that an incorrect variable is compared with nullptr after type conversion using 'dynamic_cast'. Check lines: 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))
{
....
}
}Также хочется обратить внимание на то, что упрощение подобных проверок – один из наших советов по рефакторингу кода из этого видео. Если кто-то ещё не видел – советую посмотреть. Оно короткое и познавательное :)
V522 [CERT-EXP34-C] Dereferencing of the null pointer 'document' might take place. TextBlockCursor.cpp 332
auto BlockCursor::begin() -> std::list<TextBlock>::iterator
{
return document == nullptr
? document->blocks.end() : document->blocks.begin();
}Этот фрагмент кода заслуживает отдельного фейспалма. Давайте разберёмся, что тут происходит: мы видим явную проверку указателя document на nullptr и его разыменование в обоих ветках тернарного оператора. Данный код корректен только в том случае, если целью программиста было уронить программу.
V517 [CERT-MSC01-C] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 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 стоит обратить внимание на этот фрагмент кода.
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 от указателя на AudioMux::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);
}
}
....
}V668 [CERT-MEM52-CPP] There is no sense in testing the 'pcBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. 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(). Таким образом, проверять указатель на равенство нулю не имеет смысла.
Особенно странно писать такое в коде операционной системы, которая работает в режиме реального времени. Скорее всего, в ситуации, когда невозможно выделить память, будет происходить завершение работы программы, и дальнейший код будет просто недостижим.
Проверка может иметь место при использовании 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)
{
....
}
}Анализатор обнаружил ещё несколько подобных случаев:
V509 [CERT-DCL57-CPP] The noexcept function '=' calls function 'setName' which can potentially throw an exception. Consider wrapping it in a try..catch block. 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-функции вызовется std::terminate, и программа аварийно завершится.
Возможно, стоит подумать о том, чтобы обернуть функцию setName в function-try блок и обработать там возникшую исключительную ситуацию или заменить генерацию исключения на что-то ещё.
Анализатор обнаружил очень много фрагментов кода, в которых происходят бессмысленные проверки. Давайте разберём парочку из них, а остальные оставим на откуп разработчикам:
V547 Expression 'snoozeCount == 0' is always true. 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 Expression 'currentState == ButtonState::Off' is always true. ButtonOnOff.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, на которые стоит обратить внимание:
V609 [CERT-EXP37-C] Divide by zero. The 'qfilter_CalculateCoeffs' function processes value '0'. Inspect the third argument. Check lines: 'Equalizer.cpp:26', 'unittest_equalizer.cpp:91'. Equalizer.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 будет:
У нас число с плавающей точкой, поэтому в результате деления на 0, скорее всего, получится бесконечность. Как мне кажется, автора кода не порадовал бы этот результат. Подробнее данную тему можно изучить тут.
В итоге мы видим явно опасный код, который мешает корректно тестировать продукт.
V617 Consider inspecting the condition. The 'purefs::fs::inotify_flags::close_write' argument of the '|' bitwise operation contains a non-zero value. InotifyHandler.cpp 76
V617 Consider inspecting the condition. The 'purefs::fs::inotify_flags::del' argument of the '|' bitwise operation contains a non-zero value. InotifyHandler.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. Будет как минимум ещё одна статья, в которой мы продолжим искать ответ на вопрос: "А зазвонит ли ваш будильник?".
Также мы рекомендуем разработчикам MuditaOS самостоятельно запустить анализатор PVS-Studio на своём проекте и изучить проблемные места. Это бесплатно для проектов с открытым исходным кодом.