Checking the Haiku operating system (BeOS family) with PVS-Studio. Part 1

Operating systems are one of the most complex and large projects in the software world, and therefore ideal for demonstrating the use of static code analysis techniques. After checking the Linux Kernel, I was inspired to analyze other open source operating systems.

Haiku is a free operating system for personal computers that is aimed at binary compatibility with the BeOS operating system. Haiku embodies the basic ideas of BeOS. This is a modular system, architecturally designed as a hybrid core: a microkernel architecture that can dynamically load the necessary modules.
')
The project for verification was proposed by a user familiar with the PVS-Studio product and our work on verifying open-source projects. After a relatively recent verification of the Linux Kernel , I guessed what problems I would have to face and described them in a response letter. Unexpectedly, I was offered assistance in building the operating system and integrating the analyzer. Additionally, very extensive documentation was available on the official website and I decided to try it.

After a while, I received a long-awaited analyzer check log and after analyzing the results, I decided to write two articles describing the most suspicious parts of the code in my opinion. This is the first part.

Test results

In the first article, I issued warnings to the analyzer on conditional statements. After all, an error in the condition can be interpreted as an error in the logic of program execution.

Warnings # 1, # 2:

V501 There are identical to the operator of the operator: lJack-> m_jackType <lJack-> m_jackType MediaJack.cpp 783

