Elyasin Elyasin - 3 months ago 13
C Question

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?

Answer

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