Начало коллекционирования ошибок в функциях копирования

04 Апр 2020

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

Феномен Баадера-Майнхоф? Нет, не думаю
Пример N1
Пример N2
Заключение

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

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

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

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

Кто-то может возразить, что два случая - это ещё не закономерность. И что, возможно, я обратил на них внимание исключительно из-за того, что они встретились мне через небольшое количество времени и сработал феномен Баадера-Майнхоф.

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

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

Ладно, перейдём к сути. Вступление для того, чтобы пока привести всего два примера, слишком затянулось :).

Пример N1

В статье про проверку Zephyr RTOS я описал вот такую неудачную попытку реализации аналога функции 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] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. shell.c 427

Анализатор сообщает, что функция memcpy копирует строчку, но не скопирует терминальный ноль, и это очень подозрительно. Кажется, что этот терминальный 0 копируется здесь:

((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;
}

Пример N2

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]; 
}

Этот код не мы сами выявили с помощью PVS-Studio, а я случайно встретил его на сайте Stack Overflow: C and static Code analysis: Is this safer than memcpy?

Впрочем, если проверить эту функцию с помощью анализатора PVS-Studio, он справедливо заметит:

V104 Implicit conversion of 'i' to memsize type in an arithmetic expression: i < n test.cpp 26
V108 Incorrect index type: cdest[not a memsize-type]. Use memsize type instead. test.cpp 27
V108 Incorrect index type: csrc[not a memsize-type]. Use memsize type instead. test.cpp 27

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

При большем размере копируемого буфера произойдёт переполнение знаковой переменной, что с точки зрения языка C и C++ является неопределённым поведением. И, кстати, не старайтесь угадать, как именно проявит себя ошибка. Это на самом деле непростая тема, про которую можно прочитать в статье "Undefined behavior ближе, чем вы думаете".

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

Заключение

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

#Cpp