int __CORTEX_NAMESPACE__ compareTypeAndID(....) { int retValue = 0; .... if (lJack && rJack) { if (lJack->m_jackType < lJack->m_jackType) //<== { return -1; } if (lJack->m_jackType == lJack->m_jackType) //<== { if (lJack->m_index < rJack->m_index) { return -1; } else { return 1; } } else if (lJack->m_jackType > rJack->m_jackType) { retValue = 1; } } return retValue; } 

The analyzer issued two warnings for this function. In both cases, a typo is clearly noticeable (when the analyzer is already “poked a finger”, of course) in the names lJack and rjack.

V575 The 'strchr' function processes value '2112800'. Inspect the second argument. CommandActuators.cpp 1517

 extern char *strchr(const char *string, int character); SendMessageCommandActuator:: SendMessageCommandActuator(int32 argc, char** argv) : CommandActuator(argc, argv), fSignature((argc > 1) ? argv[1] : "") { .... const char* arg = argv[i]; BString argString(arg); const char* equals = strchr(arg, ' = '); //<== .... } 

The strchr () function returns a pointer to the first occurrence of the specified character in the specified string. The character is converted to an int, in this case, '=' will be represented as the number 2112800. Most likely, they wanted to search for a single '=' character, and its code will be 61.

If you wanted to find the substring "=", then the wrong function is clearly used and it should be replaced with the call to strstr ().

Warnings # 3, # 4:

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '-' operator. AbstractLayout.cpp 244

 bool IsVisible(bool ancestorsVisible) const { int16 showLevel = BView::Private(view).ShowLevel(); return (showLevel - (ancestorsVisible) ? 0 : 1) <= 0; } 

Unfortunately, in this case, parenthesis of the variable 'ancestorsVisible' does not affect the order of calculations in this expression. Therefore, the first in priority will be the operation of subtraction (bool is subtracted from int16), only then the ternary operator '?:' Is executed.

The correct code is:

 bool IsVisible(bool ancestorsVisible) const { int16 showLevel = BView::Private(view).ShowLevel(); return (showLevel - (ancestorsVisible ? 0 : 1) ) <= 0; } 

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a operator. fnmatch.c 58

 #define FOLD(c) \ ((flags & FNM_CASEFOLD) && ISUPPER ((unsigned char) (c)) \ ? tolower ((unsigned char) (c)) \ : (c)) 

I also advise authors to check the order of operations in this macro and place brackets for clarity.

Warnings # 5, # 6

V562 It's odd to compare 0 or 1 with a value of 0. cmp.c 300

 #ifndef same_file # define same_file(s, t) \ ((((s)->st_ino == (t)->st_ino) \ && ((s)->st_dev == (t)->st_dev)) \ || same_special_file (s, t)) #endif int main (int argc, char **argv) { .... if (0 < same_file (&stat_buf[0], &stat_buf[1]) //<== && same_file_attributes (&stat_buf[0], &stat_buf[1]) && file_position (0) == file_position (1)) return EXIT_SUCCESS; .... } 

At first glance, the usual condition, but “same_file” is a macro that converts to a logical expression, like 'same_file_attributes', in the end got a strange comparison of “0 <value_of_boolean_type”. There is no 'bool' type in C language. The expression on the right will be of type 'int', but in its essence it is always “true” or “false”, so we called it 'boolean'. And the comparison “0 <boolean_expr” is quite suspicious.

Similar use of macro:

• V562 It's odd to compare 0 or 1 with a value of 0. cmp.c 313

V562 It's a bit of a value type 18: 0x12 == IsProfessionalSpdif (). CEchoGals_mixer.cpp 533

 #define ECHOSTATUS_DSP_DEAD 0x12 //<== virtual BOOL IsProfessionalSpdif() //<== { .... return( (....) ? TRUE : FALSE ); } ECHOSTATUS CEchoGals::ProcessMixerFunction ( PMIXER_FUNCTION pMixerFunction, INT32 & iRtnDataSz ) { .... case MXF_GET_PROF_SPDIF : if ( ECHOSTATUS_DSP_DEAD == IsProfessionalSpdif() ) //<== { Status = ECHOSTATUS_DSP_DEAD; } else { pMixerFunction->Data.bProfSpdif = IsProfessionalSpdif(); } .... } 

Another incorrect comparison with the use of macros. The IsProfessionalSpdif () function returns TRUE or FALSE, and we compare the return result with the number 0x12.

Warnings # 7, # 8

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value. impactv.c 520

 void Radeon_CalcImpacTVRegisters(....) { .... values->tv_hstart = internal_encoder ? values->tv_hdisp + 1 - params->mode888 + 12 : values->tv_hdisp + 1 - params->mode888 + 12; .... } 

Regardless of the value of the variable 'internal_encoder', the ternary operator returns the same values. You must check this place for typos.

V523 The 'then' statement is equivalent to the 'else' statement. mkntfs.c 1132

 static int insert_positioned_attr_in_mft_record(....) { .... if (flags & ATTR_COMPRESSION_MASK) { hdr_size = 72; /* FIXME: This compression stuff is all wrong. .... */ /* now. (AIA) */ if (val_len) mpa_size = 0; /* get_size_for_compressed_....; */ else mpa_size = 0; } else { .... } .... } 

The analyzer reminds that it is necessary to “fix” the deferred places.

Another place:

• V523 The 'then' statement is equivalent to the 'else' statement. mkntfs.c 1334

Warnings # 9, # 10

V503 This is a nonsensical comparison: pointer <= 0. Header.cpp 900

 extern char *strstr(const char *string, const char *searchString); void TTextControl::MessageReceived(BMessage *msg) { .... while (node.GetNextAttrName(buffer) == B_OK) { if (strstr(buffer, "email") <= 0) continue; .... } 

The strstr () function returns a pointer to the first occurrence of the string “email” in the string 'buffer', if no such match is found, then NULL is returned. Therefore, in this case it is necessary to compare with NULL.

Possible Solution:

 void TTextControl::MessageReceived(BMessage *msg) { .... while (node.GetNextAttrName(buffer) == B_OK) { if (strstr(buffer, "email") == NULL) continue; .... } 

V512 A call to the memcmp of the function will be "Private-key-format: v". dst_api.c 858

 dst_s_read_private_key_file(....) { .... if (memcmp(in_buff, "Private-key-format: v", 20) != 0) goto fail; .... } 

The length of the compared string does not match the specified number of compared characters. The string "Private-key-format: v" is 21 characters.

Warnings # 11, # 12

V547 Expression '* r && * r ==' '&& * r ==' \ t '' is always false. Probably the '||' operator should be used here. selection.c 546

 static int selection_rel(....) { char *r, *rname; .... while (*r && *r == ' ' && *r == '\t') r++; .... } 

Most likely there is an error. In the cycle we wanted to skip all the spaces and tabs, but one character can not be both at the same time.

Possible correct option:

 static int selection_rel(....) { char *r, *rname; .... while (*r == ' ' || *r == '\t') r++; .... } 

V590 Consider inspecting the 'path [i] ==' / '&& path [i]! =' \ 0 '' expression. The expression is misprint. storage_support.cpp 309

 status_t parse_first_path_component(const char *path, int32& length, int32& nextComponent) { .... for (; path[i] == '/' && path[i] != '\0'; i++); //<== if (path[i] == '\0') // this covers "" as well nextComponent = 0; else nextComponent = i; .... } 

Everything is fine here, but one check is superfluous and should be removed. To save the logic of work, it is enough to leave: "for (; path [i] == '/'; i ++);".

Similar places:

• V590 Consider inspecting this expression. The expression is misprint. PoseView.cpp 5773
• V590 Consider inspecting this expression. The expression is misprint. Tracker.cpp 1728
• V590 Consider inspecting the '* ptr =='; ' && * ptr! = '\ 0' 'expression. The expression is misprint. pc.c 316

Warning # 13, # 14

V547 Expression is always true. Probably the '&&' operator should be used here. StatusView.cpp 1397

 void TDragRegion::Draw(BRect) { .... if (fDragLocation != kDontDrawDragRegion || fDragLocation != kNoDragRegion) DrawDragRegion(); } 

In this function, something is always drawn. If you build a truth table of a logical expression in a condition, you can make sure that it is always true. Perhaps there should be an '&&' operator.

V547 Expression 'reservedBase <0' is always false. Unsigned type value is never <0. agp_gart.cpp 1172

 /* address types */ typedef unsigned long int __haiku_addr_t; //<== typedef __haiku_addr_t addr_t; //<== static status_t bind_aperture(...., addr_t reservedBase, ....) { .... if (status < B_OK) { if (reservedBase < 0) //<== aperture->DeleteMemory(memory); return status; } .... } 

In such a comparison with an unsigned type, the condition is always false and somewhere the memory is not cleared. Unfortunately, such checks involving unsigned types in the code are about two hundred . To describe in the article all such comparisons, or at least part of it makes no sense. They are all of the same type and uninteresting. However, we will provide developers with a full report and they will be able to analyze all such suspicious places.

Warnings # 15, # 16

V713 The pointer was used in the same logical expression. util.c 311

 char * bittok2str(register const struct tok *lp, ....) { .... while (lp->s != NULL && lp != NULL) { .... } .... } 

The loop condition is confused with a pointer test sequence. In the beginning it is dereferenced, and only then it is checked for equality to zero. The correct option is:

 while (lp != NULL && lp->s != NULL) { 

V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the following: 'A = (B! = C)'. VideoProducer.cpp 766

 int32 VideoProducer::_FrameGeneratorThread() { .... err = B_OK; // Send the buffer on down to the consumer if (wasCached || (err = SendBuffer(buffer, fOutput.source, fOutput.destination) != B_OK)) { .... } .... } 

The analyzer detected a potential error in the expression, which most likely does not work as the programmer intended. It was planned to perform the assignment “err = SendBuffer ()” and compare the result with the constant 'B_OK', but the priority of the operator '! =' Is higher than that of the = =, therefore the result of the logical operation is written to the variable 'err'.

Similar places:

• V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the following: 'A = (B! = C)'. if_age.c 590
• V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the following: 'A = (B! = C)'. if_alc.c 954
• V593 Consider reviewing the A = B> = C 'kind. The expression is calculated as the following: 'A = (B> = ​​C)'. RAW.cpp 2601

Warnings # 17, # 18

V547 Expression 'nogscale> = 0' is always true. Unsigned type value is always> = 0. tvp3026.c 212

 status_t mil2_dac_init (void) { uint32 rfhcnt, nogscale, memconfig; .... for (nogscale = 1; nogscale >= 0; nogscale--) { //<== int max = -1 + 33.2 * mclk / (nogscale? 1: 4); for (rfhcnt = 15; rfhcnt > 0; rfhcnt--) { int value = (rfhcnt & 0x0e) * 256 + (rfhcnt & 0x01) * 64; LOG(2,("mil2_dac_init factor %d, rfhcnt %2d: %d ?<= %d\n", nogscale, rfhcnt, value, max)); if (value <= max) goto rfhcnt_found; } } .... } 

Most likely due to the transition operator 'goto' they never noticed that one of the cycles is “eternal”, since when checking “nogscale> = 0”, the decrement of an unsigned variable can be made infinitely.

V621 Consider inspecting the 'for' operator. It is possible that you will not be executed at all. if_ae.c 1670

 #define AE_IDLE_TIMEOUT 100 static void ae_stop_rxmac(ae_softc_t *sc) { .... /* * Wait for IDLE state. */ for (i = 0; i < AE_IDLE_TIMEOUT; i--) { val = AE_READ_4(sc, AE_IDLE_REG); if ((val & (AE_IDLE_RXMAC | AE_IDLE_DMAWRITE)) == 0) break; DELAY(100); } .... } 

For some reason, the value of the counter in this loop changes in the wrong direction, it is more logical to increment the variable 'i' so that the wait lasts a maximum of 100 iterations, rather than millions of times more.

Similar place:

• V621 Consider inspecting the 'for' operator. It is possible that you will not be executed at all. if_ae.c 1706

Warning # 19, # 20

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. Filter.cpp 760

 uchar Scaler::Limit(intType value) { if (value < 0) { value = 0; } if (value > 255) { value = 255; } return value; } 

There is no serious error in this function, but the code is poorly designed. You must add the keyword 'else', or place the conditions on the same level.

V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. strftime.c 1263

 #define DO_NUMBER(d, v) \ digits = width == -1 ? d : width; \ number_value = v; goto do_number size_t my_strftime (s, maxsize, format, tp extra_args) { .... if (modifier == L_('O')) goto bad_format; else DO_NUMBER (1, tp->tm_year + TM_YEAR_BASE); .... } 

Macros and so deliver inconvenience when debugging, so also are the source of such errors: the macro 'DO_NUMBER' is expanded into several lines, but only the first one will be part of the conditional statement, the subsequent statements will be executed regardless of the condition.

Similarly, a wrong macro is used here:

• V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. strftime.c 1267

Conclusion

Thanks to the help of interested people in setting up the assembly of the operating system and integrating the analyzer, we were able to quickly prepare everything for verification. In fact, this is an ideal scenario for checking open-source projects, because you often have to deal with completely unfamiliar projects and, often, with your own build systems.

The next article will present the remaining analyzer warnings, which I would like to talk about. They will be of different types and divided into several groups.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 1 .