0

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?

Eli Korvigo
  • 10,265
  • 6
  • 47
  • 73
  • 3
    Using the code you show, you free the new `head` pointer. Don't free `node->next`. – Some programmer dude Jun 21 '15 at 10:00
  • `struct QueueNode` is an incomplete type throughout this code... Perhaps a nonconsequential mistake, but a mistake nonetheless, particularly if you try to dereference the `next` member. May I suggest inserting `QueueNode` directly after `typedef struct`? – autistic Jun 21 '15 at 12:12
  • It's actually defined as `typedef struct QueueNode {...` in the real code, but I appreciate the note. – Eli Korvigo Jun 21 '15 at 12:17
  • in general, typedef'ing a struct is a bad idea. typedef'ing the struct just clutters the code, clutters the compiler name space, and can lead to mis-understandings when the code is (perhaps some years from now) being maintained. – user3629249 Jun 21 '15 at 16:38
  • @user3629249 can you elaborate a little bit further on that? Every single tutorial I've seen so far shows typedefing structs is normal. – Eli Korvigo Jun 21 '15 at 16:42
  • there should be no semicolon ';' after the closing brace of a function body. – user3629249 Jun 21 '15 at 16:47
  • @user3629249 I've got functions that take and return these structs, hence I have to specify them as input and return types. Is it a valid case for typedefing structs? – Eli Korvigo Jun 21 '15 at 16:54
  • sure, typedef'ing a struct is allowed (and on certain cases desirable) however, this is not one of those few cases. Suggest googling the discussions about the use of a typedef on a struct, This link gives the (oh, it is too much to type 'struct ') excuse for using a typedef on a struct. – user3629249 Jun 21 '15 at 16:56
  • @user3629249 according to the link you provided, I have no choice but to use `typedef` and that is fairly obvious, because my functions won't compile otherwise. – Eli Korvigo Jun 21 '15 at 17:01

2 Answers2

3

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.

rems4e
  • 3,112
  • 1
  • 17
  • 24
  • Shouldn't I dynamically allocate space for the `void* data` pointer in the `pop_data` function and explicitly copy the pointer? As far as I believe `data` is a local variable and it gets freed upon `return` if it's not declared as `static` or dynamically allocated. – Eli Korvigo Jun 21 '15 at 10:10
  • `data` is indeed a local variable. But its content is probably the return value of a `malloc` call and is still valid. It's the address to the content that is returned to the caller, like you can return an integer value coming from a local variable without problem. – rems4e Jun 21 '15 at 10:15
0

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
user3629249
  • 16,402
  • 1
  • 16
  • 17