CODEWITHSUNDEEP
cfree
Ely
Ely

Reputation: 11152

Correct way to free memory for a structure that contains allocated memory referenced by pointers

I have the following function:

/* undef: from s from hashtab */
void undef(char *s) {
    struct nlist *currentPtr, *previousPtr;

    for (previousPtr = NULL, currentPtr = hashtab[hash(s)];
            currentPtr != NULL;
            previousPtr = currentPtr, currentPtr = currentPtr->next) {

        if (strcmp(currentPtr->name, s) == 0) {
            if (previousPtr == NULL) /* first element */
                hashtab[hash(s)] = currentPtr->next;
            else /* element in the middle or at the end */
                previousPtr->next = currentPtr->next;
            /* free memory */
            free(currentPtr->name);
            free(currentPtr->defn);
            //free(currentPtr);
        }
    }
}

currentPtr points to a memory allocated by malloc.

currentPtr->name and currentPtr->defn point to character arrays copied via strdup.

I am not sure what is the correct way to free the memory of a list item.

If I use 

free(currentPtr->name);
free(currentPtr->defn);

then I get no segmentation fault, but I believe the character array memory is freed, but the list structure element itself is not.

If I use 

free(currentPtr);

then I also get no segmentation fault, but I believe I freed the list structure element itself, but not the character array memory.

Using

free(currentPtr->name);
free(currentPtr->defn);
free(currentPtr);

gives me segmentation fault. But I thought that would be the correct way of doing it.

So which is correct? Why does it fail?

Upvotes: 0

Views: 72

Answers (1)

R Sahu
R Sahu

Reputation: 206567

You'll need to change your strategy a little bit since currentPtr is a dangling pointer after the call to

free(currentPtr);

Here's my suggestion:

for (previousPtr = NULL, currentPtr = hashtab[hash(s)];
        currentPtr != NULL;
        previousPtr = currentPtr) {

    if (strcmp(currentPtr->name, s) == 0)
    {
        if (previousPtr == NULL) /* first element */
            hashtab[hash(s)] = currentPtr->next;
        else /* element in the middle or at the end */
            previousPtr->next = currentPtr->next;

        /* free memory */
        free(currentPtr->name);
        free(currentPtr->defn);

        // Get hold of the next pointer before free'ing currentPtr
        struct nlist *tempPtr = currentPtr->next;
        free(currentPtr);
        currentPtr = tempPtr;
    }
    else
    {
        currentPtr = currentPtr->next;
    }
}

Update, a more streamlined version

Since you are using currentPtr->next in four places, you can streamline the loop by using:

struct nlist *nextPtr = NULL;
for (previousPtr = NULL, currentPtr = hashtab[hash(s)];
        currentPtr != NULL;
        previousPtr = currentPtr, currentPtr = nextPtr) {

    nextPtr = currentPtr->next;
    if (strcmp(currentPtr->name, s) == 0)
    {
        if (previousPtr == NULL) /* first element */
            hashtab[hash(s)] = nextPtr;
        else /* element in the middle or at the end */
            previousPtr->next = nextPtr;

        /* free memory */
        free(currentPtr->name);
        free(currentPtr->defn);
        free(currentPtr);
    }
}

Upvotes: 4

Related Questions