Это уже четвертая заметка, где я хочу поделиться полезными наблюдениями о паттернах ошибок и том, как можно с ними бороться. В этот раз я затрону такую тему, как обработка редких и аварийных ситуаций в программах. Рассматривая множество программ, я пришел к выводу, что код обработки ошибок в Си/Си++ программах - одно из самых ненадежных мест.
К чему приводят такие дефекты? Программа вместо того, чтобы выдать сообщение "файл X не найден", падает и заставляет пользователя гадать, что он не так делает. Программа для работы с базой данных выводит невразумительное сообщение вместо того, чтобы сообщить, что неверно заполнено одно из полей. Попробуем сразиться с этой разновидностью ошибок, которые досаждают нашим пользователям.
Вначале информация для читателей, которые не знакомы с моими предыдущими заметками. Их можно найти здесь:
Как всегда, я буду не абстрактен, а начну с примеров. В этот раз примеры будут взяты из исходного кода Firefox. Я постараюсь продемонстрировать, что даже в качественном и известном приложении с кодом для обработки ошибок, всё обстоит не самым лучшим образом. Дефекты были найдены мной с помощью анализатора PVS-Studio 4.50.
Пример N1. Неполноценная проверка целостности таблицы
int AffixMgr::parse_convtable(..., const char * keyword)
{
...
if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
HUNSPELL_WARNING(stderr,
"error: line %d: table is corrupt\n",
af->getlinenum());
delete *rl;
*rl = NULL;
return 1;
}
...
}Диагностика PVS-Studio: V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. affixmgr.cpp 3708
Здесь сделана попытка проверить целостность таблицы. К сожалению, эта проверка может сработать, а может и не сработать. Для вычисления длины ключевого слова используют оператор sizeof(), что естественно некорректно. В результате, работоспособность кода зависит от счастливого стечения обстоятельств (длины ключевого слова, размера указателя 'keyword' в текущей модели данных).
Пример 2. Неработающая проверка при чтении файла
int PatchFile::LoadSourceFile(FILE* ofile)
{
...
size_t c = fread(rb, 1, r, ofile);
if (c < 0) {
LOG(("LoadSourceFile: "
"error reading destination file: " LOG_S "\n",
mFile));
return READ_ERROR;
}
...
}Диагностика PVS-Studio: V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 1179
Это пример, когда код обработки ошибки пишется по принципу "чтобы было". Программист даже не задумался, что собственно он написал, и как это будет работать. Проверка некорректна. Функция fread() возвращает количество прочитанных байт с помощью беззнакового типа. Прототип функции:
size_t fread(
void *buffer,
size_t size,
size_t count,
FILE *stream
);Естественно, для хранения результата используется переменная 'c', имеющая тип size_t. Как следствие, результат проверки (c < 0) всегда ложен.
Это хороший пример. На первый взгляд кажется, что есть какая-то проверка, но на практике выясняется, что она совершенно бесполезна.
Аналогичную ошибку можно увидеть и в других местах:
V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 2373
V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. bspatch.cpp 107
Пример 3. Проверка указателя на NULL уже после его использования
nsresult
nsFrameSelection::MoveCaret(...)
{
...
mShell->FlushPendingNotifications(Flush_Layout);
if (!mShell) {
return NS_OK;
}
...
}Диагностика PVS-Studio: V595 The 'mShell' pointer was utilized before it was verified against nullptr. Check lines: 1107, 1109. nsselection.cpp 1107
Если указатель равен нулю, то мы должны обработать этот специальный случай и вернуть из функции NS_OK. Смущает то, что указатель mShell до этого момента уже используется.
Скорее всего, этот код работает, так как указатель mShell всегда неравен NULL. Пример я привожу, чтобы показать, что можно допустить ошибку даже в очень простых проверках. Проверка есть, а смысла от неё нет.
Пример 4. Проверка указателя на NULL уже после его использования
CompileStatus
mjit::Compiler::performCompilation(JITScript **jitp)
{
...
JaegerSpew(JSpew_Scripts,
"successfully compiled (code \"%p\") (size \"%u\")\n",
(*jitp)->code.m_code.executableAddress(),
unsigned((*jitp)->code.m_size));
if (!*jitp)
return Compile_Abort;
...
}Диагностика PVS-Studio:V595 The '* jitp' pointer was utilized before it was verified against nullptr. Check lines: 547, 549. compiler.cpp 547
Кстати, использование указателя до проверки - распространенная ошибка. Это ещё один пример на эту тему.
Пример 5. Неполная проверка входных значений
PRBool
nsStyleAnimation::AddWeighted(...)
{
...
if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
unit[0] == eCSSUnit_Null || unit[0] == eCSSUnit_URL) {
return PR_FALSE;
}
...
}Диагностика PVS-Studio: V501 There are identical sub-expressions 'unit [0] == eCSSUnit_Null' to the left and to the right of the '||' operator. nsstyleanimation.cpp 1767
Кажется, здесь сразу 2 опечатки. Затрудняюсь сказать, как именно должен выглядеть здесь код, но, наверное, хотели написать так:
if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
unit[0] == eCSSUnit_URL || unit[1] == eCSSUnit_URL) {Из-за опечаток функция может начать обрабатывать некорректные входные значения.
Пример 6. Неполная проверка входных значений
nsresult PresShell::SetResolution(float aXResolution, float
aYResolution)
{
if (!(aXResolution > 0.0 && aXResolution > 0.0)) {
return NS_ERROR_ILLEGAL_VALUE;
}
...
}Диагностика PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: aXResolution > 0.0 && aXResolution > 0.0 nspresshell.cpp 5114
А вот ещё один пример неудачной проверки входных параметров. В этот раз из-за опечатки не проверяется значение аргумента aYResolution.
Пример 7. Неразыменованный указатель
nsresult
SVGNumberList::SetValueFromString(const nsAString& aValue)
{
...
const char *token = str.get();
if (token == '\0') {
return NS_ERROR_DOM_SYNTAX_ERR; // nothing between commas
}
...
}Диагностика PVS-Studio: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *token == '\0'. svgnumberlist.cpp 96
Проверка, что между запятыми ничего нет, не работает. Чтобы узнать, пустая строка или нет, можно сравнить первый символ с '\0'. Но здесь с нулем сравнивается не первый символ, а указатель. Этот указатель всегда неравен нулю. Корректная проверка должна была выглядеть так: (*token == '\0').
Пример 8. Неподходящий тип для хранения индекса
PRBool
nsIEProfileMigrator::TestForIE7()
{
...
PRUint32 index = ieVersion.FindChar('.', 0);
if (index < 0)
return PR_FALSE;
...
}Диагностика PVS-Studio: V547 Expression 'index < 0' is always false. Unsigned type value is never < 0. nsieprofilemigrator.cpp 622
Функция не вернёт PR_FALSE, если в строке нет точки и продолжит работать с некорректными данными. Ошибка в том, что для переменной 'index' выбран беззнаковый тип данных. Проверка (index < 0) не имеет смысла.
Пример 9. Формирование неправильного сообщения об ошибке
cairo_status_t
_cairo_win32_print_gdi_error (const char *context)
{
...
fwprintf(stderr, L"%s: %S", context, (wchar_t *)lpMsgBuf);
...
}Диагностика PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'fwprintf' function. The pointer to string of wchar_t type symbols is expected. cairo-win32-surface.c 129
Даже если ошибка успешно обнаружена, её еще надо суметь правильно обработать. А поскольку обработчики ошибок тоже никто не тестирует, то там можно увидеть много интересного.
Функция _cairo_win32_print_gdi_error() распечатает абракадабру. В качестве третьего аргумента функция fwprintf() ожидает указатель на unicode-строку, а вместо этого получает строку в формате 'const char *'.
Пример 10. Ошибка записи дампа
bool ExceptionHandler::WriteMinidumpForChild(...)
{
...
DWORD last_suspend_cnt = -1;
...
// this thread may have died already, so not opening
// the handle is a non-fatal error
if (NULL != child_thread_handle) {
if (0 <= (last_suspend_cnt =
SuspendThread(child_thread_handle))) {
...
}Диагностика PVS-Studio: V547 Expression is always true. Unsigned type value is always >= 0. exception_handler.cc 846
Это другой пример в обработчике ошибок. Здесь некорректно обрабатывается результат, возвращаемый функцией SuspendThread. Переменная last_suspend_cnt имеет тип DWORD, а значит она всегда будет больше или равна 0.
Сделаю небольшое отступление и расскажу о результатах проверки Firefox в целом. Проект качественен, и PVS-Studio выявил мало ошибок. Однако, так как он большой, то в количественном отношении ошибок достаточно много. К сожалению, мне не удалось полноценно изучить отчет, выданный инструментом PVS-Studio. Дело в том, что для Firefox отсутствует файл проекта для Visual Studio. Проект проверялся консольной версией PVS-Studio, вызываемой из make-файла. Открыв отчет в Visual Studio, можно просмотреть все диагностические сообщения. Но раз нет проекта, то Visual Studio не подсказывает, где какие переменные объявлены, не позволяет перейти в место определения макросов и так далее. В результате, анализ неизвестного проекта крайне трудоемок, и я смог изучить только часть сообщений.
Ошибки встречаются разноплановые. Например, есть выход за границы массива:
class nsBaseStatis : public nsStatis {
public:
...
PRUint32 mLWordLen[10];
...
nsBaseStatis::nsBaseStatis(...)
{
...
for(PRUint32 i = 0; i < 20; i++)
mLWordLen[i] = 0;
...
}
...
};Диагностика PVS-Studio: V557 Array overrun is possible. The value of 'i' index could reach 19. detectcharset.cpp 89
Хотя эта и подобные ошибки интересны, они не связаны с темой данной статьи. Поэтому, если интересно, можно посмотреть на некоторые другие ошибки в этом файле: mozilla-test.txt.
Я решил привести не парочку, а 10 примеров, чтобы убедить вас в актуальности проблем наличия дефектов в обработчиках ошибок. Конечно, обработчики ошибок не самые критичные и важные участки программы. Но ведь программисты их пишут, а значит, надеются с их помощью улучшить поведение программы. К сожалению, как показывают мои наблюдения, очень часто проверки и обработчики ошибок не работают. Смотрите, мне было достаточно одного, пусть и крупного проекта, чтобы показать множество ошибок данного типа.
Что же с этим делать и какие можно дать рекомендации?
Нужно признать, что можно сделать ошибку даже в простой проверке. Это самое сложное и важное. Именно из-за того, что обработчики ошибок считаются простыми фрагментами кода, в них так много опечаток и иных дефектов. Обработчики ошибок не проверяют и не тестируют. На них не пишут тесты.
Конечно, писать тесты на обработчики ошибок очень сложно и часто экономически нецелесообразно. Но если программист хотя бы будет знать об опасности, это уже многое. Осведомлен, значит вооружен. С обработчиками ошибок можно привести и вот такую аналогию.
По статистике альпинисты чаще всего падают в конце подъема. Это случается не из-за усталости, а из-за того, что человек думает, что ему осталось совсем немного. Он расслабляется, теряет внимательность и, как результат, чаще допускает ошибки. С программистом при написании кода происходит что-то похожее. Он много сил и внимания тратит на алгоритм, а различные проверки пишет не сосредотачиваясь, так как уверен, что в них он не может допустить ошибку.
Итак, теперь вы предупреждены. И я уверен, это уже очень хорошо и полезно.
Если вы скажите, что подобные глупые ошибки допускают только студенты и неопытные программисты, то вы не правы. Опечатки легко делают все. Предлагаю на эту тему вот эту небольшую заметку "Миф второй - профессиональные разработчики не допускают глупых ошибок". Я могу подтвердить это множеством примеров из различных проектов. Но думаю, приведенных здесь вполне достаточно, чтобы задуматься.
Механизмы сохранения дампов, функции записи логов и другие подобные вспомогательные механизмы вполне заслуживают того, чтобы для них были сделаны юнит-тесты.
Неработающий механизм сохранения дампа не только бесполезен, он только создает видимость, что в случае беды им можно будет воспользоваться. Если пользователь пришлёт испорченный dump-файл, то он не только не поможет, а может только ввести в заблуждение и на поиски ошибок будет потрачено больше времени, чем если бы dump-файла вообще отсутствовал.
Рекомендация выглядит простой и очевидной. Но у многих ли, из читающих эту заметку, есть юнит-тесты для проверки класса WriteMyDump ?
Используйте статические анализаторы кода. Возможность нахождения дефектов в обработчиках ошибок является одной из сильных сторон методологии статического анализа. Статический анализ покрывает все ветви кода, в не зависимости от частоты их использования в работающем приложении. Он может выявить ошибки, проявляющие себя крайне редко.
Другими словами, при статическом анализе покрытие кода составляет 100%. Достичь такого покрытия кода с помощью других видов тестирования практически нереально. Покрытие кода при юнит-тестах и регрессионном тестировании обычно составляет менее 80%. Оставшиеся 20% протестировать очень сложно. В эти 20% входят большинство обработчиков ошибок и редких ситуаций.
Можно попробовать использовать методологию внесения неисправностей. Смысл в том, что ряд функций время от времени начинают возвращать различные коды ошибок, и программа должна корректно их обрабатывать. Например, можно написать свою функцию malloc(), которая время от времени будет возвращать NULL, даже если память ещё есть. Это позволит узнать, как будет вести себя программа, когда память действительно кончится. Аналогично можно поступать с такими функциями, как fopen(), CoCreateInstance(), CreateDC() и так далее.
Существуют специальные программы, которые позволяют автоматизировать этот процесс и не писать самостоятельно свои функции, приводящие временами к отказу. К сожалению, я не работал с подобными системами, поэтому затрудняюсь рассказать про них подробнее.
Дефекты в обработчиках ошибок встречаются часто. К сожалению, я не уверен, что приведенные в статье рекомендации достаточны, чтобы их избежать. Но я надеюсь, что вас заинтересовала эта проблема, и вы сможете придумать способы сократить количество недочетов в своих программах. Также я и другие читатели будут признательны, если вы поделитесь своими идеями и методиками, которые позволят избежать ошибки рассмотренного вида.