html текст
All interests
  • All interests
  • Design
  • Food
  • Gadgets
  • Humor
  • News
  • Photo
  • Travel
  • Video
Click to see the next recommended page
Like it
Don't like
Add to Favorites

PVS-Studio: анализируем код операционной системы ReactOS

PVS-Studio vs ReactOS
Проверив код ReactOS, я смог исполнить сразу три своих желания. Во-первых, давно хотелось написать статью об обыкновенном проекте. Не интересно проверять код таких проектов, как Chromium. Он слишком качественен и, на поддержание этого качества тратятся ресурсы, недоступные в обыкновенных проектах. Во-вторых, появился хороший пример, на котором можно показать, как необходим статический анализ в большом проекте, особенно если он разрабатывается разнородным распределенным коллективом. В-третьих, я получил подтверждение, что PVS-Studio становится всё лучше и полезнее.

PVS-Studio становится все лучше и лучше


Начну с последнего момента, по поводу пользы инструмента PVS-Studio. ReactOS косвенно подтверждает, что PVS-Studio развивается в правильном направлении. Вот новость о проверке ReactOS с помощью такого тяжеловеса, как Coverity — «Статический анализ в Coverity» [1]. Я, конечно, знаю и понимаю, что до возможностей Coverity нам далеко. Но, тем не менее, там, где благодаря Coverity «было найдено несколько новых ошибок», PVS-Studio находит их целый вагон и маленькую тележку. При этом никакой код никуда отправлять не надо. Можно просто взять и проверить проект. Значит мы на верном пути.

Что такое ReactOS?


ReactOS — это современная, свободная и открытая операционная система, основанная на архитектуре Windows XP/2003. Система была написана с нуля и имеет своей целью повторение архитектуры Windows-NT, созданной Microsoft, от аппаратного до прикладного уровня. Объем исходного кода на языке Си, Си++ и ассемблере составляет порядка 220 мегабайт.

Различные ссылки:

Ошибки в ReactOS


Теперь поговорим о вагоне ошибок, которые повстречались мне в коде ReactOS. Конечно, все они в статью не войдут. Здесь я выложил текстовый файл с описанием ошибок, которые я заметил в ходе анализа. Файл содержит диагностические сообщения с именами файлов и номерами строк. Также я выделил ошибки в короткий код и дал некоторые комментарии. Поэтому тем, кто захочет сделать правки в ReactOS, лучше руководствоваться этим файлом, а не статьёй.

А еще лучше скачать и самим проверить проект с помощью PVS-Studio. Ведь я не знаком с проектом и выписывал только те ошибки, которые мне понятны. По поводу многих фрагментов кода я не знаю, содержат они ошибки или нет. Так что мой анализ достаточно поверхностен. Ключ для проверки выделим.

Ошибки, которые можно встретить в ReactOS разнообразнейшие. Просто зоопарк. Есть опечатки из одного символа.
BOOL WINAPI GetMenuItemInfoA(...)
{
  ...
  mii->cch = mii->cch;
  ...
}

На самом деле должно быть написано вот так: «mii->cch = miiW->cch;». Потеряли букву 'W'. Как результат, программы уже не могут доверять функции GetMenuItemInfoA.

А вот другая опечатка в один символ. В этот раз некорректно работает сравнение двух имён.
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)
  ...
}

Есть путаница между оператором && и &. Очень распространенная ошибка. Встречаю практически в каждом проекте, где работают с битами или атрибутами файлов.
static LRESULT APIENTRY ACEditSubclassProc()
{
  ...
  if ((This->options && ACO_AUTOSUGGEST) &&
      ((HWND)wParam != This->hwndListBox))
  ...
}

Корректный код должен выглядеть так "(This->options & ACO_AUTOSUGGEST)". Пример ниже содержит родственную ей ошибку, из-за которой всё условие всегда ложно.
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(). Но есть, на мой взгляд, и критичные.
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» должна быть протестирована максимально полно. А здесь мы имеем условие, которое всегда ложно. Быстро заметить дефект не так просто, поэтому выделю суть ошибки:
(ErrorCode == 10065) && (ErrorCode == 10051)

Кстати, часть, связанная с сокетами, вообще выглядит сырой. Возможно, это связано с тем, что в Linux мире SOCKET принято объявлять как знаковый тип. А в Windows он беззнаковый:
typedef UINT_PTR SOCKET;

Как результат имеем разнообразные ошибки в операциях сравнения:
void adns_finish(adns_state ads) {
  ...
  if (ads->tcpsocket >= 0) adns_socket_close(ads->tcpsocket);
  ...
}

Выражение «ads->tcpsocket >= 0» не имеет смысла, так как всегда истинно.

Встречаются просто странные фрагменты. Скорее всего, это недописанные и забытые участки кода.
if (ERROR_SUCCESS == hres)
{
  Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1);
  if (Names[count])
     strcmpW(Names[count], szValue);
}

Зачем вызывать «strcmpW», если результат никак не используется?

Имеются ошибки, связанные с приоритетом операций.
VOID NTAPI
AtapiDmaInit(...)
{
  ...
  ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7;
  ...
}

