Сейчас я изучаю отчёт очередной проверки проекта Chromium и используемых в нём библиотек, с помощью анализатора кода PVS-Studio. По итогам проверки у меня намечается цикл статей, посвященный разбору некоторых видов ошибок, и тому, как их можно избежать. Однако, одна ошибка так сильно понравилась, что я решил не откладывать её описание, а сразу написать маленькую заметку в блог.
Наша команда написала уже 5 статей (1, 2, 3, 4, 5), посвященных поиску ошибок в открытом проекте Chromium. И, видимо, скоро я напишу ещё несколько заметок в наш блог на эту тему.
Сейчас я изучаю отчёт, выданный анализатором PVS-Studio и просто выписываю найденные ошибки. Написание заметок — это уже следующий этап. Я люблю в начале просмотреть отчёт, а потом уже решаю, как и о чём лучше написать. Однако, одна ошибка мне особенно понравилась, и я решил описать её сразу, не откладывая на потом.
Ошибка живёт в библиотеке Protocol Buffers (protobuf), используемой в проекте Chromium. Protocol Buffers — это протокол сериализации (передачи) структурированных данных, предложенный Google как эффективная бинарная альтернатива текстовому формату XML.
Если бы я увидел эту ошибку пару месяцев назад, я бы прошел мимо неё. Ну ошибка и ошибка. Однако, увидев её, я сразу вспомнил эпический сбой кассовых аппаратов, который недавно произошел в России. 20 декабря крупнейшие ритейлеры и сети АЗС по всей России столкнулись со сбоями в работе новых кассовых аппаратов. Первыми о проблеме узнали жители Владивостока, далее она распространилась по стране по мере начала рабочего дня, затронув Новосибирск, Барнаул, Красноярск, Кемерово и другие крупные города.
Ошибка в кассовых аппаратах и ошибка в Protocol Buffers это две разные ошибки, никак не связанные между собой. Но мне захотелось показать, как могут возникать подобные ошибки. Ведь часто дефекты кроятся не в хитрых алгоритмах, а являются следствием банальных опечаток. Я не знаю, что там было напутано в коде касс, но зато знаю, как глупая опечатка приводит к неработоспособности функции ValidateDateTime, предназначенной для проверки даты в Protocol Buffers. Давайте посмотрим на код этой функции и разберёмся, в нём.
static const int kDaysInMonth[13] = {
0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};
bool ValidateDateTime(const DateTime& time) {
if (time.year < 1 || time.year > 9999 ||
time.month < 1 || time.month > 12 ||
time.day < 1 || time.day > 31 ||
time.hour < 0 || time.hour > 23 ||
time.minute < 0 || time.minute > 59 ||
time.second < 0 || time.second > 59) {
return false;
}
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1;
} else {
return time.month <= kDaysInMonth[time.month];
}
}Функция ValidateDateTime принимает на вход дату и должна определить, корректна она или нет. В начале выполняются основные проверки. Проверяется, что номер месяца лежит в диапазоне [1..12], дни находятся в диапазоне [1..31], минуты находятся в диапазоне [0..59] и так далее. В коде всё это хорошо видно, и не требует специальных пояснений.
Далее идёт более сложная проверка, есть ли определённый день в определённом месяце. Например, декабрь состоит из 31 дня, а в ноябре 31-ого число нет, так как в месяце только 30 дней.
Чтобы проверить дни и при этом не писать множество операторов if или длинный switch, используется вспомогательный массив kDaysInMonth. В массиве записано количество дней в различных месяцах. По номеру месяца из массива извлекается максимальное количество дней и используется для проверки.
Дополнительно учитывается, что год может быть високосным, и тогда в феврале на 1 день больше.
В общем функция написана просто и красиво. Вот только неправильно.
Из-за опечатки дни проверяются неправильно. Обратите внимание, что с максимально возможным количеством дней в месяце, сравнивается не день, а месяц.
Ещё раз взгляните на код:
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1;
} else {
return time.month <= kDaysInMonth[time.month];
}В сравнениях "time.month <=" надо использовать не член структуры month, а член day. Т.е. корректный код должен выглядеть так:
if (time.month == 2 && IsLeapYear(time.year)) {
return time.day <= kDaysInMonth[time.month] + 1;
} else {
return time.day <= kDaysInMonth[time.month];
}Естественно номер месяца (от 1 до 12), всегда меньше количества дней в любом из месяцев.
Из-за этой ошибки корректными будут считаться такие дни, как, например, 31 февраля или 31 ноября.
Вот такая вот интересная ошибка встретилась. Из-за неё в обработку могут поступать некорректные даты, что в дальнейшем теоретически может использоваться для атаки системы. Тут я, пожалуй, сгущаю краски, но собственно так обычно и выглядят уязвимости. Просто где-то какие-то входные данные не проверены, и этим кто-то догадался воспользоваться.
Данная ошибка (вернее две ошибки) обнаруживается с помощью следующих предупреждений PVS-Studio:
- V547 / CWE-571 Expression 'time.month <= kDaysInMonth[time.month] + 1' is always true. time.cc 83
- V547 / CWE-571 Expression 'time.month <= kDaysInMonth[time.month]' is always true. time.cc 85
Как видите, теперь PVS-Studio также сразу классифицирует предупреждения согласно Common Weakness Enumeration (CWE).
Ещё хочу обратить внимание, что анализатор PVS-Studio всё более глубоко учится анализировать код. Сама по себе диагностика V547 старая (была реализована в 2010 году). Однако, скажем, год назад анализатор не смог бы найти эту ошибку. Теперь анализатор умеет заглянуть внутрь массива и понять, что извлекаются значения в диапазоне [28..31]. При этом он понимает, что 0 в массиве учитывать не надо, так как возможный диапазон time.month равен [1..12]. Если бы номер месяца был, скажем, равен 100, то произошел выход из функции и анализатор это понимает.
В итоге, анализатор видит, что происходят следующие сравнения диапазонов:
- [2..2] <= [28..31]
- [1..12] <= [29..32]
Следовательно, условия всегда истинны, о чём анализатор и выдаёт предупреждение. Вот такой вот глубокий анализ. Как видите, мы не только добавляем новые предупреждения в PVS-Studio, но и дорабатываем Data-Flow анализ, что сказывается на качестве уже существующих диагностик.
Почему первый диапазон [2, 2] представлен только одним числом 2? Дело в том, что учитывается уточняющее условие time.month == 2.
Следует задаться вопросом: как можно улучшить стиль, чтобы защититься от подобных ошибок?
Не знаю. Функция простая и написана в хорошем стиле. Просто человеку свойственно ошибаться, и он может допускать подобные опечатки. Ни один профессионал не застрахован от таких ошибок.
Можно только порекомендовать писать более внимательно юнит-тесты и использовать профессиональные статические анализаторы кода, такие как PVS-Studio.
Спасибо за внимание. Пойду дальше изучать отчёт.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. February 31.
