Why does Valgrind complain about this code?

Question

I am trying to implement a linked list which stores non-negative integers. My implementation looks like this:

I was curious about memory leaks so I tried this tool named Valgrind with the command "valgrind --leak-check=yes".

==2540== error calling PR_SET_PTRACER, vgdb might block
==2540== Invalid write of size 4
==2540==    at 0x10875E: node_create (in LinkedList/bin/main)
==2540==    by 0x108832: list_append (in LinkedList/bin/main)
==2540==    by 0x108920: main (in LinkedList/bin/main)
==2540==  Address 0x522d098 is 0 bytes after a block of size 8 alloc'd
==2540==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2540==    by 0x10874B: node_create (in LinkedList/bin/main)
==2540==    by 0x108832: list_append (in LinkedList/bin/main)
==2540==    by 0x108920: main (in LinkedList/bin/main)
   .
   .
   .
==2540== Invalid read of size 4
==2540==    at 0x1088BA: list_pop (in LinkedList/bin/main)
==2540==    by 0x1089E1: main (in LinkedList/bin/main)
==2540==  Address 0x522d138 is 0 bytes after a block of size 8 alloc'd
==2540==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2540==    by 0x10874B: node_create (in LinkedList/bin/main)
==2540==    by 0x108832: list_append (in LinkedList/bin/main)
==2540==    by 0x108942: main (in LinkedList/bin/main)
   .
   .
   .
==2540== HEAP SUMMARY:
==2540==     in use at exit: 0 bytes in 0 blocks
==2540==   total heap usage: 10 allocs, 10 frees, 584 bytes allocated
==2540==
==2540== All heap blocks were freed -- no leaks are possible

The corresponding functions are implemented like this:

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

struct List {
    struct Node* head;
};

typedef struct Node* Node;
typedef struct List* List;

Node node_create(int value, Node nextNode) {
    if(value < 0) {
        printf("Error: Could not create node, value is negative.\n");
        return NULL;
    }

    Node node = malloc(sizeof(Node));
    if(node != NULL)
    {
        node->value = value;
        node->next = nextNode;
    } else {
        printf("Error: Could not create node, malloc returned NULL.\n");
    }

    return node;
}

int list_append(List listHandle, int value) {
    Node current = listHandle->head;
    Node new = node_create(value, NULL);

    if(new == NULL) {
        return -1;
    }

    if(current == NULL) {
        listHandle->head = new;
    } else {
        while(current->next != NULL) {
            current = current->next;
        }
        current->next = new;
    }

    return value;
}

int list_pop(List listHandle) {
    if(listHandle->head == NULL) {
        printf("Error: Trying to pop an empty list.\n");
        return -1;
    }

    Node temp = listHandle->head;
    int value = temp->value;

    if(temp->next == NULL)
    {
        listHandle->head = NULL;
    } else {
        listHandle->head = temp->next;
    }

    free(temp);
    return value;
}

What am I doing wrong? How can I improve the code? Is this even a problem or is Valgrind just being overly pedantic?

recompile with `-ggdb` for line numbers and show full code including main — Ctx, Oct 21 '18 at 21:23
Because of your `typedef`, `sizeof(Node)` is actually `sizeof(Node*)`, so you are not allocating enough memory for `struct Node`. — GSerg, Oct 21 '18 at 21:23
Never typedef pointers to something that doesn’t look lika a pointer. — Fredrik, Oct 21 '18 at 21:28
@Fredrik Thank you very much, that was the issue. I will take this a lesson and never typedef pointers again ;) — Phoony, Oct 21 '18 at 21:35
You've just demonstrated why [Is it a good idea to typedef pointers](https://stackoverflow.com/questions/750178/is-it-a-good-idea-to-typedef-pointers) says "No, except (perhaps) for function pointers". — Jonathan Leffler, Oct 21 '18 at 23:42

KamilCuk · Answer 1 · 2018-10-21T21:37:43.893

typedef struct Node* Node;

Node node = malloc(sizeof(Node));

This will allocate sizeof(Node) == sizeof(struct Node*) bytes of memory. So Node node points not into sizeof(struct Node) bytes of memory. You will end up with out of bound / invalid memory access.

To fix you code, dereference the pointer to a struct node or implicitly use struct node with sizeof:

Node node = malloc(sizeof(*node));
Node node = malloc(sizeof(struct Node));

This is only a fix. It makes your code more confusing, and you just discovered why a hidden pointer behind a typedef is a bad idea. The line:

Node node = malloc(sizeof(*Node));

will not work, as Node names a type, can't be derefenced, as pointed out by @Ctx in comments.

Personally I highly recommend to rewrite all your code to use:

 typedef struct Node Node;

 Node *node_create(int value, Node *nextNode)  {
      ...
      Node *node = malloc(sizeof(Node));
      ...
 }

Now any programmer immediately looking at the function node_create will know, that it returns a pointer to some data, probably dynamically allocated. Is is more readable and doesn't hides pointer assignment.

score 1 · Answer 2 · answered Oct 21 '18 at 21:30

1

Node node = malloc(sizeof(Node));

Node is actually struct Node * - a pointer. BOOM! You just allocated memory for a single pointer, not your struct, which needs at least sizeof(int) bytes more. That's why you don't typedef pointers.

answered Oct 21 '18 at 21:30

ForceBru

43,482
10
63
98

Why does Valgrind complain about this code?

2 Answers2