CODEWITHSUNDEEP
cpointersstruct
Eli Korvigo
Eli Korvigo

Reputation: 10493

Properly free dynamically allocated struct

I've already read several answers given for similar questions, but there is one thing that makes my case slightly different from what I've read. I use a linked list to store data for threads to pick and process. A node is a simple typedef 

struct QueueNode;

typedef struct QueueNode {
    struct QueueNode* next; // pointer to the next node
    void* data;
} QueueNode;

The list 

typedef struct LinkedQueue {
    QueueNode* head;   // pointer to the first Node
    QueueNode* tail;   // pointer to the last Node
    long long k;       // the number of nodes in the queue
} LinkedQueue;

Both are initialized by corresponding functions that use malloc. When a thread needs data to process it calls one function that pops the head of the queue and returns the void* data pointer. 

void* pop_data(LinkedQueue* queue) {
    /*
     Pops the head node from a queue, extracts data out of it and
     frees the memory allocated by that node
     */
    assert(queue->head && "Can't pop data from an empty queue");

    void* data = queue->head->data;            // extract data from head
    QueueNode* old_head_pointer = queue->head;
    queue->head = queue->head->next;           // replacing head with a new one
    destroy_node(old_head_pointer);            // destroying the old head

    return data;
};

The thing is that destroy_node is supposed to free the memory allocated for the node without destroying the void* data pointer, because the data are used later. This is were my case becomes different. All the examples I've already read described a case of completely freeing everything inside a node, while I need to save that one pointer. 

void destroy_node(QueueNode* node) {
    /*
     Frees memory allocated by a node.
     */
    free(node->next);
    free(node);
};

In my tests this works fine, but since I know that free() doesn't actually erase the piece of memory and since my machine has tons of memory the fact that I can still access that void* data pointer without any segmentation errors can't be relied on. So the question basically is am I doing this right or are my concerns really reasonable? If this indeed might lead to memory leaks or other memory-related problems, how am I supposed to do this stuff?

Upvotes: 0

Views: 1435

Answers (2)

user3629249
user3629249

Reputation: 16540

if you want to keep the pointer to the 'Data',

Then save that pointer into a local variable before calling 'destroy_node()'

The destroy_node() function is not having any effect on the memory where the 'void *Data' is pointing.

As mentioned elsewhere, do not free() the 'next' pointer, as that actually frees the next node in the linked list (which 'head' was just set to point to)

This means that 'head' is now pointing to unallocated memory.

Any reference to that unallocated memory results in undefined behaviour.

That undefined behaviour could lead to anything, including a seg fault event.

All compile operations should be performed with most/all warnings enabled.

with the warnings enabled, the posted code would raise 2 warnings.

each warnings says:

ISO C does not allow extra ';' outside of a function

Upvotes: 0

rems4e
rems4e

Reputation: 3172

You are doing right, as you are accessing the data pointer before freeing the whole node. Doing the reverse would be a logic error. There will not be memory leaks, provided you actually free the data somewhere later in your code.

You should however not free the next field of the node, as it will make that node invalid too, and this is probably not what you want to do.

Upvotes: 3

Related Questions