CODEWITHSUNDEEP
carraysmemory-management
BMBM
BMBM

Reputation: 16033

double free when freeing structure in array

I'm reading a file line by line (the file only contains one line for testing) and I'm creating a struct for every line, and adding that struct to a predefined array.

#define _GNU_SOURCE

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


typedef struct {
    int age;
    int weight;
    char *name;
} Person;


int person_create(Person **person, unsigned int age, unsigned int weight, char *name)
{   
    *person = malloc(sizeof(Person));

    if (person == NULL) {
        return 0;
    }

    return 1;
}

void person_free(Person *person)
{
    free(person);

    return;
}

int main(void)
{
    FILE *input_file = NULL;

    input_file = fopen("names.txt", "r");
    assert(input_file != NULL);

    char *line = NULL;
    size_t _ = 0;
    ssize_t line_len = 0;
    Person persons[1] = {};
    int line_num = 0;

    while ((line_len = getline(&line, &_, input_file)) != -1) {
        if (line[line_len - 1] == '\n') {
            line[line_len - 1] = '\0';  
        }

        Person *person = NULL;
        person_create(&person, line_num, 2, line);

        persons[line_num] = *person;

        line_num++;        
    }

    free(line);

    printf("lines read %d\n", line_num);

    for (int i = 0; i < 1; i++) {
        person_free(&persons[i]);
    }

    return 0;
}

I stripped the program down as far as I could, but upon free'ing the array entry, I get an error

*** Error in `./prog': double free or corruption (out): 0x00007fff7ace9f10 ***
Aborted (core dumped)

If I leave out the call to free_person then valgrind reports lost memory.

I'm pretty sure it has to do with how I assign the person for each line to the array

    Person *person = NULL;
    person_create(&person, line_num, 2, line);

    persons[line_num] = *person;

But I can't seem to understand what exactly is going wrong.

Upvotes: 1

Views: 584

Answers (3)

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 727097

Your program has some serious undefined behavior: once line_num gets above zero, you are writing into memory outside persons[] array. You need to allocate enough elements, and also make it an array of pointers:

Person *persons[100]; // or some other MAX

Once you make persons[] an array of pointers, it becomes clear why free(&persons[i]) and persons[line_num] = *person are incorrect (the compiler should issue a warning for the assignment).

In addition, this check of malloc result is incorrect:

if (person == NULL) {
    return 0;
}

You should check *person, not person, because person is a double pointer.

Upvotes: 2

unwind
unwind

Reputation: 400159

You're losing the malloc()ed memory, since you copy it into your array of actual structures (persons). Your code doesn't need to use malloc(), and certainly not free() like it does.

It would make more sense to have an array of pointers:

Person *persons[10];

Then have the function that calls malloc() return the newly allocated memory, so you can do persons[line_num] = person_create(line_num, 2, line);. Then you need to go through and free() them all.

Upvotes: 1

Sourav Ghosh
Sourav Ghosh

Reputation: 134396

Your code shows undefined behaviour. You've decalred

 Person persons[1] = {};

and later, you're using

persons[line_num] = *person;

which causes out of bound memory access, which in turn invokes undefined behaviour.

Upvotes: 1

Related Questions