Я расставлю скобки, чтобы стало ясно, как работает это выражение на самом деле:
ULONG treg = (0x54 + (dev < 3)) ? (dev << 1) : 7;

Следующая ошибка обязательно встречается в любом большом проекте. Есть парочка и в ReactOS. Речь идет о лишней точке с запятой — ';'.
BOOLEAN
CTEScheduleEvent(PCTE_DELAYED_EVENT Event,
                 PVOID Context)
{
  ...
  if (!Event->Queued);
  {
    Event->Queued = TRUE;
    Event->Context = Context;
    ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue);
  }
  ...
}

Ещё мне нравятся ошибки с инициализацией элементов массива. Не знаю почему. Они трогательны. Возможно, я вспоминаю свои первые эксперименты с массивами на языке Basic.
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, можно вот в этом файле. На сладкое только приведу вот этот кусочек кода:
#define SWAP(a,b,c)  c = a;\
                      a = b;\
                      a = c

Пример использования:
BOOL FASTCALL
IntEngGradientFillTriangle(...)
{
  ...
  SWAP(v2,v3,t);
  ...
}

Это – шедевр.

Статический анализ кода


Я считаю ReactOS очень хорошим примером проекта, где регулярный статический анализ кода просто необходим. И дело здесь не в квалификации разработчиков. Проект большой и содержит разнообразные подсистемы. Это значит, что над таким проектом всегда трудится большое количество людей. А в большом коллективе всегда кто-то программирует хуже, кто-то лучше. Кто-то использует один стиль, кто-то другой. Но никто не застрахован от ошибок. Вот смотрите.

Один человек взял и написал в ReactOS вот так:
if ((res = setsockopt(....) == -1))

Код делает не то, что хочется. Корректный вариант: if ((res = setsockopt(....)) == -1). Если придерживаться практики писать константу в начале, то случайно никогда не сделаешь ошибочное присваивание внутри оператора «if». У нас здесь другой тип ошибки. Но если писать код по приведенному правилу, то не получится сделать ошибку и в рассматриваемом выражении: «if (-1 == res = setsockopt(....))».

Вот только знание этой практики не мешает другому человеку ошибиться альтернативным способом.
static DWORD CALLBACK
RegistrationProc(LPVOID Parameter)
{
  ...
  if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
                        UnknownError,
                        sizeof(UnknownError) /
                        sizeof(UnknownError[0] - 20)))
  ...
}

Здесь вначале красиво написана константа 0. Вот только круглая скобка закрывается не там, где надо. Обыкновенная опечатка.

К чему я все эти примеры? А к тому, что никто из нас программистов не идеален. И ни стандарт кодирования, ни технологии программирования, ни самоконтроль не гарантируют отсутствие ошибок в коде.

В крупных проектах просто необходимы такие вспомогательные технологии, как динамический и статический анализ. Подчеркну:

Считаю, что статический анализ кода должен являться обязательным элементом цикла разработки ReactOS и других крупных проектов.

Поясню своё утверждение. В подобных системах невозможно приблизиться к 100% покрытию кода при тестировании с помощью юнит-тестов или регрессионных тестов. Немного уточню. Можно конечно, но затраты на создание и поддержание таких тестов становятся недопустимо высоки.

Причина в том, что уж очень велико количество возможных состояний системы и возможных путей выполнения ветвей кода. Некоторые ветви крайне редко получают управление, но от этого не становятся ненужными. Именно здесь и получается увидеть преимущество, которым обладает статический анализ. Он проверяет весь код, в независимости от того, как часто он получает управление в процессе работы программы.

Пример проверки кода, редко получающего управление:
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 ошибок заполнения массивов константным значением.
#define MEMSET_BZERO(p,l) memset((p), 0, (l))

char *SHA384_End(SHA384_CTX* context, char buffer[]) {
  ...
  MEMSET_BZERO(context, sizeof(context));
  ...
}

Очищаем только первые байты массива, так как sizeof(context) возвращает размер указателя, а не структуры.
#define RtlFillMemory(Destination, Length, Fill) \
  memset(Destination, Fill, Length)

#define IOPM_FULL_SIZE          8196

HalpRestoreIopm(VOID)
{
  ...
  RtlFillMemory(HalpSavedIoMap, 0xFF, IOPM_FULL_SIZE);
  ...
}

Перепутаны аргументы при использовании макроса RtlFillMemory. Вызов должен быть таким:
RtlFillMemory(HalpSavedIoMap, IOPM_FULL_SIZE, 0xFF);

Снова о табуляциях и пробелах


Заранее хочу попросить не начинать в комментариях новую бурную дискуссию на эту тему. Я просто выскажу своё мнение. С ним можно быть согласным или нет. Но обсуждать не стоит.

Есть два непримиримых лагеря. Одни за использование табуляции в коде, так как это позволяет подстраивать отображение кода под себя [2]. Другие говорят, что это всё равно не работает и, что нет разумных причин использовать символы табуляции. От табуляции только вред и разъезжающееся форматирование. К их числу отношусь и я [3].

