Приглашаем попробовать найти ошибку в очень простой функции из проекта 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 ('') 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').
Возвращаемое значение. Ненулевое значение, если символ является пробельным, ноль иначе.
Функция EatWhitespace должна пропустить все символы, считающиеся пробельными, кроме перевода строки 'n'. Ещё одной причиной остановки чтения из файла может стать достижение конца файла (EOF).
А теперь, зная всё это, попробуйте найти ошибку!
Чтобы читателю случайно сразу не взглянуть на ответ, добавим парочку ждущих единорогов.
Рисунок 1. Время поискать ошибку. Единороги пока подождут.
Всё равно не видите ошибку?
Всё дело в том, что мы обманули читателей по поводу isspace. Ха-ха! Это вовсе не стандартная функция, а самодельный макрос. Да, мы — бяки и заставили вас запутаться.
Рисунок 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, выдавая предупреждение:
V560 A part of conditional expression is always true: ('n' != c). params.c 136.
Для полной ясности давайте разберём 3 варианта развития событий:
- Достигнут конец файла. Конец файла (EOF) — это не пробел и не табуляция. Подвыражение ('n' != c) из-за short-circuit evaluation не вычисляется. Цикл останавливается.
- Прочитан любой символ, не являющийся пробелом или табуляцией. Подвыражение ('n' != c) не вычисляется из-за short-circuit evaluation. Цикл останавливается.
- Прочитан символ пробела или горизонтальная табуляции. Подвыражение ('n' != c) вычисляется, но его результат всегда будет истинным.
Другими словами, рассмотренный код эквивалентен этому:
for (c = getc (InFile); c==' ' || c == 't'; c = getc (InFile))Мы выяснили, что код работает не так, как было задумано. Давайте теперь разберёмся, какие это имеет последствия.
Программист, написавший в теле функции EatWhitespace вызов isspace, рассчитывал, что будет вызвана стандартная функция. Именно поэтому он добавил условие, что перевод строки LF ('n') не следует считать пробельным символом.
Следовательно, программист планировал, что помимо пробела и горизонтальной табуляции будут пропущены такие символы, как смена страницы и вертикальный таб.
Примечательнее, что планировалось пропускать и символ возврат каретки CR (0x0d, 'r'). Этого не происходит и цикл остановится, встретив этот символ. Это приведёт к неприятным неожиданностям, если разделителем строк в файле является последовательность CR+LF, используемая в некоторых не-UNIX системах, таких как Microsoft Windows.
Для тех, кто хочет узнать больше об исторических причинах использования в качестве разделителей строк LF или CR+LF, предлагаем вниманию статью на Wikipedia "Перевод строки".
Функция EatWhitespace по задумке должна одинаково обрабатывать файлы, где в качестве разделителя используется как LF, так и CR+LF. Для случая CR+LF это не так. Другими словами, если ваш файл прибыл из мира Windows, то вам не повезло :).
Возможно, это и не является серьёзной ошибкой, тем более, что GNU Midnight Commander распространён в UNIX-подобных операционных системах, где для перевода строки используется символ LF (0x0a, 'n'). Однако из-за таких мелочей и возникают различные досадные проблемы несовместимости данных, подготавливаемых в Linux и Windows системах.
Описанная ошибка интересна тем, что её почти невозможно обнаружить при классическом обзоре кода. Не все разработчики проекта могут знать про тонкости макроса, да и забыть их очень легко. Это хороший пример, когда статический анализ кода дополняет обзоры кода и другие методики поиска ошибок.
Переопределение стандартных функций является плохой практикой. Кстати, недавно в статье "Любите статический анализ кода" рассматривался похожий случай с макросом #define sprintf std::printf.
Более правильным решением было бы дать макросу уникальное имя, например, is_space_or_tab. Тогда путаница была бы невозможна.
Возможно, причиной создания макроса была медленная работа стандартной функции isspace и программист создал её более быстрый вариант, достаточный для решения всех необходимых задач. Но всё равно такое решение неправильное. Надёжнее было бы определить isspace так, чтобы получался некомпилируемый код. А нужную функциональность реализовать в макросе с уникальным именем.
Спасибо за внимание. Приглашаем скачать и попробовать анализатор PVS-Studio для проверки своих проектов. Плюс напоминаем, что недавно в анализаторе появилась поддержка языка Java.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Wanna Play a Detective? Find the Bug in a Function from Midnight Commander.
Автор: Andrey2008
