Est-ce un bon moyen de libérer de la mémoire ?

Question

La fonction permettant de libérer une instance de struct Foo est indiqué ci-dessous :

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

Un de mes collègues m'a suggéré la solution suivante :

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

Je constate que le fait de placer le pointeur sur NULL Après la libération, c'est mieux, mais je ne suis pas sûr de ce qui suit :

1. Est-il vraiment nécessaire d'assigner le pointeur à un pointeur temporaire ? Est-ce utile en termes de concurrence et de mémoire partagée ?

2. Est-ce vraiment une bonne idée de mettre tout le bloc à 0 pour forcer le programme à se planter ou au moins à produire des résultats avec des écarts significatifs ?

Answer 1

Est-il vraiment nécessaire d'assigner le pointeur à un pointeur temporaire ? Est-ce que cela en termes de concurrence et de mémoire partagée ?

Cela n'a rien à voir avec la concurrence ou la mémoire partagée. C'est inutile.

Est-ce vraiment une bonne idée de mettre l'ensemble du bloc le programme à se planter ou au moins à produire des résultats avec une significatifs ?

Non, pas du tout.

La solution proposée par votre collègue est terrible. Voici pourquoi :

• Le fait de mettre le bloc entier à 0 ne donne rien non plus. Parce que quelqu'un utilise un bloc free() par accident, il ne le saurait pas en se basant sur les valeurs du bloc. C'est le genre de bloc calloc() les retours. Il est donc impossible de savoir s'il s'agit d'une mémoire fraîchement allouée ( calloc() o malloc()+memset() ) ou celui qui a été free() par votre code plus tôt. En fait, c'est un travail supplémentaire pour votre programme que de remettre à zéro chaque bloc de mémoire qui est en train d'être libéré ().

• free(NULL); est bien défini et est un no-op, donc la fonction if condition en if(ptr) {free(ptr);} n'aboutit à rien.

• Depuis free(NULL); est sans effet, la mise en place du pointeur sur NULL cacherait en fait ce bogue, car si une fonction appelle en fait free() sur un pointeur déjà free(), alors ils ne le sauraient pas.

• la plupart des fonctions utilisateur vérifient la présence de NULL au début et n'envisagent pas de passer le mot "NULL". NULL en tant que condition d'erreur :

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

  /Do something with ptr here / }

Ainsi, toutes ces vérifications supplémentaires et la réduction à zéro donnent une fausse impression de "robustesse" alors qu'elles n'ont pas vraiment amélioré quoi que ce soit. Cela a juste remplacé un problème par un autre, le coût supplémentaire de la performance et du gonflement du code.

Il suffit donc d'appeler free(ptr); sans fonction enveloppante est à la fois simple et robuste (la plupart des malloc() s'effondreraient immédiatement en cas de double libération, ce qui est une bon chose).

Il n'y a pas de solution simple pour éviter d'appeler "accidentellement" des personnes. free() deux fois ou plus. Il est de la responsabilité du programmeur de garder une trace de toute la mémoire allouée et de tout ce qui a été alloué. free() de manière appropriée. Si quelqu'un trouve cela difficile à gérer, le langage C n'est probablement pas fait pour lui.

Answer 2

Ce que suggère votre collègue rendra le code plus "sûr" au cas où la fonction serait appelée deux fois (voir le commentaire de Sleske... car "plus sûr" ne signifie pas forcément la même chose pour tout le monde...;-).

Avec votre code, il y a de fortes chances qu'il y ait un plantage :

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

Avec la version du code de vos collègues, il n'y aura pas de plantage :

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

Maintenant, pour ce scénario spécifique, faire tmpFoo = 0; (au sein de DestroyFoo ) suffit. memset(tmpFoo, 0, sizeof(Foo)); évitera un crash si Foo a des attributs supplémentaires qui pourraient être accédés de façon erronée après la libération de la mémoire.

Je dirais donc que oui, il peut s'agir d'une bonne pratique....mais ce n'est qu'une sorte de la sécurité contre les développeurs qui ont de mauvaises pratiques (parce qu'il n'y a aucune raison d'appeler les développeurs à la sécurité). DestroyFoo deux fois sans la réaffecter) ...en fin de compte, vous faites DestroyFoo "plus sûr" mais plus lent (il fait plus de choses pour empêcher une mauvaise utilisation).

Answer 3

La deuxième solution semble trop élaborée. Bien sûr, dans certaines situations, elle pourrait être plus sûre, mais les frais généraux et la complexité sont tout simplement trop importants.

Si vous voulez être sûr de vous, vous devez mettre le pointeur à NULL après avoir libéré la mémoire. C'est toujours une bonne pratique.

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

De plus, je ne sais pas pourquoi les gens vérifient si le pointeur est NULL avant d'appeler free(). Ce n'est pas nécessaire car free() fera le travail à votre place.

Mettre la mémoire à 0 (ou autre chose) n'est une bonne pratique que dans certains cas, car free() n'efface pas la mémoire. Elle marque simplement une région de la mémoire comme étant libre afin qu'elle puisse être réutilisée. Si vous souhaitez effacer la mémoire, afin que personne ne puisse la lire, vous devez la nettoyer manuellement. Mais il s'agit d'une opération assez lourde et c'est pourquoi elle ne doit pas être utilisée pour libérer toute la mémoire. Dans la plupart des cas, libérer sans effacer est suffisant et il n'est pas nécessaire de sacrifier les performances pour effectuer une opération inutile.

Answer 4

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

Le code de votre collègue est mauvais parce que

• il se plantera si foo est NULL
• il est inutile de créer des variables supplémentaires
• il est inutile de mettre les valeurs à zéro
• libérer une structure directement ne fonctionne pas si elle contient des choses qui doivent être libérées

Je pense que votre collègue a peut-être à l'esprit le cas d'utilisation suivant

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

destroyFoo(NULL);
destroyFoo(&a);
destroyFoo(&b);

Dans ce cas, il faut procéder comme suit. essayez ici

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

---

Tout d'abord, nous devons jeter un coup d'œil sur Foo Supposons qu'il ressemble à ceci

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

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

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

Pour définir comment il doit être détruit, définissons d'abord comment il doit être créé

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

Nous pouvons maintenant écrire une fonction de destruction opposée à la fonction de création

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

Test essayez ici

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

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

    printf("success");
    return 0;
}

Answer 5

Malheureusement, cette idée ne fonctionne pas.

Si l'intention était de lutter contre la double liberté, elle ne couvre pas les cas suivants.

Supposons ce code :

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

La proposition consiste à écrire à la place :

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

Le problème est que le deuxième appel à DestroyFoo() se plantera quand même, parce que ptr_2 n'est pas réinitialisé à NULL et pointe toujours vers une mémoire déjà libérée.