Vladislav Stolyarov

Feb 01 2022

Tags:

#Cpp #OS #Embedded

MuditaOS: Will your alarm clock go off? Part I

Feb 01 2022

Author: Vladislav Stolyarov

About the project
The analyzer's warnings
Conclusion

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.

About the project

MuditaOS is an operating system based on FreeRTOS that PVS-Studio checked a while ago. What did we find? Check out this article! 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 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:

Since the project is regularly updated, I froze it in version 8cc1f77.

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:

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 [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) 
              {
                ....
              }
              ....
            };
  ....
}

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

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:

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:

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

The correct code looks like this:

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. Do take a look if you haven't already! It's short and you may learn something new :)

Warning N4

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

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 [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;
  }
  ....
}

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 Recurring check. The 'if (activeInput)' condition was already verified in line 249. ServiceAudio.cpp 250
V547 Expression 'activeInput' is always true. 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);
    }
  }
  ....
}

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 :).

The fixed code:

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

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 overload of new is employed:

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

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

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:

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

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.

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

Here are a few more V547 that are worth paying attention to:

V547 Expression 'status != 0x00' is always false. AVRCP.cpp 68
V547 Expression 'stream_endpoint->close_stream == 1' is always false. avdtp.c 1223
V547 Expression 'stream_endpoint->abort_stream == 1' is always false. avdtp.c 1256
V547 Expression 'what == info_type::start_sector' is always true. disk_manager.cpp 340

Warning N19

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);

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.

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

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:

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 analyzer on their own for their project and inspect the problem areas. This is free for open-source projects.

#Cpp #OS #Embedded