Как стать автором
Обновить

Комментарии 33

Сделали бы сразу size_t знаковым и не было бы такого рода ошибок.
Плюс функции strncpy и strncat провоцируют ошибки, ибо позволяют строке в итоге не оканчиваться на ноль.

Размер структуры в памяти не может быть отрицательным, следовательно и смысла делать size_t знаковым тоже нет. Тем паче, что от ошибки это никак бы не уберегло.

Такой есть: ssize_t — Used for a count of bytes or an error indication.
Но, как уже сказали, в данном случае это не решает проблему.
Сугубо для меня, из статьи очень сложно понять в чем на самом деле ошибка. Сначала сам разобрался, а потом пришлось потратиь время, что бы понять что именно это имеется ввиду в статье. Но за статью в целом плюс.
Спасибо за фидбек! Учту при написании следующих статей.

Тут ещё и логическая ошибка, программист слишком перемудрил

strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) –
                                      strlen(a.consoleText) - 5);

в третьей позиции должно быть:

Maximum number of characters to be appended.

так что sizeof(a.consoleText) было бы более чем достаточно.

Правда это не замечание для анализатора, а для кода в целом.

В случае просто sizeof() туда можно будет скопировать больше чем влезет...

Логику этого поведения мне не совсем понять (это именно сколько добавить, а не сколько всего места есть в буфере...) но таки правильно записано. ну, почти.

https://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"

лучше смотреть 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;
}

вот, что бывает, когда долго работаешь со фреймворками, забываешь стандартное поведение.

Я так понимаю, что a.consoleText не обязательно zero-terminated может быть, к тому же.
И чисто теоретически, strlen(a.consoleText) может вернуть любой мусор в этом случае.

Почему? strncat делает, цитирую, "Appends the first num characters of source to destination, plus a terminating null-character.". Именно из-за "plus a terminating null-character" нам и надо вычитать в третьем аргументе 1цу

Человек написал о том, что строка может быть не C-строкой, а просто массивом байт без завершения. Кто знает, что туда записали до этого?!

Это вопрос контракта. Использование на массиве str* функций подразумевает что данные тут предполагают 0-терминацию.

Судя по всему не заполнять массив настолько, чтобы целочисленного переполнения не произошло - это тоже вопрос контракта? Зачем нам статический анализатор? Работаем на доверии и без ошибок.

Вот как раз следить за тем что контракт сохранится до и после каждой из функций и нужен статан.

А не хотите выделить подобные случаи в отдельную диагностику? "buffer overflow" тут выглядит не очень понятным, хотелось бы видеть что-то типа "третий аргумент не должен быть отрицательным" (если он отрицательный – это уже ошибка, и не надо даже смотреть, переполняется ли буфер).

Так он не будет отрицательным. Там беззнаковое целое. Его переполнение (выход за верхнюю/нижнюю границу допустимых значений) это "законная" операция в Си.

Да, действительно. Но всё равно, хоть формально и законное, по смыслу – нет. Можно диагностировать :-)
Вроде статья про С++, но код на СИ. Почему не использоватьstd::string_view и std::string и STL? Сишный код это почти всегда источник багов. Современный С++ даёт возможно писать надёжный код, но этой возможностью не пользуются, даже разработчики статического анализатора для С++.

Или просто поменять тэг у статьи.

А откуда появилось магическое число пять?

Здесь суть в самом предупреждении, в том, что оно не напоминает про underflow. Если оно даже запутало разработчика PVS-Studio, то что уж делать простым смертным.

Возможно, можно как-то изменить предупреждение, чтобы оно напоминало про underflow в этом конкретном примере, но это будет очень сложно, и я даже не представляю, как это может работать с точки зрения пользователя.
Хотелось бы видеть достойные внимания примеры кода, а не устаревшие решения. В работе постоянно сталкиваюсь с тем, что новобранцы пишут код самым непотребным образом, потому, что в инете примеры именно такие, вот пример:

Надо подсчитать количество подстрок в строке на С++.

Решение, которое пишут в 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 резко ниже.
Количество вхождений подстроки в строку, в смысле? И видимо непересекающихся вхождений.

То, что небольшое тело цикла упихано в for — по-моему скорее антипаттерн, не слишком удобно читается.

А как должен быть исправлен проблемный фрагмент чтобы PVS-Studio перестал диагностировать ошибку? Перед вызовом strncat нужно сделать проверку длины строки? Типа:


if(strlen(a.consoleText)+5u < sizeof(a.consoleText))
{
   strncat(a.consoleText, inputBuffer, sizeof(a.consoleText) –
                                      strlen(a.consoleText) - 5);
}
Наверное, код должен был бы выглядеть иначе, например написать функцию слияния строк, что-то вроде:
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, если речь о С++.

Класс std::string для строчек в ~500 байт будет использовать динамическую память, а это может быть нежелательно по тем или иным причинам. Кроме того, могут быть причины для того, чтобы оставить код в его исходном виде, не переписывая на модный и современный стандарт C++.

Если нужна статическая память, то есть полиморфные аллокаторы. А причин оставлять такой вонючий код я не вижу.

Ваше мнение понятно. Позволю, однако, напомнить, что свой вопрос я задавал не вам, а разработчику PVS-Studio чтобы получить ответ из первых рук.

Да, если написать условие, то мы перестанем выдавать ложное срабатывание.

После обнаружения данного срабатывания мы добавили его, как пример, в документацию V645 и там предлагаем такой вариант фикса:

size_t textSize = strlen(a.consoleText);
if (sizeof(a.consoleText) - textSize > 5)
{
     strncat(a.consoleText, inputBuffer, 
              sizeof(a.consoleText) - textSize - 5);
}

Понятно, спасибо за ответ.


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 — это явно ошибочная ситуация, которая должна предотвращаться другими способами? У меня нет однозначного ответа на этот вопрос.

Спасибо за замечание по поводу сравнения, поправил в документации. По поводу отсутствия 0-символа, мне кажется, это уже совсем частный случай)
Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.