# PVS\-Studio: анализируем код операционной системы ReactOS Проверив код ReactOS, я смог исполнить сразу три своих желания\. Во\-первых, давно хотелось написать статью об обыкновенном проекте\. Не интересно проверять код таких проектов, как Chromium\. Он слишком качественен и, на поддержание этого качества тратятся ресурсы, недоступные в обыкновенных проектах\. Во\-вторых, появился хороший пример, на котором можно показать, как необходим статический анализ в большом проекте, особенно если он разрабатывается разнородным распределенным коллективом\. В\-третьих, я получил подтверждение, что PVS\-Studio становится всё лучше и полезнее\. ## PVS\-Studio становится все лучше и лучше ![a0076_ReactOS_ru/image1.png](https://import.viva64.com/docx/blog/a0076_ReactOS_ru/image1.png) Начну с последнего момента, по поводу пользы инструмента [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. ReactOS косвенно подтверждает, что PVS\-Studio развивается в правильном направлении\. Вот новость о проверке ReactOS с помощью такого тяжеловеса, как Coverity \- "Статический анализ в Coverity" \[[1](https://reactos.org/blogs/newsletter-79/)\]\. Я, конечно, знаю и понимаю, что до возможностей Coverity нам далеко\. Но, тем не менее, там, где благодаря Coverity "было найдено несколько новых ошибок", PVS\-Studio находит их целый вагон и маленькую тележку\. При этом никакой код никуда отправлять не надо\. Можно просто взять и проверить проект\. Значит мы на верном пути\. ## Что такое ReactOS? ReactOS \- это современная, свободная и открытая операционная система, основанная на архитектуре Windows XP/2003\. Система была написана с нуля и имеет своей целью повторение архитектуры Windows\-NT, созданной Microsoft, от аппаратного до прикладного уровня\. Объем исходного кода на языке Си, Си\+\+ и ассемблере составляет порядка 220 мегабайт\. Различные ссылки: * [Официальный сайт проекта ReactOS](https://reactos.org/)\. * [Как принять участие в разработке ReactOS](https://reactos.org/developing/)\. * Wikipedia\. [ReactOS](https://ru.wikipedia.org/wiki/ReactOS)\. * Статья в Компьютерре\. [ReactOS: догнать и перегнать](http://old.computerra.ru/focus/332964/)\. ## Ошибки в ReactOS Теперь поговорим о вагоне ошибок, которые повстречались мне в коде ReactOS\. Конечно, все они в статью не войдут\. [Здесь](https://pvs-studio.com/external-pictures/txt/pvs-studio-vs-reactos-ru-unicode.txt) я выложил текстовый файл с описанием ошибок, которые я заметил в ходе анализа\. Файл содержит диагностические сообщения с именами файлов и номерами строк\. Также я выделил ошибки в короткий код и дал некоторые комментарии\. Поэтому тем, кто захочет сделать правки в ReactOS, лучше руководствоваться этим файлом, а не статьёй\. А еще лучше скачать и самим проверить проект с помощью PVS\-Studio\. Ведь я не знаком с проектом и выписывал только те ошибки, которые мне понятны\. По поводу многих фрагментов кода я не знаю, содержат они ошибки или нет\. Так что мой анализ достаточно поверхностен\. Ключ для проверки выделим\. Ошибки, которые можно встретить в ReactOS разнообразнейшие\. Просто зоопарк\. Есть опечатки из одного символа\. ```cpp BOOL WINAPI GetMenuItemInfoA(...) { ... mii->cch = mii->cch; ... } ``` На самом деле должно быть написано вот так: "mii\-\>cch \= miiW\-\>cch;"\. Потеряли букву 'W'\. Как результат, программы уже не могут доверять функции GetMenuItemInfoA\. А вот другая опечатка в один символ\. В этот раз некорректно работает сравнение двух имён\. ```cpp static void _Stl_loc_combine_names(_Locale_impl* L, const char* name1, const char* name2, locale::category c) { if ((c & locale::all) == 0 || strcmp(name1, name1) == 0) ... } ``` Есть путаница между оператором && и &\. Очень распространенная ошибка\. Встречаю практически в каждом проекте, где работают с битами или атрибутами файлов\. ```cpp static LRESULT APIENTRY ACEditSubclassProc() { ... if ((This->options && ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) ... } ``` Корректный код должен выглядеть так "\(This\-\>options & ACO\_AUTOSUGGEST\)"\. Пример ниже содержит родственную ей ошибку, из\-за которой всё условие всегда ложно\. ```cpp void adns__querysend_tcp(adns_query qu, struct timeval now) { ... if (!(errno == EAGAIN || EWOULDBLOCK || errno == EINTR || errno == ENOSPC || errno == ENOBUFS || errno == ENOMEM)) { ... } ``` Если присмотреться, то можно заметить коварный фрагмент: "\|\| EWOULDBLOCK \|\|"\. Кстати, в ReactOS нашлось достаточно много условий, которые всегда истинны или ложны\. Некоторые нестрашные, так как, например, располагаются в макросе assert\(\)\. Но есть, на мой взгляд, и критичные\. ```cpp INT WSAAPI connect(IN SOCKET s, IN CONST struct sockaddr *name, IN INT namelen) { ... /* Check if error code was due to the host not being found */ if ((Status == SOCKET_ERROR) && (ErrorCode == WSAEHOSTUNREACH) && (ErrorCode == WSAENETUNREACH)) { ... } ``` Согласитесь, что реализация таких функций как "connect" должна быть протестирована максимально полно\. А здесь мы имеем условие, которое всегда ложно\. Быстро заметить дефект не так просто, поэтому выделю суть ошибки: ```cpp (ErrorCode == 10065) && (ErrorCode == 10051) ``` Кстати, часть, связанная с сокетами, вообще выглядит сырой\. Возможно, это связано с тем, что в Linux мире SOCKET принято объявлять как знаковый тип\. А в Windows он беззнаковый: ```cpp typedef UINT_PTR SOCKET; ``` Как результат имеем разнообразные ошибки в операциях сравнения: ```cpp void adns_finish(adns_state ads) { ... if (ads->tcpsocket >= 0) adns_socket_close(ads->tcpsocket); ... } ``` Выражение "ads\-\>tcpsocket \>\= 0" не имеет смысла, так как всегда истинно\. Встречаются просто странные фрагменты\. Скорее всего, это недописанные и забытые участки кода\. ```cpp if (ERROR_SUCCESS == hres) { Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1); if (Names[count]) strcmpW(Names[count], szValue); } ``` Зачем вызывать "strcmpW", если результат никак не используется? Имеются ошибки, связанные с приоритетом операций\. ```cpp VOID NTAPI AtapiDmaInit(...) { ... ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7; ... } ``` Я расставлю скобки, чтобы стало ясно, как работает это выражение на самом деле: ```cpp ULONG treg = (0x54 + (dev < 3)) ? (dev << 1) : 7; ``` Следующая ошибка обязательно встречается в любом большом проекте\. Есть парочка и в ReactOS\. Речь идет о лишней точке с запятой \- ';'\. ```cpp BOOLEAN CTEScheduleEvent(PCTE_DELAYED_EVENT Event, PVOID Context) { ... if (!Event->Queued); { Event->Queued = TRUE; Event->Context = Context; ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue); } ... } ``` Ещё мне нравятся ошибки с инициализацией элементов массива\. Не знаю почему\. Они трогательны\. Возможно, я вспоминаю свои первые эксперименты с массивами на языке Basic\. ```cpp HPALETTE CardWindow::CreateCardPalette() { ... //include button text colours cols[0] = RGB(0, 0, 0); cols[1] = RGB(255, 255, 255); //include the base background colour cols[1] = crBackgnd; //include the standard button colours... cols[3] = CardButton::GetHighlight(crBackgnd); cols[4] = CardButton::GetShadow(crBackgnd); ... } ``` Можно продолжать приводить разные интересные участки кода\. К сожалению, тогда статья станет слишком длинной и нужно останавливаться\. Напомню, что посмотреть на другие ошибки, найденные мной в ReactOS, можно вот в этом [файле](https://pvs-studio.com/external-pictures/txt/pvs-studio-vs-reactos-ru-unicode.txt)\. На сладкое только приведу вот этот кусочек кода: ```cpp #define SWAP(a,b,c) c = a;\ a = b;\ a = c ``` Пример использования: ```cpp BOOL FASTCALL IntEngGradientFillTriangle(...) { ... SWAP(v2,v3,t); ... } ``` Это – шедевр\. ## Статический анализ кода Я считаю ReactOS очень хорошим примером проекта, где регулярный статический анализ кода просто необходим\. И дело здесь не в квалификации разработчиков\. Проект большой и содержит разнообразные подсистемы\. Это значит, что над таким проектом всегда трудится большое количество людей\. А в большом коллективе всегда кто\-то программирует хуже, кто\-то лучше\. Кто\-то использует один стиль, кто\-то другой\. Но никто не застрахован от ошибок\. Вот смотрите\. Один человек взял и написал в ReactOS вот так: ```cpp if ((res = setsockopt(....) == -1)) ``` Код делает не то, что хочется\. Корректный вариант: if \(\(res \= setsockopt\(\.\.\.\.\)\) \=\= \-1\)\. Если придерживаться практики писать константу в начале, то случайно никогда не сделаешь ошибочное присваивание внутри оператора "if"\. У нас здесь другой тип ошибки\. Но если писать код по приведенному правилу, то не получится сделать ошибку и в рассматриваемом выражении: "if \(\-1 \=\= res \= setsockopt\(\.\.\.\.\)\)"\. Вот только знание этой практики не мешает другому человеку ошибиться альтернативным способом\. ```cpp static DWORD CALLBACK RegistrationProc(LPVOID Parameter) { ... if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR, UnknownError, sizeof(UnknownError) / sizeof(UnknownError[0] - 20))) ... } ``` Здесь вначале красиво написана константа 0\. Вот только круглая скобка закрывается не там, где надо\. Обыкновенная опечатка\. К чему я все эти примеры? А к тому, что никто из нас программистов не идеален\. И ни стандарт кодирования, ни технологии программирования, ни самоконтроль не гарантируют отсутствие ошибок в коде\. В крупных проектах просто необходимы такие вспомогательные технологии, как динамический и статический анализ\. Подчеркну: **Считаю, что статический анализ кода должен являться обязательным элементом цикла разработки ReactOS и других крупных проектов\.** Поясню своё утверждение\. В подобных системах невозможно приблизиться к 100% покрытию кода при тестировании с помощью юнит\-тестов или регрессионных тестов\. Немного уточню\. Можно конечно, но затраты на создание и поддержание таких тестов становятся недопустимо высоки\. Причина в том, что уж очень велико количество возможных состояний системы и возможных путей выполнения ветвей кода\. Некоторые ветви крайне редко получают управление, но от этого не становятся ненужными\. Именно здесь и получается увидеть преимущество, которым обладает статический анализ\. Он проверяет весь код, в независимости от того, как часто он получает управление в процессе работы программы\. Пример проверки кода, редко получающего управление: ```cpp static HRESULT STDMETHODCALLTYPE CBindStatusCallback_OnProgress(...) { ... if (This->szMimeType[0] != _T('\0')) _tprintf(_T("Length: %I64u [%s]\n"), This->Size, This->szMimeType); else _tprintf(_T("Length: %ull\n"), This->Size); ... } ``` Скорее всего, изначально код был написан неправильно\. Потом кто\-то заметил, что сообщение формируется неправильно и внес исправление, написав "%I64u"\. А вот на код по соседству он не обратил внимание\. И там по\-прежнему имеется некорректный формат "%ull"\. Видимо ветка вызывается крайне редко\. Статический анализ такое не пропустит\. Собственно он и не пропустил, раз я могу продемонстрировать этот пример\. Другим хорошим примером может служить большое количество ошибок очистки памяти, которые я увидел ReactOS\. Почему их так много, мне понятно\. Никто не тестирует, заполнилась память или нет\. Во\-первых, сложно осознать, что в этих простых местах можно ошибиться\. А во\-вторых, не так просто проверить очистился внутри функции какой\-то временный буфер или нет\. Здесь статический анализ вновь на высоте\. Приведу только пару примеров\. На самом деле я насчитал как минимум 13 ошибок заполнения массивов константным значением\. ```cpp #define MEMSET_BZERO(p,l) memset((p), 0, (l)) char *SHA384_End(SHA384_CTX* context, char buffer[]) { ... MEMSET_BZERO(context, sizeof(context)); ... } ``` Очищаем только первые байты массива, так как sizeof\(context\) возвращает размер указателя, а не структуры\. ```cpp #define RtlFillMemory(Destination, Length, Fill) \ memset(Destination, Fill, Length) #define IOPM_FULL_SIZE 8196 HalpRestoreIopm(VOID) { ... RtlFillMemory(HalpSavedIoMap, 0xFF, IOPM_FULL_SIZE); ... } ``` Перепутаны аргументы при использовании макроса RtlFillMemory\. Вызов должен быть таким: ```cpp RtlFillMemory(HalpSavedIoMap, IOPM_FULL_SIZE, 0xFF); ``` ## Снова о табуляциях и пробелах _Заранее хочу попросить не начинать в комментариях новую бурную дискуссию на эту тему\. Я просто выскажу своё мнение\. С ним можно быть согласным или нет\. Но_ _обсуждать_ _не_ _стоит\._ Есть два непримиримых лагеря\. Одни за использование табуляции в коде, так как это позволяет подстраивать отображение кода под себя \[[2](https://habr.com/ru/post/118208/)\]\. Другие говорят, что это всё равно не работает и, что нет разумных причин использовать символы табуляции\. От табуляции только вред и разъезжающееся форматирование\. К их числу отношусь и я \[[3](https://habr.com/ru/company/pvs-studio/blog/118204/)\]\. Можно сколько угодно говорить о том, что табуляции надо использовать правильно и тогда всё будет хорошо\. К сожалению, это говорят те, кто работает замкнуто над одним проектом и не сталкиваются с внешним миром\. В любом открытом или просто большом проекте, не удаётся достичь нормального оформления кода, если разрешить использовать табуляцию в любом виде\. Я не буду заниматься абстрактными рассуждениями\. В этот раз я просто приведу своим оппонентам наглядный пример\. Сейчас таким примером станет код ReactOS\. В стандарте кодирования ReactOS написано хорошее правило с теоретической точки зрения: **Generic note about TABs usage: Don't use TABs for formatting; use TABs for indenting only and use only spaces for formatting\.** ```cpp Example: NTSTATUS SomeApi(IN Type Param1, [spaces]IN Type Param2) { [TAB]ULONG MyVar; [TAB]MyVar = 0; [TAB]if ((MyVar == 3) && [TAB][sp](Param1 == TRUE)) [TAB]{ [TAB][TAB]CallSomeFunc(); ... ``` Поклонники табуляции довольны\. Однако я открываю исходные коды ReactOS и наблюдаю во многих местах испорченное форматирование\. Почему? ![a0076_ReactOS_ru/image2.png](https://import.viva64.com/docx/blog/a0076_ReactOS_ru/image2.png) ![a0076_ReactOS_ru/image3.png](https://import.viva64.com/docx/blog/a0076_ReactOS_ru/image3.png) ![a0076_ReactOS_ru/image4.png](https://import.viva64.com/docx/blog/a0076_ReactOS_ru/image4.png) Ответ конечно очевиден\. Потому, что это сложно помнить, где надо нажать TAB, а где поставить несколько пробелов, если это не единственный проект, с которым работаешь\. Вот люди постоянно и ошибаются\. А раз так, нечего быть теоретиками, а надо быть практиками\. Надо взять и запретить использовать табуляцию вообще\. И тогда всё у всех будет одинаково, а виновника, кто начинает использовать табуляцию, легко найти и сделать ему внушение\. Это не шаг назад в оформлении кода\! Это шаг вперёд\! Это следующий уровень осознания\. Теоретическая красота настраиваемых отступов не сочетается с практикой\. В первую очередь важно обеспечить однозначное представление кода и легкость разработки в большой команде\. В компании Google это понимают\. И в их стандарте для форматирования используются только пробелы \[[5](https://google.github.io/styleguide/cppguide.html)\]\. Тем, кто ратует за использование табуляции, еще раз рекомендую задумываться, почему распределенная команда высококлассных профессионалов, которые разрабатывают Chromium, выбрала именно пробелы\. И еще раз\. Теоретическая красота настраиваемых отступов не сочетается с практикой\. Неважно, как красиво звучит теория, если она не работает\. А именно так и обстоит дело в ReactOS\. Рекомендую команде разрабатывающей ReactOS модифицировать их стандарт и отказаться от использования табуляции\. Любая табуляция должна расцениваться, как ошибка и быть устранена из кода\. Кстати, подобная практика позволит находить вот такие вот безобразия в коде ReactOS: ```cpp BOOLEAN KdInitSystem(IN ULONG BootPhase, IN PLOADER_PARAMETER_BLOCK LoaderBlock) { ... /* Check if this is a comma, a space or a tab */ if ((*DebugOptionEnd == ',') || (*DebugOptionEnd == ' ') || (*DebugOptionEnd == ' ')) ... } ``` Последнее сравнение, это сравнение с символом табуляции, а не с пробелом, как может показаться\. Нормальный код должен выглядеть так: "\(\*DebugOptionEnd \=\= '\\t'\)"\. **Примечание для сторонников табуляции\.** Не надо мне вновь рассказывать, как правильно использовать табуляции\. И это не мой код\. Смотрите, вот есть вполне конкретный проект ReactOS\. В нем есть плохо отформатированный код\. Подумайте, какие действия позволят сделать так, чтобы новый программист мог открыть код проекта и не гадать, какой размер символа табуляции ему необходимо установить в настройках редактора\. Мысли в духе "нужно сразу было писать правильно" практической ценности не имеют\. ## Библиографический список 1. [Выпуск новостей ReactOS N79\. Статический анализ в Coverity\.](https://reactos.org/blogs/newsletter-79/) 1. [Пора завязывать использовать пробелы вместо табуляции в коде\.](https://habr.com/ru/post/118208/) 1. [Пора завязывать использовать символы табуляции в коде\.](https://habr.com/ru/company/pvs-studio/blog/118204/) 1. [Google C\+\+ Style Guide](https://google.github.io/styleguide/cppguide.html)\.