Я уже несколько раз замечал, что программисты склонны делать ошибки в простых функциях копирования. Написание серьезной статьи на эту тему займет довольно много времени, так как мне придется провести тщательное исследование и собрать образцы, но сейчас я хотел бы поделиться парой примеров, на которые я недавно наткнулся.

Феномен Баадера-Майнхоф? я так не думаю

Как член команды PVS-Studio я сталкиваюсь с многочисленными ошибками, обнаруженными с помощью нашего инструмента в различных проектах. И как DevRel я люблю рассказывать об этом людям :). Сегодня я расскажу о неправильно реализованных функциях копирования.

Я видел такие функции раньше, но никогда не записывал их, так как не считал их достойными упоминания. Но так как я обнаружил тенденцию, я не могу не начать их собирать. Для начала я покажу вам два недавно найденных экземпляра.

Вы можете возразить, что два случая еще не формируют тенденцию; что я обратил внимание исключительно потому, что они произошли слишком близко по времени и сработал феномен Баадер-Майнхоф.

Феномен Баадера-Майнхоф, также называемый частотной иллюзией, представляет собой когнитивное искажение, при котором человек натыкается на фрагмент информации и вскоре после этого снова сталкивается с тем же предметом, что заставляет его думать, что этот предмет появляется исключительно часто.

Я не думаю, что это так. У меня уже был подобный опыт с плохо написанными функциями сравнения, и мое наблюдение впоследствии было подтверждено реальными примерами: Зло внутри функций сравнения.

Хорошо, давайте к делу. Это введение было слишком длинным для краткой заметки о двух примерах :).

Пример 1

В статье о проверке ОСРВ Zephyr я упоминал о неудачной попытке сделать функцию, которая должна работать как strdup:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;
  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}

Диагностическое сообщение PVS-Studio: V575 [CWE-628] Функция memcpy копирует не всю строку. Используйте функцию strcpy / strcpy_s, чтобы сохранить терминальный нуль. оболочка.c 427

Анализатор говорит, что функция memcpy копирует строку, но не может скопировать завершающий нулевой символ, что является очень странным поведением. Вы можете подумать, что копирование завершающего нуля происходит в следующей строке:

((u8_t *)mntpt)[strlen(mntpt)] = '\0';

Но это неправильно — это опечатка, из-за которой завершающий ноль копируется сам в себя. Обратите внимание, что целевой массив — это mntpt, а не cpy_mntpt. В результате функция mntpt_prepare возвращает незавершенную строку.

Вот как на самом деле должен был выглядеть код:

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';

Однако я не вижу причин для такой сложной и нестандартной реализации этой функции. Из-за этого чрезмерного усложнения то, что должно было быть маленькой и простой функцией, оказалось с критической ошибкой. Этот код можно сократить до следующего:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;
  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}

Пример 2

void myMemCpy(void *dest, void *src, size_t n) 
{ 
   char *csrc = (char *)src; 
   char *cdest = (char *)dest; 
   for (int i=0; i<n; i++) 
     cdest[i] = csrc[i]; 
}

Мы не поймали этого; Я наткнулся на StackOverflow: C и статический анализ кода: это безопаснее, чем memcpy?

Что ж, если вы проверите эту функцию с помощью PVS-Studio, она ожидаемо выдаст следующие предупреждения:

  • V104 Неявное преобразование i в тип memsize в арифметическом выражении: i ‹ n test.cpp 26
  • V108 Неверный тип индекса: cdest[не memsize-тип]. Вместо этого используйте тип memsize. test.cpp 27
  • V108 Неверный тип индекса: csrc[не memsize-тип]. Вместо этого используйте тип memsize. test.cpp 27

Действительно, в этом коде есть изъян, и на него указывалось в ответах на StackOverflow. Вы не можете использовать переменную типа int в качестве индекса. В 64-битной программе переменная int наверняка (об экзотических архитектурах мы сейчас не говорим) будет иметь длину 32 бита и функция сможет скопировать только столько байтов, сколько INT_MAX, т.е. не более 2 ГБ.

При копировании буфера большего размера произойдет переполнение целого числа со знаком, что является неопределенным поведением в C и C++. Кстати, не пытайтесь угадать, как именно проявит себя баг. Удивительно, но это довольно сложная тема, которая раскрыта в статье «Неопределенное поведение ближе, чем вы думаете».

Самое забавное, что код, показанный выше, был написан в попытке устранить какое-то предупреждение анализатора Checkmarx, срабатывающее при вызове функции memcpy. Самое мудрое, что мог придумать программист, — заново изобрести велосипед. Но получившаяся функция копирования, какой бы простой она ни была, оказалась ошибочной. Программист, вероятно, сделал вещи еще хуже, чем они уже были. Вместо того, чтобы пытаться найти причину предупреждения, он или она предпочли скрыть проблему, написав собственную функцию (тем самым запутав анализатор). Кроме того, они сделали ошибку, использовав в качестве счетчика переменную типа int. И да, такой код не может быть оптимизирован. Использование пользовательской функции вместо существующей эффективной и оптимизированной функции memcpy не является эффективным решением. Не делай этого :)

Вывод

Что ж, это только начало пути, и мне может понадобиться несколько лет, прежде чем я соберу достаточно примеров, чтобы написать глубокую статью на эту тему. Собственно, я только сейчас начинаю следить за такими случаями. Спасибо за прочтение и обязательно попробуйте PVS-Studio на своем C/C++/C#/Java-коде — возможно, вы найдете для себя что-то интересное.