Блог

Функция для освобождения экземпляра struct Foo приведена ниже:

 void DestroyFoo(Foo* foo)
{
    if (foo) free(foo);
}
  

Мой коллега предложил вместо этого следующее:

 void DestroyFoo(Foo** foo)
{
    if (!(*foo)) return;
    Foo *tmpFoo = *foo;
    *foo = NULL; // prevents future concurrency problems
    memset(tmpFoo, 0, sizeof(Foo));  // problems show up immediately if referred to free memory
    free(tmpFoo);
}
  

Я вижу, что установка указателя на NULL после освобождения лучше, но я не уверен в следующем:

1. Нам действительно нужно назначить указатель на временный? Помогает ли это с точки зрения параллелизма и разделяемой памяти?

2. Действительно ли хорошая идея установить для всего блока значение 0, чтобы вызвать сбой программы или, по крайней мере, вывести результаты со значительным расхождением?

Нам действительно нужно назначить указатель на временный? Помогает ли это с точки зрения параллелизма и разделяемой памяти?

Это не имеет никакого отношения к параллелизму или общей памяти. Это бессмысленно.

Действительно ли хорошая идея установить для всего блока значение 0, чтобы вызвать сбой программы или, по крайней мере, вывести результаты со значительным расхождением?

Нет. Вовсе нет.

Решение, предложенное вашим коллегой, ужасно. Вот почему:

• Установка целого блока в 0 также ничего не дает. Поскольку кто-то случайно использует блок free()’ed, он не узнает об этом, основываясь на значениях в блоке. Именно такой блок calloc() возвращается. Таким образом, невозможно узнать, является ли это недавно выделенной памятью ( calloc() или malloc() memset() ) или той, которая была освобождена () вашим кодом ранее. Во всяком случае, для вашей программы требуется дополнительная работа по обнулению каждого освобождаемого блока памяти ()’ed.

• free(NULL); четко определено и не выполняется, поэтому if условие в if(ptr) {free(ptr);} ничего не дает.

• Поскольку free(NULL); не работает, установка указателя на NULL фактически скрыла бы эту ошибку, потому что, если какая-то функция на самом деле вызывается free() по уже свободному () ‘ed указателю, то они бы этого не знали.

• большинство пользовательских функций будут иметь нулевую проверку при запуске и могут не рассматривать передачу NULL ей как условие ошибки:

 void do_some_work(void *ptr) {
    if (!ptr) {
        return; 
    }

   /*Do something with ptr here */
}
  

Таким образом, все эти дополнительные проверки и обнуление дают ложное ощущение «надежности», в то время как на самом деле это ничего не улучшило. Это просто заменило одну проблему другой — дополнительными затратами на производительность и раздуванием кода.

Таким образом, простой вызов free(ptr); без какой-либо функции-оболочки является одновременно простым и надежным (большинство malloc() реализаций немедленно завершились бы сбоем при double free, что хорошо).

Нет простого способа обойти «случайный» вызов free() дважды или более. Ответственность программиста заключается в том, чтобы отслеживать всю выделенную память и free() ее соответствие. Если кому-то трудно с этим справиться, то C, вероятно, не тот язык для них.

То, что предлагает ваш коллега, сделает код «более безопасным» в случае, если функция вызывается дважды (см. sleske comment…as «безопаснее» может означать не одно и то же для всех …;-).

С вашим кодом это, скорее всего, приведет к сбою:

 Foo* foo = malloc( sizeof(Foo) );
DestroyFoo(foo);
DestroyFoo(foo); // will call free on memory already freed
  

С версией кода ваших коллег это не приведет к сбою:

 Foo* foo = malloc( sizeof(Foo) );
DestroyFoo(amp;foo);
DestroyFoo(amp;foo); // will have no effect
  

Теперь, для этого конкретного сценария, достаточно выполнения tmpFoo = 0; (внутри DestroyFoo ). memset(tmpFoo, 0, sizeof(Foo)); предотвратит сбой, если Foo имеет дополнительные атрибуты, к которым может быть получен неправильный доступ после освобождения памяти.

Итак, я бы сказал, да, это может быть хорошей практикой …. но это всего лишь своего рода защита от разработчиков, которые используют плохие методы (потому что определенно нет причин вызывать DestroyFoo дважды без перераспределения) … в конце концов, вы делаете DestroyFoo «безопаснее», но медленнее (это делает больше вещей, чтобы предотвратить его неправильное использование).

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

Что вам следует сделать, если вы хотите быть в безопасности, так это присвоить указателю значение NULL после освобождения памяти. Это всегда хорошая практика.

 Foo* foo = malloc( sizeof(Foo) );
DestroyFoo(foo);
foo = NULL;
  

Более того, я не знаю, почему люди проверяют, равен ли указатель нулю, перед вызовом free(). Это не обязательно, так как free () сделает эту работу за вас.

Установка памяти в 0 (или что-то еще) является хорошей практикой только в некоторых случаях, поскольку free () не будет очищать память. Это просто отметит область памяти как свободную, чтобы ее можно было использовать повторно. Если вы хотите очистить память, чтобы никто не смог ее прочитать, вам нужно очистить ее вручную. Но это довольно тяжелая операция, и именно поэтому ее не следует использовать для освобождения всей памяти. В большинстве случаев достаточно освобождения без очистки, и вам не нужно жертвовать производительностью для выполнения ненужных операций.

 void destroyFoo(Foo** foo)
{
    if (!(*foo)) return;
    Foo *tmpFoo = *foo;
    *foo = NULL;
    memset(tmpFoo, 0, sizeof(Foo));
    free(tmpFoo);
}
  

Код вашего коллеги плох, потому что

• произойдет сбой, если foo будет NULL
• нет смысла создавать дополнительную переменную
• нет смысла устанавливать значения равными нулям
• освобождение структуры напрямую не работает, если она содержит вещи, которые необходимо освободить

Я думаю, что ваш коллега мог бы иметь в виду такой вариант использования

 Foo* a = NULL;
Foo* b = createFoo();

destroyFoo(NULL);
destroyFoo(amp;a);
destroyFoo(amp;b);
  

В таком случае, это должно быть так. попробуйте здесь

 void destroyFoo(Foo** foo)
{
    if (!foo || !(*foo)) return;
    free(*foo);
    *foo = NULL;
}
  

Сначала нам нужно взглянуть на Foo , давайте предположим, что это выглядит следующим образом

 struct Foo
{
    // variables
    int number;
    char character;

    // array of float
    int arrSize;
    float* arr;

    // pointer to another instance
    Foo* myTwin;
};
  

Теперь, чтобы определить, как это должно быть уничтожено, давайте сначала определим, как это должно быть создано

 Foo* createFoo (int arrSize, Foo* twin)
{
    Foo* t = (Foo*) malloc(sizeof(Foo));

    // initialize with default values
    t->number = 1;
    t->character = '1';

    // initialize the array
    t->arrSize = (arrSize>0?arrSize:10);
    t->arr = (float*) malloc(sizeof(float) * t->arrSize);

    // a Foo is a twin with only one other Foo
    t->myTwin = twin;
    if(twin) twin->myTwin = t;

    return t;
}
  

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

 Foo* destroyFoo (Foo* foo)
{
    if (foo)
    {
        // we allocated the array, so we have to free it
        free(t->arr);

        // to avoid broken pointer, we need to nullify the twin pointer
        if(t->myTwin) t->myTwin->myTwin = NULL;
    }

    free(foo);

    return NULL;
}
  

Попробуйте здесь протестировать

 int main ()
{
    Foo* a = createFoo (2, NULL);
    Foo* b = createFoo (4, a);

    a = destroyFoo(a);
    b = destroyFoo(b);

    printf("success");
    return 0;
}
  

К сожалению, эта идея просто не работает.

Если целью было перехватить double free, это не распространяется на случаи, подобные следующему.

Предположим, что этот код:

 Foo *ptr_1 = (FOO*) malloc(sizeof(Foo));
Foo *ptr_2 = ptr_1;
free (ptr_1);
free (ptr_2); /* This is a bug */
  

Предложение состоит в том, чтобы написать вместо:

 Foo *ptr_1 = (FOO*) malloc(sizeof(Foo));
Foo *ptr_2 = ptr_1;
DestroyFoo (amp;ptr_1);
DestroyFoo (amp;ptr_2); /* This is still a bug */
  

Проблема в том, что второй вызов DestroyFoo() все равно завершится сбоем, потому что ptr_2 не сбрасывается в значение NULL и по-прежнему указывает на уже освобожденную память.