Главное предназначение статических анализаторов – найти те ошибки, которые остались незамеченными разработчиком. И недавно команда PVS-Studio снова столкнулась с интересным примером мощи этой методики.
Работа с инструментами статического анализа кода требует внимательности. Часто код, на который указал анализатор, кажется корректным. В таких случаях хочется посчитать предупреждение ложным срабатыванием. На днях мы сами попали в такую ловушку. Вот как это получилось.
Недавно мы доработали ядро анализатора, и коллега, который просматривал новые срабатывания, обнаружил среди них ложное. Он выписал срабатывание для тимлида, тот бегло просмотрел код и создал задачу. Задачу взял я. Таким нехитрым образом и образовалась цепочка из трёх программистов.
Предупреждение анализатора: V645 The 'strncat' function call could lead to the 'а.consoleText' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold.
Фрагмент кода:
struct A
{
char consoleText[512];
};
void foo(A a)
{
char inputBuffer[1024];
....
strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) –
strlen(a.consoleText) - 5);
....
}
Перед изучением примера давайте вспомним, что делает функция strncat:
char *strncat(
char *strDest,
const char *strSource,
size_t count
);
где:
'destination' — строка-получатель;
'source' — строка-источник;
'count' — максимальное число символов, которое можно добавить.
На первый взгляд, кажется, что с кодом всё отлично. В нём вычисляется количество пустого места в буфере. При этом там даже запас в 4 байта вроде как есть... И именно из-за этого ощущения, что с кодом всё хорошо, он и был выписан как пример, где анализатор выдаёт ложное предупреждение.
Давайте разберёмся, как обстоят дела на самом деле. В выражении:
sizeof(a.consoleText) – strlen(a.consoleText) – 5
максимальное значение может быть достигнуто при минимальном значении второго операнда:
strlen(a.consoleText) = 0
Тогда результатом будет 507, и никакого переполнения не случится. О чём же тогда говорит PVS-Studio? Давайте немного углубимся во внутренние механизмы анализатора и попробуем разобраться.
В статическом анализаторе за подсчёт таких выражений отвечает механизм анализа потока данных (data-flow). В большинстве случаев, когда выражение состоит из констант времени компиляции, data-flow вернёт строгое значение выражения. В остальных случаях, как и в случае с предупреждением, он сформирует только диапазон возможных значений выражения.
В данном случае значение операнда strlen(a.consoleText) неизвестно нам на этапе компиляции. Давайте посмотрим диапазон.
Спустя несколько минут отладки мы получаем от data-flow аж 2 диапазона:
[0, 507] U [0xFFFFFFFFFFFFFFFC, 0xFFFFFFFFFFFFFFFF]
На первый взгляд, кажется, что второй диапазон лишний. Однако это не так. Просто мы забыли о том, что результатом выражения может стать отрицательное число. Например, такое может случиться при strlen(a.consoleText) = 508. В таком случае произойдет переполнение беззнакового числа, и результатом выражения будет максимальное значение результирующего типа, в данном случае — size_t.
Получается, что анализатор прав! В данном выражении к полю consoleText может добавиться гораздо большее количество символов, чем его размер, что приведёт к переполнению буфера и, как следствие, неопределённому поведению. Причина неожиданного предупреждения – то, что ложного срабатывания тут нет!
Вот так мы снова получили подтверждение, что ключевое преимущество статического анализа над человеком – его внимательность. Вдумчивое изучение предупреждений анализатора помогает разработчику сэкономить время при отладке и нервы, уберегает от ошибок и скоропалительных выводов.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. One Day in the Life of PVS-Studio Developer, or How I Debugged Diagnostic That Surpassed Three Programmers.
Комментарии (33)
Punk_Joker
05.07.2021 16:50+3Сугубо для меня, из статьи очень сложно понять в чем на самом деле ошибка. Сначала сам разобрался, а потом пришлось потратиь время, что бы понять что именно это имеется ввиду в статье. Но за статью в целом плюс.
Tujh
05.07.2021 17:13Тут ещё и логическая ошибка, программист слишком перемудрил
strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) – strlen(a.consoleText) - 5);в третьей позиции должно быть:
Maximum number of characters to be appended.
так что sizeof(a.consoleText) было бы более чем достаточно.
Правда это не замечание для анализатора, а для кода в целом.
datacompboy
05.07.2021 17:17+2В случае просто sizeof() туда можно будет скопировать больше чем влезет...
Логику этого поведения мне не совсем понять (это именно сколько добавить, а не сколько всего места есть в буфере...) но таки правильно записано. ну, почти.
Tujh
05.07.2021 18:13+1был неправ
datacompboy
05.07.2021 18:15https://www.cplusplus.com/reference/cstring/strncat/
/* strncat example */ #include <stdio.h> #include <string.h> int main () { char str1[20]; char str2[20]; strcpy (str1,"To be "); strcpy (str2,"or not to be"); strncat (str1, str2, 6); puts (str1); return 0; }Output: "To be or not"
Tujh
05.07.2021 18:20лучше смотреть man
A simple implementation of strncat() might be: char* strncat(char *dest, const char *src, size_t n) { size_t dest_len = strlen(dest); size_t i; for (i = 0 ; i < n && src[i] != '\0' ; i++) dest[dest_len + i] = src[i]; dest[dest_len + i] = '\0'; return dest; }вот, что бывает, когда долго работаешь со фреймворками, забываешь стандартное поведение.
Racheengel
05.07.2021 17:54-1Я так понимаю, что a.consoleText не обязательно zero-terminated может быть, к тому же.
И чисто теоретически, strlen(a.consoleText) может вернуть любой мусор в этом случае.datacompboy
05.07.2021 18:17Почему? strncat делает, цитирую, "Appends the first num characters of source to destination, plus a terminating null-character.". Именно из-за "plus a terminating null-character" нам и надо вычитать в третьем аргументе 1цу
rat1
05.07.2021 20:42+1Человек написал о том, что строка может быть не C-строкой, а просто массивом байт без завершения. Кто знает, что туда записали до этого?!
datacompboy
05.07.2021 20:53Это вопрос контракта. Использование на массиве str* функций подразумевает что данные тут предполагают 0-терминацию.
rat1
06.07.2021 00:14Судя по всему не заполнять массив настолько, чтобы целочисленного переполнения не произошло - это тоже вопрос контракта? Зачем нам статический анализатор? Работаем на доверии и без ошибок.
datacompboy
06.07.2021 02:32Вот как раз следить за тем что контракт сохранится до и после каждой из функций и нужен статан.
aamonster
05.07.2021 19:06А не хотите выделить подобные случаи в отдельную диагностику? "buffer overflow" тут выглядит не очень понятным, хотелось бы видеть что-то типа "третий аргумент не должен быть отрицательным" (если он отрицательный – это уже ошибка, и не надо даже смотреть, переполняется ли буфер).
ciubotaru
05.07.2021 19:46+3Так он не будет отрицательным. Там беззнаковое целое. Его переполнение (выход за верхнюю/нижнюю границу допустимых значений) это "законная" операция в Си.
aamonster
05.07.2021 21:09Да, действительно. Но всё равно, хоть формально и законное, по смыслу – нет. Можно диагностировать :-)
valeramikhaylovsky
05.07.2021 21:03+5Вроде статья про С++, но код на СИ. Почему не использоватьstd::string_view и std::string и STL? Сишный код это почти всегда источник багов. Современный С++ даёт возможно писать надёжный код, но этой возможностью не пользуются, даже разработчики статического анализатора для С++.
kunitsyn
06.07.2021 01:16+1Здесь суть в самом предупреждении, в том, что оно не напоминает про underflow. Если оно даже запутало разработчика PVS-Studio, то что уж делать простым смертным.
Возможно, можно как-то изменить предупреждение, чтобы оно напоминало про underflow в этом конкретном примере, но это будет очень сложно, и я даже не представляю, как это может работать с точки зрения пользователя.
valeramikhaylovsky
06.07.2021 09:53Хотелось бы видеть достойные внимания примеры кода, а не устаревшие решения. В работе постоянно сталкиваюсь с тем, что новобранцы пишут код самым непотребным образом, потому, что в инете примеры именно такие, вот пример:
Надо подсчитать количество подстрок в строке на С++.
Решение, которое пишут в 80% случаев:size_t substr_count(const char *str, const char* substr) { size_t count = 0; const char *p = str; do { p = strstr(p, substr); } while (p != nullptr && ++count && *(++p) != 0); return count; }
Тут С++ -ом даже и не пахнет, зато арифметика на сырых указателях, куча условий в цикле. И это ещё один из лучших примеров, который приходилось видеть.
А теперь как сделать тоже самое на С++:std::size_t substr_count(std::string_view text, std::string_view sub) noexcept { std::size_t t_count {0}; for (std::size_t pos {0}; (pos = text.find(sub, pos)) != text.npos; ++t_count, pos += sub.length()); return t_count; }
Даже тело цикла не требуется, и вероятность получить UB резко ниже.
mk2
06.07.2021 10:00+4Количество вхождений подстроки в строку, в смысле? И видимо непересекающихся вхождений.
То, что небольшое тело цикла упихано в for — по-моему скорее антипаттерн, не слишком удобно читается.
eao197
06.07.2021 10:13+1А как должен быть исправлен проблемный фрагмент чтобы PVS-Studio перестал диагностировать ошибку? Перед вызовом strncat нужно сделать проверку длины строки? Типа:
if(strlen(a.consoleText)+5u < sizeof(a.consoleText)) { strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) – strlen(a.consoleText) - 5); }valeramikhaylovsky
06.07.2021 13:38-1Наверное, код должен был бы выглядеть иначе, например написать функцию слияния строк, что-то вроде:
std::string concatenate(Args const&... args) { const size_t len = (0 + ... + std::string_view(args).size()); std::string result; result.reserve(len); (result.append(args), ...); return result; }
И работать с классом std::string, если речь о С++.eao197
06.07.2021 13:59Класс std::string для строчек в ~500 байт будет использовать динамическую память, а это может быть нежелательно по тем или иным причинам. Кроме того, могут быть причины для того, чтобы оставить код в его исходном виде, не переписывая на модный и современный стандарт C++.
valeramikhaylovsky
06.07.2021 14:22-1Если нужна статическая память, то есть полиморфные аллокаторы. А причин оставлять такой вонючий код я не вижу.
eao197
06.07.2021 14:29Ваше мнение понятно. Позволю, однако, напомнить, что свой вопрос я задавал не вам, а разработчику PVS-Studio чтобы получить ответ из первых рук.
Stolyarrr Автор
06.07.2021 16:09+1Да, если написать условие, то мы перестанем выдавать ложное срабатывание.
После обнаружения данного срабатывания мы добавили его, как пример, в документацию V645 и там предлагаем такой вариант фикса:size_t textSize = strlen(a.consoleText); if (sizeof(a.consoleText) - textSize > 5) { strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) - textSize - 5); }eao197
06.07.2021 16:23Понятно, спасибо за ответ.
sizeof(a.consoleText) — textSize > 5
Какой-нибудь gcc на высоких уровнях предупреждений ругнется на сравнение unsigned с signed. Уж извините за занудство :)
Кроме того, тут напрашивается еще вот какой момент: этот вариант можно считать надежным только в случае, если гарантируется, что в a.consoleText будет 0-символ. Если же разработчик где-то со значением consoleText ошибся и 0-символ содержится в sizeof(a.consoleText)+1u или sizeof(a.consoleText)+2, то (sizeof(a.consoleText) — textSize) приведет к переходу через ноль.
Но стоит ли этим заморачиваться, ведь отсутствие 0-символа в a.consoleText — это явно ошибочная ситуация, которая должна предотвращаться другими способами? У меня нет однозначного ответа на этот вопрос.
Stolyarrr Автор
07.07.2021 17:50Спасибо за замечание по поводу сравнения, поправил в документации. По поводу отсутствия 0-символа, мне кажется, это уже совсем частный случай)
sidorovmax
Сделали бы сразу size_t знаковым и не было бы такого рода ошибок.
Плюс функции strncpy и strncat провоцируют ошибки, ибо позволяют строке в итоге не оканчиваться на ноль.
Maccimo
Размер структуры в памяти не может быть отрицательным, следовательно и смысла делать
size_tзнаковым тоже нет. Тем паче, что от ошибки это никак бы не уберегло.dmitryrf
Такой есть: ssize_t — Used for a count of bytes or an error indication.
Но, как уже сказали, в данном случае это не решает проблему.