Можно сколько угодно говорить о том, что табуляции надо использовать правильно и тогда всё будет хорошо. К сожалению, это говорят те, кто работает замкнуто над одним проектом и не сталкиваются с внешним миром. В любом открытом или просто большом проекте, не удаётся достичь нормального оформления кода, если разрешить использовать табуляцию в любом виде.

Я не буду заниматься абстрактными рассуждениями. В этот раз я просто приведу своим оппонентам наглядный пример. Сейчас таким примером станет код ReactOS.

В стандарте кодирования ReactOS [4] написано хорошее правило с теоретической точки зрения:
Generic note about TABs usage: Don't use TABs for formatting; use TABs for indenting only and use only spaces for formatting.
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 и наблюдаю во многих местах испорченное форматирование. Почему?
Tabs vs Spaces 1

Tabs vs Spaces 2

Tabs vs Spaces 3
Ответ конечно очевиден. Потому, что это сложно помнить, где надо нажать TAB, а где поставить несколько пробелов, если это не единственный проект, с которым работаешь. Вот люди постоянно и ошибаются. А раз так, нечего быть теоретиками, а надо быть практиками. Надо взять и запретить использовать табуляцию вообще. И тогда всё у всех будет одинаково, а виновника, кто начинает использовать табуляцию, легко найти и сделать ему внушение.

Это не шаг назад в оформлении кода! Это шаг вперёд! Это следующий уровень осознания. Теоретическая красота настраиваемых отступов не сочетается с практикой. В первую очередь важно обеспечить однозначное представление кода и легкость разработки в большой команде. В компании Google это понимают. И в их стандарте для форматирования используются только пробелы [5]. Тем, кто ратует за использование табуляции, еще раз рекомендую задумываться, почему распределенная команда высококлассных профессионалов, которые разрабатывают Chromium, выбрала именно пробелы.

И еще раз. Теоретическая красота настраиваемых отступов не сочетается с практикой. Неважно, как красиво звучит теория, если она не работает. А именно так и обстоит дело в ReactOS.

Рекомендую команде разрабатывающей ReactOS модифицировать их стандарт и отказаться от использования табуляции. Любая табуляция должна расцениваться, как ошибка и быть устранена из кода.

Кстати, подобная практика позволит находить вот такие вот безобразия в коде ReactOS:
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. http://www.viva64.com/go.php?url=722
  2. Пора завязывать использовать пробелы вместо табуляции в коде. http://www.viva64.com/go.php?url=725
  3. Пора завязывать использовать символы табуляции в коде. http://www.viva64.com/go.php?url=726
  4. ReactOS. Coding Style. http://www.viva64.com/go.php?url=724
  5. Google C++ Style Guide. http://www.viva64.com/go.php?url=679
Читать дальше
Twitter
Одноклассники
Мой Мир

материал с habrahabr.ru

1

      Add

      You can create thematic collections and keep, for instance, all recipes in one place so you will never lose them.

      No images found
      Previous Next 0 / 0
      500
      • Advertisement
      • Animals
      • Architecture
      • Art
      • Auto
      • Aviation
      • Books
      • Cartoons
      • Celebrities
      • Children
      • Culture
      • Design
      • Economics
      • Education
      • Entertainment
      • Fashion
      • Fitness
      • Food
      • Gadgets
      • Games
      • Health
      • History
      • Hobby
      • Humor
      • Interior
      • Moto
      • Movies
      • Music
      • Nature
      • News
      • Photo
      • Pictures
      • Politics
      • Psychology
      • Science
      • Society
      • Sport
      • Technology
      • Travel
      • Video
      • Weapons
      • Web
      • Work
        Submit
        Valid formats are JPG, PNG, GIF.
        Not more than 5 Мb, please.
        30
        surfingbird.ru/site/
        RSS format guidelines
        500
        • Advertisement
        • Animals
        • Architecture
        • Art
        • Auto
        • Aviation
        • Books
        • Cartoons
        • Celebrities
        • Children
        • Culture
        • Design
        • Economics
        • Education
        • Entertainment
        • Fashion
        • Fitness
        • Food
        • Gadgets
        • Games
        • Health
        • History
        • Hobby
        • Humor
        • Interior
        • Moto
        • Movies
        • Music
        • Nature
        • News
        • Photo
        • Pictures
        • Politics
        • Psychology
        • Science
        • Society
        • Sport
        • Technology
        • Travel
        • Video
        • Weapons
        • Web
        • Work

          Submit

          Thank you! Wait for moderation.

          Тебе это не нравится?

          You can block the domain, tag, user or channel, and we'll stop recommend it to you. You can always unblock them in your settings.

          • AndreyKarpov2012
          • домен habrahabr.ru

          Get a link

          Спасибо, твоя жалоба принята.

          Log on to Surfingbird

          Recover
          Sign up

          or

          Welcome to Surfingbird.com!

          You'll find thousands of interesting pages, photos, and videos inside.
          Join!

          • Personal
            recommendations

          • Stash
            interesting and useful stuff

          • Anywhere,
            anytime

          Do we already know you? Login or restore the password.

          Close

          Add to collection

             

            Facebook

            Ваш профиль на рассмотрении, обновите страницу через несколько секунд

            Facebook

            К сожалению, вы не попадаете под условия акции