why do I get an Invalid read of size 8

My goal is to pass an array of integers to a function that will return a pointer to the last Item in the linked list and it will fill the array of Item pointers with pointers for each item created

everything works fine and compiling does not show any errors and running the program does not show any errors as well but when I use valgrind with it I get the error in valgrind output below

typedef struct Item
{
    int num ;   
    struct Item *next;  
}Item;


Item * create_list(int * arr, int len, Item ** lst)
{
    Item * tmpItem = malloc(sizeof(Item));

    for (int i=0; i < len; i  )
    {
        lst[i] = tmpItem;
        tmpItem->num = arr[i];
        if ( i 1!=len )
        {
            tmpItem->next = malloc(sizeof(Item));
            tmpItem = tmpItem->next;
        }
        else
            tmpItem->next = NULL;
    }
    return tmpItem;
}



void free_lst(Item ** lst, int len)
{
    for (int i =0; i < len; i  )
    {
        free(lst[i]);
    }
}
int main()
{
    int arr[] = {1,2,3,4,5};
    Item * items[sizeof(arr)/sizeof(int)];
    Item * tmp = create_list(arr, sizeof(arr)/sizeof(int), items);
    free_lst(items, sizeof(arr)/sizeof(int));
    printf('%p\n',tmp->next);

}

valgrind output

==12169== Invalid read of size 8
==12169==    at 0x109270: main (main.c:26)
==12169==  Address 0x4a59228 is 8 bytes inside a block of size 16 free'd
==12169==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==12169==    by 0x10939D: free_lst (list.c:45)
==12169==    by 0x10926B: main (main.c:24)
==12169==  Block was alloc'd at
==12169==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==12169==    by 0x10931C: create_list (list.c:24)
==12169==    by 0x109209: main (main.c:13)
==12169== 

I don't understand why am I getting this error but I guess this is because the last element in the linked list has a null pointer and this causes the last element to be 8 bytes long not 16

CodePudding user response：

Two main bugs here:

1. printf('%p'... instead of printf("%p"...

2. You try to printf() tmp->next after you've already freed it in free_lst(). Meaning that you've already freed this heap block, but you're still trying to read from it. And that's why valgrind throws an error.

CodePudding user response：

Some problems

One is missing required headers.

typedef struct Item

See discussion of typedef.

Item * create_list(int * arr, int len, Item ** lst)

The first two arguments are necessary, but the last one is unused. In the function itself, one forward declares the pointer in a manner that is confusing and will not work for a null list. This could be simplified.

void free_lst(struct Item ** lst, int len)

This frees a list as an array. This doesn't make sense since one is converting it to a linked-list.

int main()

See What's the correct declaration of main()?.

Item * items[sizeof(arr)/sizeof(int)];

Why do you need an array of pointer to Item?

free_lst(items, sizeof(arr)/sizeof(int));

One is freeing the array of elements that haven't been initialized. I think you are seeing this behaviour from valgrind because free is being called on uninitialized memory.

printf('%p\n',tmp->next);

See when should I use single quotes? One assumes that the list is not empty and prints the second element?

Intent

One is trying to push an item on the linked-list, an operation that is efficient, but the code is unnecessarily complex. Also, when one pushes a dynamically allocated object, it makes sense to pop to free the object. I've modified one's code with error checking (assuming POSIX-like compliance) and the changes listed above.

#include <stdlib.h>
#include <stdio.h>

struct Item
{
    int num ;
    struct Item *next;
};

int main(void)
{
    int arr[] = {1,2,3,4,5};
    struct Item * head = 0, *cursor;
    /* `size_t` stddef, included by stdlib and stdio */
    size_t i = sizeof arr / sizeof *arr;
    int success = EXIT_SUCCESS; /* stdlib */
    while(i) { /* Push backwards. */
        struct Item *tmp;
        if(!(tmp = malloc(sizeof *tmp))) goto catch; /* stdlib */
        tmp->num = arr[--i];
        tmp->next = head;
        head = tmp;
    }
    for(cursor = head; cursor; cursor = cursor->next)
        printf("%d\n", cursor->num); /* stdio */
    goto finally;
catch:
    success = EXIT_FAILURE; /* stdlib */
    perror("to list"); /* stdio */
finally:
    while(head) { /* Pop off linked-list. */
        struct Item *const prev = head;
        head = head->next;
        free(prev); /* stdlib */
    }
    return success;
}