# Analysis of Haiku Operating System \(BeOS Family\), by PVS\-Studio, Part 1 Operating systems are among the largest and most complicated software projects, and that means they perfectly suit the purpose of demonstrating the capabilities of static code analysis\. After the successful analysis of the Linux Kernel, I felt inspired to try analyzing other open\-source operating systems as well\. ![0317_Haiku_1/image1.png](https://import.viva64.com/docx/blog/0317_Haiku_1/image1.png) ## Introduction [Haiku](https://www.haiku-os.org/) is a free and open\-source operating system for PC, designed to be binary compatible with the BeOS operating system, and embodying the basic ideas of BeOS\. It's a modular system with the hybrid\-kernel architecture \- microkernel architecture capable of dynamical module linking\. The idea to check this project was suggested by one [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) user familiar with our work on open\-source software analysis\. With the experience of the relatively recent [Linux Kernel analysis](https://pvs-studio.com/en/blog/posts/cpp/0299/), I could predict what troubles I would face checking the Haiku project, and told that guy about them in a reply email\. Unexpectedly, I was offered assistance in building the operating system and analyzer integration\. Also, there was highly extensive documentation available at the official site, so I decided to give it a try\. It was some time before I managed to get the long\-awaited analysis log, and after studying it, I decided to write one large article in two parts, describing code fragments I found to be the most suspicious\. This article is the first part\. ## Analysis results In the first article, I'm discussing the analyzer's warnings on conditional operators, for errors in conditions can be treated as execution logic errors, can't they? ### Warnings No\. 1, 2 [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '<' operator: lJack\-\>m\_jackType < lJack\-\>m\_jackType MediaJack\.cpp 783 ```cpp 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; } ``` This function triggered two warnings at once\. In both cases, there is a typo you can clearly see \(of course, not before the analyzer has "pointed a finger" at it\) in the names lJack and rjack\. [V575](https://pvs-studio.com/en/docs/warnings/v575/) The 'strchr' function processes value '2112800'\. Inspect the second argument\. CommandActuators\.cpp 1517 ```cpp 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 return result of the strchr\(\) function is the pointer to the first occurrence of the specified character in the specified string\. The character is cast to int, and in this case ' \= ' will be presented as the number 2112800\. The programmer most likely intended to search for a single '\=' character, and its code is 61\. If the programmer wanted to find the " \= " substring, the function used in the code is obviously the wrong choice, and should be replaced with the strstr\(\) call\. ### Warnings No\. 3, 4 [V502](https://pvs-studio.com/en/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\-' operator\. AbstractLayout\.cpp 244 ```cpp bool IsVisible(bool ancestorsVisible) const { int16 showLevel = BView::Private(view).ShowLevel(); return (showLevel - (ancestorsVisible) ? 0 : 1) <= 0; } ``` Unfortunately, enclosing the 'ancestorsVisible' variable in parentheses won't affect the expression evaluation order in this case\. Therefore, according to the operation precedence hierarchy, the first operation to be executed is subtraction \(bool is subtracted from int16\), and only then the ternary operator '?:' will be executed\. This is what the correct version of this code should look like: ```cpp bool IsVisible(bool ancestorsVisible) const { int16 showLevel = BView::Private(view).ShowLevel(); return (showLevel - (ancestorsVisible ? 0 : 1) ) <= 0; } ``` [V502](https://pvs-studio.com/en/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '&&' operator\. fnmatch\.c 58 ```cpp #define FOLD(c) \ ((flags & FNM_CASEFOLD) && ISUPPER ((unsigned char) (c)) \ ? tolower ((unsigned char) (c)) \ : (c)) ``` I also recommend that the authors check the operation execution order in this macro, and add parentheses wherever necessary to make it more transparent\. ### Warnings No\. 5, 6 [V562](https://pvs-studio.com/en/docs/warnings/v562/) It's odd to compare 0 or 1 with a value of 0\. cmp\.c 300 ```cpp #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; .... } ``` This is an ordinary condition at first sight, but actually "same\_file" is a macro converted into a logical expression, just like 'same\_file\_attributes', so what we get is a strange comparison "0 < value\_of\_boolean\_type"\. There is no 'bool' type in the C language\. The expression to the right will have the 'int' type, but in fact it is always either "true" or "false", that's why it is called 'boolean'\. So the "0 < boolean\_expr" comparison does look pretty suspicious\. A similar issue with a macro: * V562 It's odd to compare 0 or 1 with a value of 0\. cmp\.c 313 [V562](https://pvs-studio.com/en/docs/warnings/v562/) It's odd to compare a bool type value with a value of 18: 0x12 \=\= IsProfessionalSpdif\(\)\. CEchoGals\_mixer\.cpp 533 ```cpp #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 of macros\. The IsProfessionalSpdif\(\) function returns TRUE or FALSE, while the returned result is compared to the number 0x12\. ### Warnings No\. 7, 8 [V583](https://pvs-studio.com/en/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value\. impactv\.c 520 ```cpp 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 'internal\_encoder' variable, the ternary operator returns identical values\. This code needs to be examined for typos\. [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. mkntfs\.c 1132 ```cpp 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 us that suspended code fragments should be fixed\. Another issue of this kind: * V523 The 'then' statement is equivalent to the 'else' statement\. mkntfs\.c 1334 ### Warnings No\. 9, 10 [V503](https://pvs-studio.com/en/docs/warnings/v503/) This is a nonsensical comparison: pointer <\= 0\. Header\.cpp 900 ```cpp 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 the pointer to the first occurrence of the "email" string in the 'buffer' string\. If no such correspondence is found, NULL is returned\. Therefore, it is NULL that it should be compared to\. A possible solution: ```cpp void TTextControl::MessageReceived(BMessage *msg) { .... while (node.GetNextAttrName(buffer) == B_OK) { if (strstr(buffer, "email") == NULL) continue; .... } ``` [V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'memcmp' function will lead to underflow of the buffer '"Private\-key\-format: v"'\. dst\_api\.c 858 ```cpp dst_s_read_private_key_file(....) { .... if (memcmp(in_buff, "Private-key-format: v", 20) != 0) goto fail; .... } ``` The length of the string being compared doesn't coincide with the specified number of characters to be compared\. The "Private\-key\-format: v" string contains 21 characters\. ### Warnings No\. 11, 12 [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression '\* r && \* r \=\= ' ' && \* r \=\= '\\t'' is always false\. Probably the '\|\|' operator should be used here\. selection\.c 546 ```cpp static int selection_rel(....) { char *r, *rname; .... while (*r && *r == ' ' && *r == '\t') r++; .... } ``` I'm almost sure there is an error here\. The programmer intended to skip all the space characters and tabs in the loop, but one and the same character can't be both at a time\. The possible correct version of this code is as follows: ```cpp static int selection_rel(....) { char *r, *rname; .... while (*r == ' ' || *r == '\t') r++; .... } ``` [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the 'path\[i\] \=\= '/' && path\[i\] \!\= '\\0'' expression\. The expression is excessive or contains a misprint\. storage\_support\.cpp 309 ```cpp 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 OK in this code, but one check is excessive and should be removed\. Without affecting the code logic, we can simply leave the following: "for \(; path\[i\] \=\= '/'; i\+\+\);"\. Other similar fragments: * V590 Consider inspecting this expression\. The expression is excessive or contains a misprint\. PoseView\.cpp 5773 * V590 Consider inspecting this expression\. The expression is excessive or contains a misprint\. Tracker\.cpp 1728 * V590 Consider inspecting the '\* ptr \=\= ';' && \* ptr \!\= '\\0'' expression\. The expression is excessive or contains a misprint\. pc\.c 316 ### Warnings No\. 13, 14 [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. StatusView\.cpp 1397 ```cpp void TDragRegion::Draw(BRect) { .... if (fDragLocation != kDontDrawDragRegion || fDragLocation != kNoDragRegion) DrawDragRegion(); } ``` In this function, something is constantly getting drawn\. If we build the truth table for the logical expression in the condition, we will find that it is always true\. Perhaps the '&&' operator is missing here\. [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'reservedBase < 0' is always false\. Unsigned type value is never < 0\. agp\_gart\.cpp 1172 ```cpp /* 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 a comparison against an unsigned type like this, the condition is always false, and somewhere memory fails to be cleared\. It's sad to say, but there are **about two hundred** similar checks involving unsigned types in the Haiku project\. It wouldn't make sense to talk about all these instances, or even part of them in this article, for they all are very alike and not very interesting\. However, we will send a complete log to the developers, so that they can examine all those suspicious fragments\. ### Warnings No\. 15, 16 [V713](https://pvs-studio.com/en/docs/warnings/v713/) The pointer lp was utilized in the logical expression before it was verified against nullptr in the same logical expression\. util\.c 311 ```cpp char * bittok2str(register const struct tok *lp, ....) { .... while (lp->s != NULL && lp != NULL) { .... } .... } ``` The pointer check order in the loop condition is incorrect\. The pointer is first dereferenced and only then checked for being null\. The correct code: ```cpp while (lp != NULL && lp->s != NULL) { ``` [V593](https://pvs-studio.com/en/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. VideoProducer\.cpp 766 ```cpp 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 has detected a potential error in an expression that is very likely to work differently to how the programmer wanted\. What was intended is to execute the "err \= SendBuffer\(\)" assignment, and compare the result to the 'B\_OK' constant, but the '\!\=' operator's precedence is higher than that of '\=', so the 'err' variable will be used to store the result of the logical operation\. Other similar fragments: * V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. if\_age\.c 590 * V593 Consider reviewing the expression of the 'A \= B \!\= C' kind\. The expression is calculated as following: 'A \= \(B \!\= C\)'\. if\_alc\.c 954 * V593 Consider reviewing the expression of the 'A \= B \>\= C' kind\. The expression is calculated as following: 'A \= \(B \>\= C\)'\. RAW\.cpp 2601 ### Warnings No\. 17, 18 [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'nogscale \>\= 0' is always true\. Unsigned type value is always \>\= 0\. tvp3026\.c 212 ```cpp 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; } } .... } ``` The 'goto' operator was probably the reason the programmer never noticed that one of the loops was infinite, since an unsigned variable can be endlessly decremented in a check like "nogscale \>\= 0"\. [V621](https://pvs-studio.com/en/docs/warnings/v621/) Consider inspecting the 'for' operator\. It's possible that the loop will be executed incorrectly or won't be executed at all\. if\_ae\.c 1670 ```cpp #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 counter in this loop runs in the opposite direction: it would make more sense to increment the 'i' variable so that the program would have to wait for 100 iterations at most, instead of millions of times more\. Another similar issue: * V621 Consider inspecting the 'for' operator\. It's possible that the loop will be executed incorrectly, or won't be executed at all\. if\_ae\.c 1706 ### Warnings No\. 19, 20 [V646](https://pvs-studio.com/en/docs/warnings/v646/) Consider inspecting the application's logic\. It's possible that 'else' keyword is missing\. Filter\.cpp 760 ```cpp 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 badly formatted\. The key word 'else' should be added, or the conditions should be aligned at one level\. [V640](https://pvs-studio.com/en/docs/warnings/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 ```cpp #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 are always a headache in debugging, but moreover, they are often sources of the following bugs: the 'DO\_NUMBER' macro is expanded into several lines, but only the first of them will be part of the conditional operator, while all the next operators will be executed independently of the condition\. Here's another code fragment where a macro is used in a similarly incorrect way: * 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 a few guys interested in setting up the build of the Haiku operating system and analyzer integration, we managed to get everything necessary for analysis ready quickly\. This is in fact an ideal scenario of open\-source software analysis, because we often have to deal with completely unfamiliar projects which often have their own build systems\. In the [next article,](https://pvs-studio.com/en/blog/posts/cpp/0318/) we discussed the remaining warnings I have picked for you\. They are grouped into several categories according to their types\.