free() the value of a struct** pointer

Question

I'm curious about how to delete a struct that I stored in a list. I tried this code but it gives me a segmentation fault error and I can't see the mistake.

typedef struct double_stack_head_struct
{
    struct double_stack_head_struct* tail;
    double                           value;
} double_stack_head;

typedef struct double_stack_struct    // the structure containing the state of a stack
{
    int                size;     // size of stack
    double_stack_head* head;
} double_stack;

void push_double_stack(double_stack* stack, double value)
{
    double_stack_head* new_head = malloc(sizeof(double_stack_head));
    if(new_head != NULL) {
        new_head->tail = stack->head;
        new_head->value = value;

        stack->head = new_head;
        stack->size += 1;
    }
}

int pop_double_stack(double_stack* stack, double* value)
{
    if(empty_double_stack(stack)) {
        return false;
    } else {
        *value = stack->head->value;
        free(stack->head);
        stack->size -= 1;
        stack->head = malloc(sizeof(stack->head));
        stack->head = stack->head->tail;
        return true;
    }
}

Your pop function calls malloc, then immediately overwrites the saved pointer. This leaks, and may also be the cause of the crash (if the other value isn't also a valid pointer from malloc). — nobody, May 23 '15 at 14:37
It also makes little sense to allocate a new head after popping. — nobody, May 23 '15 at 14:38

score 1 · Answer 1 · answered May 23 '15 at 14:50

Remember that malloc and free should be symmetrical with respect to each other, in that for a given malloc call to allocate memory on the heap, there should be a matching free call to deallocate it.

If we just glance at your stack logic, your push function allocates a new node. Your pop function frees a node, but it also allocates a new one. So we have something asymmetrical here, and that's going to be a problem.

Another thing to note is that sizeof(struct Node*) is different from sizeof(struct Node). When you do sizeof(stack->head), you're actually checking the size of the type of size->head which is the size of the pointer rather than the pointee.

However, you don't really want to be allocating new nodes in a pop function, so its your stack logic that you need to correct. When you pop from a singly-linked list like this, you want to capture a pointer to the node for the head (top of the stack), set the head to point to the next element (right below the top of the stack), and free that former head.

Thanks I had a minor mistake, but you are right. i took a shower and noticed that I have to copy the "tail" which I didnt. Code works fine now. — Bui Thanh Binh, May 23 '15 at 18:17

score 0 · Answer 2 · answered May 23 '15 at 14:41

0

  don't do this!!

  stack->head = malloc(sizeof(stack->head));

use this:

  stack->head = (double_stack_head *)malloc(sizeof(double_stack_head));

also pay attention on the following function

 void push_double_stack(double_stack* stack, double value)

it creates a reference cycle between double_stack and double_stack_struct

answered May 23 '15 at 14:41

Narcisse Doudieu Siewe

629
5
15

[casting the return of `malloc` in *C* is not a good idea](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc), and [`sizeof(*stack->head);`](http://stackoverflow.com/questions/17258647/why-is-it-safer-to-use-sizeofpointer-in-malloc) is also preferable to explicitly mentioning the type. – ryanpattison May 23 '15 at 15:08

free() the value of a struct** pointer

2 Answers2