# Проверка операционной системы Haiku \(семейство BeOS\) с помощью PVS\-Studio\. Часть 1 Операционные системы являются одними из самых сложных и крупных проектов в мире программного обеспечения, а значит идеально подходят для демонстрации применения методики статического анализа кода\. После проверки Linux Kernel, я вдохновился проанализировать и другие открытые операционные системы\. ![0317_Haiku_1_ru/image1.png](https://import.viva64.com/docx/blog/0317_Haiku_1_ru/image1.png) ## Введение [Haiku](https://www.haiku-os.org/) — свободная операционная система для персональных компьютеров, которая нацелена на двоичную совместимость с операционной системой BeOS\. Haiku воплощает в себе основные идеи BeOS\. Это модульная система, архитектурно решённая как гибридное ядро: микроядерная архитектура, способная динамически подгружать необходимые модули\. Проект для проверки был предложен пользователем, знакомым с продуктом [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) и нашей работе по проверке open\-source проектов\. После сравнительно недавней [проверки Linux Kernel](https://pvs-studio.ru/ru/blog/posts/cpp/0299/), я догадывался, с какими проблемами мне придётся столкнуться и описал их в ответном письме\. Неожиданно мне предложили содействие в сборке операционной системы и интеграции анализатора\. Дополнительно на официальном сайте была доступна очень обширная документация и я решил попробовать\. Через некоторое время я получил долгожданный лог проверки анализатором и после анализа результатов, я решил написать две статьи, описав самые подозрительные на мой взгляд участки кода\. Это первая часть\. ## Результаты проверки В первую статью я вынес предупреждения анализатора на условные операторы\. Ведь ошибку в условии можно трактовать как ошибку в логике выполнения программы\. ### Предупреждения \#1, \#2: [V501](https://pvs-studio.ru/ru/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; } ``` На эту функцию анализатор выдал целых два предупреждения\. В обоих случаях хорошо заметна опечатка \(когда уже анализатор "тыкнул пальцем", конечно\) в именах lJack и rjack\. [V575](https://pvs-studio.ru/ru/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, ' = '); // <= .... } ``` Функция strchr\(\) возвращает указатель на первое вхождение указанного символа в указанной строке\. Символ преобразуется в int, в данном случае ' \= ' будет представлен как число 2112800\. Скорее всего хотели искать одиночный символ '\=', а его код будет 61\. Если хотели найти подстроку " \= ", то явно используется не та функция и её следует заменить на вызов strstr\(\)\. ### Предупреждения \#3, \#4: [V502](https://pvs-studio.ru/ru/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; } ``` К сожалению, в данном случае взятие в скобочки переменной 'ancestorsVisible' никак не влияет на порядок вычислений в этом выражении\. Поэтому первой по приоритету будет выполняться операция вычитания \(из int16 вычитается bool\), только потом выполняется тернарный оператор '?:'\. Правильный код: ```cpp bool IsVisible(bool ancestorsVisible) const { int16 showLevel = BView::Private(view).ShowLevel(); return (showLevel - (ancestorsVisible ? 0 : 1) ) <= 0; } ``` [V502](https://pvs-studio.ru/ru/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)) ``` Также я советую авторам проверить порядок выполнения операций в этом макросе и расставить скобки для наглядности\. ### Предупреждения \#5, \#6 [V562](https://pvs-studio.ru/ru/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; .... } ``` На первый взгляд обычное условие, но "same\_file" является макросом, преобразующимся в логическое выражение, как и 'same\_file\_attributes', в итоге получили странное сравнение "0 < value\_of\_boolean\_type"\. В языке Си нет типа 'bool'\. Выражение справа будет иметь тип 'int', но по своей сути она всегда "истина" или "ложь", поэтому мы и назвали его 'boolean'\. И сравнение "0 < boolean\_expr" весьма подозрительно\. Аналогичное использование макроса: * V562 It's odd to compare 0 or 1 with a value of 0\. cmp\.c 313 [V562](https://pvs-studio.ru/ru/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(); } .... } ``` Ещё одно некорректное сравнение с использованием макросов\. Функция IsProfessionalSpdif\(\) возвращает TRUE или FALSE, а мы сравниваем возвращаемый результат с числом 0x12\. ### Предупреждения \#7, \#8 [V583](https://pvs-studio.ru/ru/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; .... } ``` Независимо от значения переменной 'internal\_encoder', тернарный оператор возвращает одинаковые значения\. Необходимо проверить это место на наличие опечаток\. [V523](https://pvs-studio.ru/ru/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 { .... } .... } ``` Анализатор напоминает, что необходимо "пофиксить" отложенные места\. Ещё такое место: * V523 The 'then' statement is equivalent to the 'else' statement\. mkntfs\.c 1334 ### Предупреждения \#9, \#10 [V503](https://pvs-studio.ru/ru/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; .... } ``` Функция strstr\(\) возвращает указатель на первое вхождение строки "email" в строке 'buffer', если такого соответствия не найдено, то возвращается NULL\. Следовательно, в данном случае необходимо с NULL и сравнивать\. Возможное решение: ```cpp void TTextControl::MessageReceived(BMessage *msg) { .... while (node.GetNextAttrName(buffer) == B_OK) { if (strstr(buffer, "email") == NULL) continue; .... } ``` [V512](https://pvs-studio.ru/ru/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; .... } ``` Длина сравниваемой строки не совпадает с указанным количеством сравниваемых символов\. В строке "Private\-key\-format: v" 21 символ\. ### Предупреждения \#11, \#12 [V547](https://pvs-studio.ru/ru/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++; .... } ``` Скорее всего тут ошибка\. В цикле хотели пропустить все пробелы и символы табуляции, но один символ никак не может быть и тем и другим одновременно\. Возможный корректный вариант: ```cpp static int selection_rel(....) { char *r, *rname; .... while (*r == ' ' || *r == '\t') r++; .... } ``` [V590](https://pvs-studio.ru/ru/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; .... } ``` Здесь всё хорошо, но одна проверка является лишней и её стоит удалить\. Для сохранения логики работы, достаточно оставить: "for \(; path\[i\] \=\= '/'; i\+\+\);"\. Похожие места: * 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 ### Предупреждение \#13, \#14 [V547](https://pvs-studio.ru/ru/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(); } ``` В этой функции что\-то всегда отрисовывается\. Если построить таблицу истинности логического выражения в условии, можно убедиться, что оно всегда истинно\. Возможно, тут должен быть оператор '&&'\. [V547](https://pvs-studio.ru/ru/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; } .... } ``` При таком сравнении с беззнаковым типом, условие всегда ложно и где\-то не выполняется очистка памяти\. К сожалению, подобных проверок с участием беззнаковых типов в коде **около двух сотен**\. Описывать в статье все такие сравнения, или хотя\-бы часть нет никакого смысла\. Они все однотипны и неинтересны\. Однако, мы предоставим разработчикам полный отчёт и они смогут проанализировать все такие подозрительные места\. ### Предупреждения \#15, \#16 [V713](https://pvs-studio.ru/ru/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) { .... } .... } ``` В условии цикла перепутана последовательность проверки указателя\. В начале он разыменовывается, а уже потом проверяется на равенство нулю\. Правильный вариант: ```cpp while (lp != NULL && lp->s != NULL) { ``` [V593](https://pvs-studio.ru/ru/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)) { .... } .... } ``` Анализатор обнаружил потенциальную ошибку в выражении, которое, скорее всего, работает не так, как задумывал программист\. Планировалось выполнить присваивание "err \= SendBuffer\(\)", а результат сравнить с константой 'B\_OK', но приоритет оператора '\!\=' выше, чем у '\=', поэтому в переменную 'err' записывается результат логической операции\. Похожие места: * 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 ### Предупреждения \#17, \#18 [V547](https://pvs-studio.ru/ru/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; } } .... } ``` Скорее всего из\-за оператора перехода 'goto' никогда не замечали, что один из циклов "вечный", т\.к\. при проверке "nogscale \>\= 0" декремент беззнаковой переменной можно делать бесконечно\. [V621](https://pvs-studio.ru/ru/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); } .... } ``` Почему\-то значение счётчика в этом цикле изменяется не в ту сторону, логичнее делать инкремент переменной 'i', чтобы ожидание длилось максимум 100 итераций, а не в миллионы раз больше\. Похожее место: * 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 ### Предупреждение \#19, \#20 [V646](https://pvs-studio.ru/ru/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; } ``` В этой функции нет серьёзной ошибки, но код плохо оформлен\. Необходимо добавить ключевое слово 'else', либо разместить условия на одном уровне\. [V640](https://pvs-studio.ru/ru/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); .... } ``` Макросы и так доставляют неудобства при отладке, так ещё и являются источником вот таких ошибок: макрос 'DO\_NUMBER' раскрывается в несколько строк, но только первая из них будет частью условного оператора, последующие же операторы будут выполняться независимо от условия\. Аналогично неправильно макрос используется здесь: * 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 ## Заключение Благодаря помощи заинтересованных людей в настройке сборки операционной системы и интеграции анализатора, удалось в короткие сроки подготовить всё для проверки\. По сути это идеальный сценарий проверки open\-source проектов, потому что часто приходится сталкиваться с абсолютно незнакомыми проектами и, зачастую, имеющими собственные сборочные системы\. В [следующей статье](https://pvs-studio.ru/ru/blog/posts/cpp/0318/) представлены оставшиеся предупреждения анализатора, о которых я хотел бы рассказать\. Они будут различных типов и разделены на несколько групп\.