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

    memcpy

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

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


    Как член команды 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, а я случайно встретил его на сайте StackOverflow: 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

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

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

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

    Заключение


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



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Starting My Collection of Bugs Found in Copy Functions.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

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

      +4
      Везде где есть циклы for и while будет огромное поле для ошибок. Разве не для ибегания этого придумали STL в C++?
        +5

        В первом примере хочется (в C++) передавать const аргумент, тогда ошибку заметит компилятор.

          +3

          Всегда топлю за const-correctness. Не панацея, но хотя бы подобного рода ошибок позволяет избежать.


          В качестве возражения Андрею, допускаю, что memcpy() может быть реализовать более оптимально, нежели strcpy(): не нужно проверять признак конца строки, можно копировать словами и т.п. С другой стороны, там несколько раз считается длинна строки через strlen()...

            –2
            Дык не считайте её — посчитайте один раз и положите в переменную — длина же не меняется на протяжении этой функции.

            ПС
            Вообще пример 1 — это бред сивой кобылы имхо — если такое пишут внутри ОС — за это надо наказывать у позорного столба

            ПС2
            А зачем вообще коллекционировать функции копирования памяти? Одно то что таковые наличествуют в коде — говорит о том, что это странный код и странный программист, который решил переписывать стандартные решения на свои костыли(если конечно это не разработчик стандартной библиотеки для какого либо компилятора, делающий это для оптимизации конкретно под него)
              +1
              не считайте её — посчитайте один раз и положите в переменную

              О том и речь, если автор кода думал о производительности, используя memset(), то почему он длину считает несколько раз?


              Вообще пример 1 — это бред сивой кобылы имхо — если такое пишут внутри ОС — за это надо наказывать у позорного столба

              Это вариант strdup со своим распределителем. В ядре может быть уйма распределителей памяти под разные критерии. К примеру, для DMA могут быть критичными требования непрерывности блока в физической памяти. А для остальных подсистем — достаточно непрерывного блока виртуальной памяти. Какой-то распределитель по-умолчанию выделяет из некешируемой памяти (для взаимодействия с другими аппаратными блоками, к примеру) и так далее. Так что если по существу, опишите, что не так?


              Одно то что таковые наличествуют в коде — говорит о том, что это странный код и странный программист

              Я странный программист, я написал необходимое подмножество стандартной библиотеки и C++ рантайма для используемого проекта на контроллере, что позволило сэкономить 120кБ из 300кБ доступных для кода.

          +1
          Для пример 2 нельзя разве сделать
          while (n != 0)
          {
          --n;
          *cdest++ = *csrc++;
          }
          а для 32 разрядных можно немного увеличить размер и сделать копирование по 4 байта если
          ((csrc & 0x3 == 0) && (cdest & 0x3 == 0) && n >= 4)
          или по 2 байта если
          ((csrc & 0x1 == 0) && (cdest & 0x1 == 0) && n >= 2)
          ?
            +4
            а для 32 разрядных можно немного увеличить размер и сделать копирование по 4 байта

            Если нужен оптимизированный код, а каких-то специальных требований нет — используйте memcpy, а не изобретайте велосипеды. Выше вероятность допустить ошибку в подобном коде.

              0
              Этот ответ надо перенаправить писателям примера 2. Я и так использую memcpy.
              0
              Для пример 2 нельзя разве сделать

              Читать такой код сложнее, а преимущества неочевидны. Современные компиляторы всё равно эти циклы оптимизируют. Например, clang использует инструкцию movups из SSE. Не факт, что вариант с while сэкономит память или будет быстрее.


              а для 32 разрядных можно немного увеличить размер и сделать копирование по 4 байта если
              Компилятор и так будет блоками копировать. Вот, можно поизучать ассемблерный код после оптимизации.
                0

                Впрочем, в libgcc используется такой вариант:


                #include <stddef.h>
                
                void *
                memcpy (void *dest, const void *src, size_t len)
                {
                  char *d = dest;
                  const char *s = src;
                  while (len--)
                    *d++ = *s++;
                  return dest;
                }

                Но он и написан 9 лет назад, да и GCC менее агрессивно такие случаи оптимизирует при -O2.

                  –3
                  *d++ = *s++; какой же жестяк…
                    +3

                    Почему? Достаточно популярная конструкция.

                      +3

                      А в чём, собственно, проблема?
                      Вы видите более приемлемый вариант? По какому критерию?

                        –5
                        Ну вот эти два плюсика в конце, да ещё и дважды в одном выражении. Во всех мейнстримовых языках от них давно отказались. А ещё они бывают и вначале переменной.
                          +3
                          Жалко, что первое апреля прошло. Иначе было бы хорошей шуткой: «В рамках программы переведения с++ в „мейнстримовый язык“, от обеих форм операторов „++“ и „--“ отказались. Вследствие этого „с++“ тоже пришлось переименовать. По логике, просто отказались от „++“. Но потом кто-то подсказал, что язык с названием „с“ уже существует...»

                          А если серьёзно, код, который Вам не понравился — нормальный код для с. Да и для с++, в общем-то, тоже не вижу криминала.
                            +3

                            Да ну, серьёзно?
                            Java, C# — не мейнстрим? Kotlin?
                            Инфиксные, да, есть не везде, но это чисто синтаксические заморочки авторов.

                  0

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


                  char* newStr(const char* s, int n) {
                      if (!s) {
                          return 0;
                      }
                      char* result = new char[n + 1];
                      strncpy(result, s, n);
                      result[n] = '\0';
                      return result;
                  }
                    0
                    Если реально строка очень короткая, делаете бессмысленную работу: strncpy() заливает весь хвост буфера NULами.
                    (Хотя полезно для копирования паролей и т.п. — чтобы нельзя было косвенно узнать длину.)
                    Ну и случай превышения входной строкой указанной длины не ловится.
                      0

                      Слишком длинная строка обрежется до указанной длины.

                        0
                        Я о том, что оно обрежется молча.
                          –2

                          Мир C++ жесток.

                            0

                            А причём тут C++?

                    +3
                    Ждём аналога memmove — там поле не паханное для багодельни.
                      0

                      Если следовать описанию в стандарте — можно много вещей проигнорировать, как и во всяких strcpy.
                      Да, анализатор, к сожалению, читать пока не умеет — потому могут быть интересные срабатывания.

                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                    Самое читаемое