Красивая ошибка в реализации функции конкатенации строк
У нас, разработчиков статического анализатора кода PVS-Studio, специфическое представление о красоте. О красоте багов. Нам нравится находить изящество в ошибках, разбираться в них, пытаться угадать, как они появились. Сейчас как раз интересный случай, когда в коде спутались понятия длины и размера.
Ошибка из проекта LFortran
Услышав о выходе очередного CppCast на тему LFortran, мы решили проверить этот самый LFortran. Проект небольшой, поэтому не знаем, наберется ли материал для классической статьи о проверке открытого проекта. Однако на глаза сразу попала ошибка, которая заслуживает написания отельной маленькой заметки. На наш вкус, ошибка очень милая.
В проекте LFortran есть функции для конкатенации (объединения) двух строк в новом буфере.
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = strlen(&trmn);
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}Если хотите, прежде чем мы разберём этот код, вы можете сами попробовать найти ошибку. Чтобы случайно не прочитать пояснение, вставлю длинную картинку :). Есть мем «длиннокот». А у нас будет «длинноединорог» :).
Функция должна работать следующим образом. Вычисляется размер буфера, способного вместить обе объединяемые строки и терминальный ноль. Выделяется буфер, в него копируются строки и добавляется терминальный ноль. На самом деле, выделяется буфер недостаточного размера. Его размер на 1 байт меньше, чем требуется. В результате терминальный ноль будет записан уже за пределами выделенного буфера.
Программист, писавший код, увлёкся использованием функции strlen и использовал её даже для того, чтобы определить размер терминального нуля. Произошла путаница между размером объекта (терминального нуля) и длиной пустой строки. Это странный и неверный код. Но на наш взгляд, это красивая необычная ошибка.
Пояснение:
char trmn = '\0';
int trmn_size = strlen(&trmn);Здесь символ trmn интерпретируется как пустая строка. Её длина нулевая. Соответственно, переменная trmn_size, название которой означает размер терминального нуля, всегда будет равна 0.
Нужно было не считать длину пустой строки, а посчитать с помощью оператора sizeof, сколько байт занимает терминальный символ. Правильный код:
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = sizeof(trmn); // <=
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}Обнаружение ошибки
Как уже было сказано, ошибка была найдена с помощью статического анализатора кода PVS-Studio. К сожалению, он не смог обнаружить ошибку, именно как выход за границу массива. Это сделать достаточно сложно. Анализ потока данных не смог сопоставить, как связан размер буфера dest_char и значение переменной cntr, увеличивающейся в циклах. Ошибка была обнаружена косвенным путём.
PVS-Studio выдал предупреждение: V742 [CWE-170, CERT-EXP37-C] Function receives an address of a 'char' type variable instead of pointer to a buffer. Inspect the first argument. lfortran_intrinsics.c 550
Очень странно считать длину строки с помощью strlen, передав ей указатель на одиночный символ. И действительно, в процессе изучения обнаруженная анализатором аномалия оказалась серьёзным багом. Статический анализ — это круто!
Продолжаем улучшать код
Выше уже было показано, как исправить ошибку в коде. Однако у кода есть и другие недостатки, на которые указывает анализатор. И было бы полезно провести дополнительный рефакторинг.
Во-первых, анализатору не нравится, что нет проверки указателя, который возвращает функция malloc. Это важно. Предупреждение: V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'dest_char'. Check lines: 553, 551. lfortran_intrinsics.c 553
Во-вторых, анализатор выдаёт несколько предупреждений на тему 64-битных ошибок. Код не готов к работе со строками, длина которых может быть больше INT_MAX символов. Понятно, что это экзотика, но всё равно некрасиво и потенциально опасно. Лучше использовать тип size_t вместо int.
Улучшенный вариант функции:
void _lfortran_strcat(const char** s1, const char** s2, char** dest)
{
if (s1 == NULL || *s1 == NULL ||
s2 == NULL || *s2 == NULL || dest == NULL)
{
// Какая-то обработка ошибки, уместная в данном проекте.
....
}
size_t cntr = 0;
const char trmn = '\0';
const size_t s1_len = strlen(*s1);
const size_t s2_len = strlen(*s2);
const int trmn_size = sizeof(trmn);
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
if (dest_char == NULL)
{
// Какая-то обработка ошибки, уместная в данном проекте.
....
}
for (size_t i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1)[i];
cntr++;
}
for (size_t i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2)[i];
cntr++;
}
dest_char[cntr] = trmn;
*dest = dest_char;
}Новый код тоже не претендует на идеальность, но он стал явно лучше. Спасибо за внимание. И приходите попробовать PVS-Studio для проверки собственных проектов.
Дополнительные ссылки
- Начало коллекционирования ошибок в функциях копирования.
- Теперь PVS-Studio ещё лучше знает, что за зверь такой — strlen.
- Разработка 64-битных приложений.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. A Beautiful Error in the Implementation of the String Concatenation Function.
