Why we don't have to free the embedded pointer of a struct in C

Question

I have seen the following code,

/* stack.c */
typedef struct Stack *StackPtr;

struct Stack
{
    int *mStack;
    int mCurSize;
};

StackPtr StackCreate()
{
    return (StackPtr) calloc(sizeof(struct Stack), 1);
}

void StackDestroy(StackPtr stack)
{
    if (stack)
    {
        free(stack);
    }
}

void StackPush(StackPtr stack, int val)
{
    if (! stack)
        return;

    if (stack->mStack)
    {
        int newsize = stack->mCurSize + 1;
        int *newptr = realloc(stack->mStack, sizeof(struct Stack)*newsize);
        if (newptr)
        {
            stack->mStack = newptr;
            stack->mStack[newsize-1] = val;
            stack->mCurSize = newsize;
        }
    }
    else
    {
        stack->mStack = malloc(sizeof(struct Stack));
        if (stack->mStack)
        {
            stack->mStack[0] = val;
            stack->mCurSize = 1;
        }
    }
}

int StackPop(StackPtr stack)
{
    if (! StackIsEmpty(stack))
    {
        return stack->mStack[--stack->mCurSize];
    }
    return 0;
}


void StackDestroyMyWay(StackPtr stack) // This is my understanding
{
    if (stack)
    {
        if (stack->mStack)
            free(stack->mStack);
        free(stack);
    }
}

int StackIsEmpty(const StackPtr stack)
{
    return stack == NULL || stack->mCurSize == 0;
}

/* main.c */
int main(int argc, char *argv[])
{
    /* Create a new stack */
    StackPtr stack = StackCreate();
    int val;

    /* push and pop a value to the stack */
    printf( "Empty: %d\n", StackIsEmpty(stack));
    StackPush(stack, 10);
    printf("Empty: %d\n", StackIsEmpty(stack));
    val = StackPop(stack);
    printf("Popped off: %d\n", val);
    printf("Empty: %d\n", StackIsEmpty(stack));

    /* clean up the stack */
    StackDestroy(stack);
    return 0;
}

Question> I assume the original StackDestory is implemented correctly but I don't understand why we don't have to free stack->mStack explicitly.

I think your way is the correct way. Given what you have shown us mStack will leak unless explicitly freed. — John Knoeller, Dec 06 '11 at 16:47
I think you have to, and original implementation has a memory leak. Unless it is freed somewhere in the code that you don't show :) — , Dec 06 '11 at 16:47
I think `StackDestroyMyWay()` is correct too. Note, however, that [you don't have to check for null before `free()`](http://stackoverflow.com/q/1912325/10077). So you could just say, `if (stack) { free(stack->mStack); free(stack);}`. — Fred Larson, Dec 06 '11 at 16:50
For each malloc/realloc/calloc, you need one call to free. It's a 1-1 correspondance. — u0b34a0f6ae, Dec 07 '11 at 09:16

cnicutar · Accepted Answer · 2011-12-06T16:51:17.967

6

Actually you do have to free mStack somewhere or you will leak memory. If StackDestroy doesn't do it for you, you'll have to do it yourself later.

When designing an API that allocates and frees stuff think of a few things:

Did the client allocate the object ? Perhaps he should also free it. Is it possible he passed an object that wasn't obtained via malloc ?
Can the client do useful things with the object beyond the demise of our object ?

In your case the client doesn't even know about the existence of mStack (technically you could use opaque objects) so, since you allocate it, you should also free it.

edited Dec 06 '11 at 16:51

answered Dec 06 '11 at 16:46

cnicutar

178,505
25
365
392

So you think the function `StackDestroyMyWay` is implemented correctly? – q0987 Dec 06 '11 at 16:47
@q0987: I think everyone does so, unless there are some details that you are not aware about... – Dec 06 '11 at 16:48

Why we don't have to free the embedded pointer of a struct in C

1 Answers1