CODEWITHSUNDEEP
cpointersmemory-leaksvalgrind
CisBae
CisBae

Reputation: 165

C Memory leak with valgrind

I'm trying to implement a Queue using a linked list in C, in an open-source project. I'm at the point where I've implemented the Queue, wrote unit tests, and worked out most of the memory leaks using valgrind.

The problem now is that I've been trying to find the leak for these last 8 bytes for a couple hours now, and I can't seem to figure it out.

Here is the output from valgrind:

==25806== 8 bytes in 1 blocks are definitely lost in loss record 1 of 1
==25806==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==25806==    by 0x80484E3: node_create (in /home/karysto/c-datastructures/queue/tests.out)
==25806==    by 0x8048511: enqueue (in /home/karysto/c-datastructures/queue/tests.out)
==25806==    by 0x8048851: test_dequeue_updates_size (in /home/karysto/c-datastructures/queue/tests.out)
==25806==    by 0x8048978: test_suite (in /home/karysto/c-datastructures/queue/tests.out)
==25806==    by 0x80489B0: main (in /home/karysto/c-datastructures/queue/tests.out)
==25806== 
==25806== LEAK SUMMARY:
==25806==    definitely lost: 8 bytes in 1 blocks
==25806==    indirectly lost: 0 bytes in 0 blocks
==25806==      possibly lost: 0 bytes in 0 blocks
==25806==    still reachable: 0 bytes in 0 blocks
==25806==         suppressed: 0 bytes in 0 blocks
==25806==

Here's my structs for the queue linked list:

struct Queue {
    int size;
    struct Node *front;
    struct Node *back;
};

struct Node {
    int data;
    struct Node *next;
};

Here's queue.c, the implementation for dequeue.

void
dequeue(struct Queue *q) {

    /* Dequeue from an empty queue. */
    if (q->size == 0) {
        return;
    }

    /* Dequeue the only node. */
    else if (q->size == 1) {
        q->front = NULL;
        q->back  = NULL;
        free(q->front);
        free(q->back);
    }

    /* Regular case. */
    else if (q->size > 1) {
        struct Node *temp;
        temp = q->front;
        q->front = q->front->next;
        free(temp);
    }

    --(q->size);
    return;
}

And here's the enqueue in the same file.

void
enqueue(struct Queue *q, int data) {
    struct Node *head = node_create(data);

    /* First enqueue on empty. */
    if (q->front == NULL && q->back == NULL) {
        q->front = head;
        q->back  = head;
    }

    else {
        q->back->next = head;
        q->back       = head;
    }

    ++(q->size);
    return;
}

For completness, here's the node_create function that is being referenced from enqueue:

struct Node*
node_create(int d) {
    struct Node *n = malloc(sizeof(struct Node));
    n->data = d;
    n->next = NULL;
    return n;
}

Upvotes: 1

Views: 365

Answers (2)

M Oehm
M Oehm

Reputation: 29126

I think that your problem lies in dequeuing the last node:

/* Dequeue the only node. */
else if (q->size == 1) {
    q->front = NULL;
    q->back  = NULL;
    free(q->front);
    free(q->back);
}

You effectively call free(NULL) twice. free(NULL) is allowed, but it doesn't do anything. So you end up not freeing the node and lose the memory. You should assign NULL to the front and back after freeing and you should also free the last node only once:

/* Dequeue the only node. */
else if (q->size == 1) {
    free(q->front);
    q->front = NULL;
    q->back  = NULL;
}

(Here, the front and back are the same. You are also remobving at most one node when dequeuing, so you should only free at most one node.)

Upvotes: 5

Betamos
Betamos

Reputation: 28897

How about:

/* Dequeue the only node. */
    else if (q->size == 1) {
        free(q->front);
        q->front = NULL;
        q->back  = NULL;
    }

Explanation: In your original solution you were first pointing both q->front and q->back to NULL before free()ing them. Running free() on NULL doesn't destroy the universe but it doesn't do very much, i.e. not free()ing your last node.

Upvotes: 1

Related Questions