Хабрахабр

Для тех, кто хочет поиграть в детектива: найди ошибку в функции из Midnight Commander

Найди ошибку!

Приглашаем попробовать найти ошибку в очень простой функции из проекта GNU Midnight Commander. Зачем? Просто так. Это забавно и интересно. Хотя нет, мы соврали. Мы в очередной раз хотим продемонстрировать ошибку, которую с трудом находит человек в процессе code review, но легко находит статический анализатор кода PVS-Studio.
Недавно нам прислали письмо, где спрашивали, почему анализатор выдаёт предупреждение на функцию EatWhitespace, код которой приведён ниже. На самом деле вопрос не так уж и прост. Попробуйте самостоятельно догадаться, что не так с этим кодом.

static int
EatWhitespace (FILE * InFile) /* ----------------------------------------------------------------------- ** * Scan past whitespace (see ctype(3C)) and return the first non-whitespace * character, or newline, or EOF. * * Input: InFile - Input source. * * Output: The next non-whitespace character in the input stream. * * Notes: Because the config files use a line-oriented grammar, we * explicitly exclude the newline character from the list of * whitespace characters. * - Note that both EOF (-1) and the nul character ('\0') are * considered end-of-file markers. * * ----------------------------------------------------------------------- ** */
{ int c; for (c = getc (InFile); isspace (c) && ('\n' != c); c = getc (InFile)) ; return (c);
} /* EatWhitespace */

Как видите, функция EatWhitespace совсем маленькая. Даже комментарий к функции занимает больше места, чем тело самой функции :). Теперь немного деталей.

Описание функции getc:

int getc ( FILE * stream );

Функция возвращает символ, на который указывает внутренний индикатор позиции файла указанного потока. Затем индикатор переходит к следующему символу. Если на момент вызова потока достигнут конец файла, функция возвращает значение EOF и устанавливает индикатор конца файла для данного потока. Если возникает ошибка чтения, функция возвращает значение EOF и устанавливает индикатор ошибки для данного потока (ferror).

Описание функции isspace:

int isspace( int ch );

Функция проверяет, является ли символ пробельным по классификации текущей локали. В стандартной локали следующие символы являются пробельными:

  • пробел (0x20, ' ');
  • смена страницы (0x0c, '\f');
  • перевод строки LF (0x0a, '\n');
  • возврат каретки CR (0x0d, '\r');
  • горизонтальный таб (0x09, '\t');
  • вертикальный таб (0x0b, '\v').

Возвращаемое значение. Ненулевое значение, если символ является пробельным, ноль иначе.

Ещё одной причиной остановки чтения из файла может стать достижение конца файла (EOF). Функция EatWhitespace должна пропустить все символы, считающиеся пробельными, кроме перевода строки '\n'.

А теперь, зная всё это, попробуйте найти ошибку!

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

Время поискать ошибку. Рисунок 1.  Единороги пока подождут.

Рисунок 1. Время поискать ошибку. Единороги пока подождут.

Всё равно не видите ошибку?

Ха-ха! Всё дело в том, что мы обманули читателей по поводу isspace. Да, мы — бяки и заставили вас запутаться. Это вовсе не стандартная функция, а самодельный макрос.

Единорог дарит читателям ложное представление о том, что такое isspace."/> <img src="http://orion-int.ru/wp-content/uploads/2019/02/efbbbfdlya-tex-kto-xochet-poigrat-v-detektiva-najdi-oshibku-v-funkcii-iz-midnight-commander-2.png" alt="Рисунок 2.

Рисунок 2. Единорог дарит читателям ложное представление о том, что такое isspace.

Путаницу внесли авторы проекта GNU Midnight Commander, решив создать собственную реализацию isspace в файле charset.h: На самом деле виноваты, конечно, не мы и не наш единорог.

#ifdef isspace
#undef isspace
#endif
....
#define isspace(c) ((c)==' ' || (c) == '\t')

Создав такой макрос, одни разработчики запутали других разработчиков. Код написан из предположения, что isspace — это стандартная функция, считающая возврат каретки (0x0d, '\r') одним из пробельных символов.

Давайте подставим макрос и посмотрим, что получится. Реализованный же макрос считает пробельными символами только пробелы и табуляции.

for (c = getc (InFile); ((c)==' ' || (c) == '\t') && ('\n' != c); c = getc (InFile))

Подвыражение ('\n' != c) является лишним (избыточным), так как его результатом всегда будет true. Об этом и предупреждает анализатор PVS-Studio, выдавая предупреждение:

params.c 136. V560 A part of conditional expression is always true: ('\n' != c).

Для полной ясности давайте разберём 3 варианта развития событий:

  • Достигнут конец файла. Конец файла (EOF) — это не пробел и не табуляция. Подвыражение ('\n' != c) из-за short-circuit evaluation не вычисляется. Цикл останавливается.
  • Прочитан любой символ, не являющийся пробелом или табуляцией. Подвыражение ('\n' != c) не вычисляется из-за short-circuit evaluation. Цикл останавливается.
  • Прочитан символ пробела или горизонтальная табуляции. Подвыражение ('\n' != c) вычисляется, но его результат всегда будет истинным.

Другими словами, рассмотренный код эквивалентен этому:

for (c = getc (InFile); c==' ' || c == '\t'; c = getc (InFile))

Мы выяснили, что код работает не так, как было задумано. Давайте теперь разберёмся, какие это имеет последствия.

Именно поэтому он добавил условие, что перевод строки LF ('\n') не следует считать пробельным символом. Программист, написавший в теле функции EatWhitespace вызов isspace, рассчитывал, что будет вызвана стандартная функция.

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

Этого не происходит и цикл остановится, встретив этот символ. Примечательнее, что планировалось пропускать и символ возврат каретки CR (0x0d, '\r'). Это приведёт к неприятным неожиданностям, если разделителем строк в файле является последовательность CR+LF, используемая в некоторых не-UNIX системах, таких как Microsoft Windows.

Для тех, кто хочет узнать больше об исторических причинах использования в качестве разделителей строк LF или CR+LF, предлагаем вниманию статью на Wikipedia "Перевод строки".

Для случая CR+LF это не так. Функция EatWhitespace по задумке должна одинаково обрабатывать файлы, где в качестве разделителя используется как LF, так и CR+LF. Другими словами, если ваш файл прибыл из мира Windows, то вам не повезло :).

Однако из-за таких мелочей и возникают различные досадные проблемы несовместимости данных, подготавливаемых в Linux и Windows системах. Возможно, это и не является серьёзной ошибкой, тем более, что GNU Midnight Commander распространён в UNIX-подобных операционных системах, где для перевода строки используется символ LF (0x0a, '\n').

Не все разработчики проекта могут знать про тонкости макроса, да и забыть их очень легко. Описанная ошибка интересна тем, что её почти невозможно обнаружить при классическом обзоре кода. Это хороший пример, когда статический анализ кода дополняет обзоры кода и другие методики поиска ошибок.

Кстати, недавно в статье "Любите статический анализ кода" рассматривался похожий случай с макросом #define sprintf std::printf. Переопределение стандартных функций является плохой практикой.

Тогда путаница была бы невозможна. Более правильным решением было бы дать макросу уникальное имя, например, is_space_or_tab.

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

Приглашаем скачать и попробовать анализатор PVS-Studio для проверки своих проектов. Спасибо за внимание. Плюс напоминаем, что недавно в анализаторе появилась поддержка языка Java.

Wanna Play a Detective? Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Find the Bug in a Function from Midnight Commander.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть