Why does free()-ing my memory malloc()-ed in another function cause a crash?

Question

I have a linked list of structures and functions to add, remove, and create data from/in it. Now the problem arises when I try to free() data that I have allocated in the create function. The program works fine if I remove the part of my code where I free my memory, but then I do not free the memory.

The struct:

typedef struct carinfo_t
{
    char * brand;
    char * model;
    int year;
    float value;
    struct carinfo_t * next;
} carinfo_t;

The function where I create the items that are to be added in the list:

struct carinfo_t *createCarinfo(char *brand, char *model, int year, float value)
{
    carinfo_t *newInfo = (carinfo_t *) malloc (sizeof(newInfo));
    newInfo->brand = malloc (sizeof(brand));
    newInfo->model = malloc (sizeof(model));

    if (!newInfo)
    {
        printf("createCarinfo: error: no space left\n");
    }
    else
    {
        strcpy(newInfo->brand, brand);
        strcpy(newInfo->model, model);
        newInfo->year = year;
        newInfo->value = value;
        newInfo->next = NULL;
    }
    return newInfo;
}

The free function:

void freeCarinfo(struct carinfo_t *carinfo)
{
    if(carinfo->brand != 0)
    {
        free(carinfo->brand);
        carinfo->brand = 0;
    }
    if(carinfo->model != 0)
    {
        free(carinfo->model);
        carinfo->model = 0;
    }
    if(carinfo != 0)
    {
       free(carinfo);
       carinfo = 0;
    }
}

And freeCarinfo() is called by my removeCarinfo() function which basically makes sure that all the items in the queue are then properly matched when I remove the items that I seek.

What I want to do now, is to be able to free the memory that I have given the program with

carinfo_t *newInfo = (carinfo_t *) malloc (sizeof(newInfo));
newInfo->brand = malloc (sizeof(brand));
newInfo->model = malloc (sizeof(model));

Answer 1

Firstly, when you're allocating space for data, you're making a common mistake:

newInfo->brand = malloc (sizeof(brand));

This allocates enough space to store a pointer to char, not the data itself. Then you try to copy data into it:

strcpy(newInfo->brand, brand);

which may well write more data in than you have created space for.

What you need is to create enough space for the whole string, plus the \0 end-of-string marker:

newInfo->brand = malloc (strlen(brand) + 1);

And you'll want this as well somewhere:

#include <string.h>

Secondly, when you compare a pointer to 0, the traditional C way is to use NULL rather than 0 (or not at all). It's (mostly) the same effect, but it's clearer that you're doing pointer comparisons.

Thirdly, in freeCarinfo you use carinfo and then test whether it is NULL (0). You really, really need to know whether the function can be called with the value NULL in which case you most not use carinfo->brand, or whether it is definitely not NULL in which case you don't need the last test.

Fourthly, as mentioned by others elsewhere, you're allocating newInfo in a way which doesn't give it the right amount of space:

carinfo_t *newInfo = (carinfo_t *) malloc (sizeof(newInfo));

What you probably need instead is:

carinfo_t *newInfo = malloc (sizeof(carinfo_t));

or even better:

carinfo_t *newInfo = malloc (sizeof(*newInfo));

In summary, there's lots and lots wrong here, which is why you're finding it so frustrating. Have a reread about pointers and dynamic allocation, and have another look at your code based on what you're learned.