{"id":380706,"date":"2024-06-29T03:15:14","date_gmt":"2024-06-29T03:15:14","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=380706"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=380706","title":{"rendered":"MuditaOS: Will your alarm clock go off? Part I<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

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.<\/p>\n

<\/figcaption><\/figure>\n

About the project<\/h3>\n

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

We followed the instructions<\/a> 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<\/em>. This is what it looked like:<\/p>\n

<\/figcaption><\/figure>\n

Since the project is regularly updated, I froze it in version 8cc1f77<\/a>.<\/p>\n

The analyzer’s warnings<\/h3>\n

Warnings N1\u2013N3<\/h4>\n

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<\/em> slide:<\/p>\n

<\/figcaption><\/figure>\n

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.<\/p>\n

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.<\/p>\n

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:<\/p>\n

V595<\/a> [CERT-EXP12-C] The ‘result’ pointer was utilized before it was verified against nullptr. Check lines: 81, 82. AudioModel.cpp 81<\/p>\n

void AudioModel::play(....) {   ....   auto cb = [_callback = callback, this](auto response)              {               auto result = dynamic_cast                             <service::AudioStartPlaybackResponse *>(response);               lastPlayedToken = result->token;               if (result == nullptr)                {                 ....               }               ....             };   .... } <\/code><\/pre>\n
In this code fragment, the developer uses dynamic_cast<\/em> for type casting. This operation’s result is a potentially null pointer that is later dereferenced. Then, this the pointer is checked for nullptr<\/em>.<\/p>\n
Fixing this code is easy. First, check the result<\/em> pointer for null. Then use it.<\/p>\n
Below are two cases that are even more interesting:<\/p>\n
V757<\/a> [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<\/p>\n
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data) {   ....   if (auto switchData = dynamic_cast                         <calllog::CallLogSwitchData *>(data); data != nullptr)    {     ....   }   .... } <\/code><\/pre>\n
Here developer uses dynamic_cast<\/em> to cast the pointer to the base class, to the pointer to the derivative. Then the pointer being cast is checked for nullptr<\/em>. However, most likely, the developer intended to check the cast’s result for nullptr<\/em>. In case this is indeed a typo, one can fix the code as follows:<\/p>\n
void CallLogDetailsWindow::onBeforeShow(...., SwitchData *data) {   ....   if (auto switchData = dynamic_cast<calllog::CallLogSwitchData *>(data))    {     ....   }   .... } <\/code><\/pre>\n
It’s possible not everyone likes this fix, but we consider it short and convenient \u2014 we initialize and check the pointer in one operation \u2014 which is why we use the approach everywhere.<\/p>\n
Note. This is different from the case when an existing variable is assigned inside a condition. The code below is considered poor practice:<\/p>\n
int x = ...; if (x = foo()) <\/code><\/pre>\n
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<\/a> about such code \u2014and 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.<\/p>\n
Let’s get back to the project’s code. Below is one similar case:<\/p>\n
V757<\/a> [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<\/p>\n
void PhoneNameWindow::onBeforeShow(ShowMode \/*mode*\/, SwitchData *data) {   if (const auto newData = dynamic_cast<PhoneNameData *>(data);                                                              data != nullptr)    {     ....   } } <\/code><\/pre>\n
The correct code looks like this:<\/p>\n
void PhoneNameWindow::onBeforeShow(ShowMode \/*mode*\/, SwitchData *data) {   if (const auto newData = dynamic_cast<PhoneNameData *>(data))    {     ....   } } <\/code><\/pre>\n
Note that simplifying such checks is one of our code refactoring recommendations we covered in this video.<\/a> Do take a look if you haven’t already! It’s short and you may learn something new \ud83d\ude42<\/p>\n
Warning N4<\/h4>\n
V522<\/a> [CERT-EXP34-C] Dereferencing of the null pointer ‘document’ might take place. TextBlockCursor.cpp 332<\/p>\n
auto BlockCursor::begin() -> std::list<TextBlock>::iterator {   return document == nullptr              ? document->blocks.end() : document->blocks.begin(); } <\/code><\/pre>\n
This code fragment deserves its very own facepalm. Let’s figure out what happens here. The developer explicitly checks the document<\/em> pointer for nullptr<\/em>. 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.<\/p>\n
Warning N5<\/h4>\n
V517<\/a> [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<\/p>\n
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;   }   .... } <\/code><\/pre>\n
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.<\/p>\n
Warnings N6, N7<\/h4>\n