Красивая ошибка в реализации функции конкатенации строк

22 Июл 2021

Автор: Андрей Карпов

Ошибка из проекта LFortran
Обнаружение ошибки
Продолжаем улучшать код
Дополнительные ссылки

У нас, разработчиков статического анализатора кода 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);
    char* dest_char = (char*)malloc((s1_len+s2_len+1)*sizeof(char));
    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 для проверки собственных проектов.