# MuditaOS: Will your alarm clock go off? Part I

Operating systems are a kind of software where code quality is critical\. This time the PVS\-Studio analyzer checked MuditaOS\. So let's take a look at what the static analyzer found in this open\-source OS\.

![0914_MuditaOS_1/image1.png](https://import.viva64.com/docx/blog/0914_MuditaOS_1/image1.png)

## About the project

[MuditaOS](https://mudita.com/) is an operating system based on FreeRTOS that PVS\-Studio checked a while ago\. What did we find? Check out [this article](https://pvs-studio.com/en/blog/posts/cpp/0684/)\! MuditaOS runs on Mudita devices that include a phone, alarm clocks, and a watch\. The source code is in C and C\+\+\. So\. Why don't we take a look? How good are these alarm clocks, really? :\)

We followed the [instructions](https://github.com/mudita/MuditaOS/blob/master/doc/quickstart.md) from the official repository and built the project under Ubuntu 20\.04\. We checked the debug version for the Mudita Bell alarm clock\. At the end of 2021 the alarm clock cost _$60_\. This is what it looked like:

![0914_MuditaOS_1/image2.png](https://import.viva64.com/docx/blog/0914_MuditaOS_1/image2.png)

Since the project is regularly updated, I froze it in version [8cc1f77](https://github.com/mudita/MuditaOS/tree/8cc1f779db7824a665240c5a89f1590a5831502c)\.

## The analyzer's warnings

### Warnings N1–N3

Before moving on to errors, I'll tell you about one amusing case\. I've recently given a lecture at Tula State University about undefined behavior\. Here's what I wrote on the _bio_ slide:

![0914_MuditaOS_1/image3.png](https://import.viva64.com/docx/blog/0914_MuditaOS_1/image3.png)

This requires a bit of a clarification\. During code analysis, the PVS\-Studio analyzer builds an abstract syntax tree that represents the project's code\. This is one of the intermediate stages of analysis\. The tree's nodes represent various language constructs\. The latter ones are positioned according to the inheritance hierarchy\. From node to node, the language constructs are converted through casts\.

When I was just starting out at PVS\-Studio, I crashed the analyzer several times \(during trial runs\), because I was too sure that I knew the type of the node to which I was casting the base type node\.

Today I'll prove to you that, same as me, MuditaOS developers do not like checking type casts' results too much\. Let's see what the analyzer warns about:

[V595](https://pvs-studio.com/en/docs/warnings/v595/) \[CERT\-EXP12\-C\] The 'result' pointer was utilized before it was verified against nullptr\. Check lines: 81, 82\. AudioModel\.cpp 81

```cpp
void AudioModel::play(....)
{
  ....
  auto cb = [_callback = callback, this](auto response) 
            {
              auto result = dynamic_cast
                            <service::AudioStartPlaybackResponse *>(response);
              lastPlayedToken = result->token;
              if (result == nullptr) 
              {
                ....
              }
              ....
            };
  ....
}
```

In this code fragment, the developer uses _dynamic\_cast_ for type casting\. This operation's result is a potentially null pointer that is later dereferenced\. Then, this the pointer is checked for _nullptr_\. 

Fixing this code is easy\. First, check the _result_ pointer for null\. Then use it\.

Below are two cases that are even more interesting:

[V757](https://pvs-studio.com/en/docs/warnings/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

```cpp
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data)
{
  ....
  if (auto switchData = dynamic_cast
                        <calllog::CallLogSwitchData *>(data); data != nullptr) 
  {
    ....
  }
  ....
}
```

Here developer uses _dynamic\_cast_ to cast the pointer to the base class, to the pointer to the derivative\. Then the pointer being cast is checked for _nullptr_\. However, most likely, the developer intended to check the cast's result for _nullptr_\. In case this is indeed a typo, one can fix the code as follows:

```cpp
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data)
{
  ....
  if (auto switchData = dynamic_cast<calllog::CallLogSwitchData *>(data)) 
  {
    ....
  }
  ....
}
```

It's possible not everyone likes this fix, but we consider it short and convenient — we initialize and check the pointer in one operation — which is why we use the approach everywhere\.

Note\. This is different from the case when an existing variable is assigned inside a condition\. The code below is considered poor practice:

```cpp
int x = ...;
if (x = foo())
```

It's not clear whether they attempted to write a comparison, but made a typo or whether they truly intended to assign and check the variable simultaneously\. Most compilers and analyzers [warn](https://pvs-studio.com/en/docs/warnings/v559/) about such code —and rightfully so\. The code is dangerous and unclear\. However, it's a completely different matter when someone creates a new variable as is shown in the example\. There someone attempted to create a new variable and initialize it with a specific value\. You wouldn't be able to perform the \=\= operation there, no matter how bad you might want it\.

Let's get back to the project's code\. Below is one similar case:

[V757](https://pvs-studio.com/en/docs/warnings/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

```cpp
void PhoneNameWindow::onBeforeShow(ShowMode /*mode*/, SwitchData *data)
{
  if (const auto newData = dynamic_cast<PhoneNameData *>(data); 
                                                            data != nullptr) 
  {
    ....
  }
}
```

The correct code looks like this:

```cpp
void PhoneNameWindow::onBeforeShow(ShowMode /*mode*/, SwitchData *data)
{
  if (const auto newData = dynamic_cast<PhoneNameData *>(data)) 
  {
    ....
  }
}
```

Note that simplifying such checks is one of our code refactoring recommendations we covered in [this video\.](https://pvs-studio.com/en/blog/video/10513/) Do take a look if you haven't already\! It's short and you may learn something new :\)

### Warning N4

[V522](https://pvs-studio.com/en/docs/warnings/v522/) \[CERT\-EXP34\-C\] Dereferencing of the null pointer 'document' might take place\. TextBlockCursor\.cpp 332

```cpp
auto BlockCursor::begin() -> std::list<TextBlock>::iterator
{
  return document == nullptr 
            ? document->blocks.end() : document->blocks.begin();
}
```

This code fragment deserves its very own facepalm\. Let's figure out what happens here\. The developer explicitly checks the _document_ pointer for _nullptr_\. Then the pointer is dereferenced in both branches of the ternary operator\. The code is correct only if the developer aimed to crash the program\.

### Warning N5

[V517](https://pvs-studio.com/en/docs/warnings/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

```cpp
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;
  }
  ....
}
```

Here we can see classic copy\-pasted code\. There are two ways to understand and fix this code: either the second branch should contain a different check, or the second check is redundant and needs to be removed\. Since the two branches contain different logic, I assume the first variant applies here\. In any case, I recommend MuditaOS developers to take a look at this code snippet\.

### Warnings N6, N7

* [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(activeInput\)' condition was already verified in line 249\. ServiceAudio\.cpp 250
* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'activeInput' is always true\. ServiceAudio\.cpp 250

```cpp
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);
    }
  }
  ....
}
```

Let's investigate\. The _activeinput_ type is an _std::optional_ entity from the pointer to _AudioMax::input_\. The nested _if_ statement contains the _value member function call\._ The function is guaranteed to return the pointer and will not throw an exception\. After, the result is dereferenced\.

However, the function may return either a valid — or a null pointer\. The plan for the nested _if_ statement was probably to check this pointer\. Hm, I also like wrapping pointers and boolean values in _std::optional_\! And then going through the same grief each time :\)\.

![0914_MuditaOS_1/image4.png](https://import.viva64.com/docx/blog/0914_MuditaOS_1/image4.png)

The fixed code:

```cpp
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);
    }
  }
  ....
}
```

### Warning N8–N11

[V668](https://pvs-studio.com/en/docs/warnings/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

```cpp
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) 
  {
    ....
  }
}
```

Here the pointer value, that the _new_ operator \(which is not overloaded, as far as I can tell\) returns_,_ is compared to _NULL_\. However, if the _new_ operator fails to allocate memory, then, according to the language standard, the _std::bad\_alloc\(\)_ exception is generated\. Consequently, checking the pointer for null makes no sense\.

Even less so in the code of an operating system that functions in real time\. Most likely, in cases when memory cannot be allocated, the program will crash and the code that follows will be simply unreachable\.

The check may take place if the [_nothrow_](https://en.cppreference.com/w/cpp/memory/new/nothrow) overload of _new_ is employed:

```cpp
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) 
  {
    ....
  }
}
```

The analyzer found several more such cases\.

* [V668](https://pvs-studio.com/en/docs/warnings/v668/) \[CERT\-MEM52\-CPP\] There is no sense in testing the 'fontData' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. FontManager\.cpp 56
* [V668](https://pvs-studio.com/en/docs/warnings/v668/) \[CERT\-MEM52\-CPP\] There is no sense in testing the 'data' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. ImageManager\.cpp 85
* [V668](https://pvs-studio.com/en/docs/warnings/v668/) \[CERT\-MEM52\-CPP\] There is no sense in testing the 'data' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. ImageManager\.cpp 131

### Warning N12

[V509](https://pvs-studio.com/en/docs/warnings/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

```cpp
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());
}
```

Here the analyzer detected that a function, marked as _noexcept_, calls a function that throws an exception\. If an exception arises from the nothrow function's body, the nothrow function calls _std::terminate_, and the program crashes\.

It could make sense to wrap the _setName_ function in the function\-try block and process the exceptional situation there — or one could use something else instead of generating the exception\.

### Warnings N13–N18

The analyzer found many code fragments that contain meaningless checks\. Let's examine a few of them, and leave the rest to the developers:

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'snoozeCount \=\= 0' is always true\. NotificationProvider\.cpp 117

```cpp
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();
}
```

As is obvious from the code, the _snoozeCount_ variable is of an unsigned type — and, consequently, cannot be less than zero\. So the second check is redundant\. The code becomes more concise if we replace _else if_ with the conditionless _else_:

```cpp
void NotificationProvider::handleSnooze(unsigned snoozeCount)
{
  if (snoozeCount > 0) 
  {
    notifications[NotificationType::AlarmSnooze] =
       std::make_shared<notifications::AlarmSnoozeNotification>(snoozeCount);
  }
  else
  {
    notifications.erase(NotificationType::AlarmSnooze);
  }

  send();
}
```

The analyzer also issued a warning for this code fragment:

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'currentState \=\= ButtonState::Off' is always true\. ButtonOnOff\.cpp 33

```cpp
enum class ButtonState : bool
{
  Off,
  On
};
....
void ButtonOnOff::switchState(const ButtonState newButtonState)
{
  currentState = newButtonState;
  if (currentState == ButtonState::On) 
  {
    ....
  }
  else if (currentState == ButtonState::Off) 
  {
    ....
  }
}
```

This warning is interesting, because normally developers could just suppress it\. Let's figure out what happens here: we have an _enum_ with the underlying _bool_ type and two states that we are checking\. 

We all know that developers often expand enumerations and add new values\. With time, this enumeration could obtain more states and the total could exceed two\. Then the analyzer would have stopped warning about this code fragment\. 

However, I would like to draw your attention to the fact that this is a button's state\. It can be clicked — or not — but I doubt that the authors are planning to invent a Schroedinger button any time soon and add a third state\. You can use the same approach to fix this code — replace _else if_ with the unconditional _else_\.

```cpp
void ButtonOnOff::switchState(const ButtonState newButtonState)
{
  currentState = newButtonState;
  if (currentState == ButtonState::On) 
  {
    ....
  }
  else
  {
    ....
  }
}
```

Here are a few more [V547](https://pvs-studio.com/en/docs/warnings/v547/) that are worth paying attention to:

* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'status \!\= 0x00' is always false\. AVRCP\.cpp 68
* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'stream\_endpoint\-\>close\_stream \=\= 1' is always false\. avdtp\.c 1223
* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'stream\_endpoint\-\>abort\_stream \=\= 1' is always false\. avdtp\.c 1256
* [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'what \=\= info\_type::start\_sector' is always true\. disk\_manager\.cpp 340

### Warning N19

[V609](https://pvs-studio.com/en/docs/warnings/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

```cpp
// 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);
```

Yes, a unit test was what triggered the analyzer here\. However, I think that this case is interesting and could be a good example\. This is a very strange operation and our intermodular analysis detected it\.

By the way, intermodular analysis is a large new feature in the PVS\-Studio analyzer\. For more information on this feature, see [this article](https://pvs-studio.com/en/blog/posts/cpp/0851/)\.

But let's get back to the warning\. Here the developer who wrote the test most likely did not look inside the _qfilter\_CalculateCoeffs_ function\. The result of dividing by _0_ is the following:

* for integers — undefined behavior, after which there's no point testing anything, since anything can happen;
* for real numbers — the _±Inf_ value if the type in question supports arithmetic with floating point numbers, according to the _IEC 559_ / _IEEE 754_, otherwise it's undefined behavior, same as for integers\.

Here we have a floating point number\. This is why when dividing by _0_, we will most likely get infinity\. The result probably would not make the code author happy\. Click [here](https://en.cppreference.com/w/cpp/language/operator_arithmetic) to learn more on this topic\.

As a result, we see that the test contains clearly dangerous code that prevents the correct testing of the product\.

### Warnings N20–N21

[V617](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v617/) Consider inspecting the condition\. The 'purefs::fs::inotify\_flags::del' argument of the '\|' bitwise operation contains a non\-zero value\. InotifyHandler\.cpp 79

```cpp
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)) 
  {
    ....
  }
  ....
}
```

This case looks like a classic pattern when a developer wants to make sure that one of the flags is set in _inotify\-\>flags_\. In the first case it's _close\_write_ or _move\_dst_, in the second cast it's _del_ or _move\_src_ consequently\.

Let's think about how we can make this happen\. To do this, first, we need to join constants through the use of the _\|_ operation — that's exactly what the developer did\. Then make sure that one of them is set in _flags_ through the _&_ operation\.

This code fragment looks strange and is hardly correct\. The && operator's second operand is always true\.

Most likely, the developer mixed up the logical _&&_ and the bitwise _&_\. The correct code is as follows:

```cpp
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)) 
  {
    ....
  }
  ....
}
```

## Conclusion

In this article, I've described only a part of all GA warnings that PVS\-Studio found in this project\. In fact, there are more of them\. It's also worth pointing out that it's not the end — I'll write more on the interesting things that the PVS\-Studio analyzer found in MuditaOS\. We will have at least one more article where we will keep looking to answer one simple question — "Will your alarm clock ring after all?"

We also recommend MuditaOS developers to run the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/) analyzer on their own for their project and inspect the problem areas\. This is [free](https://pvs-studio.com/en/pvs-studio/try-free/) for open\-source projects\.