CODEWITHSUNDEEP
cmallocfreevalgrind
ant2009
ant2009

Reputation: 22486

cannot free memory

gcc 4.4.4 c89

I have the following function but I cannot free the memory. The message I get in Valgrind is suspecting the getline function. However, I am free the file pointer at the end of the function. So it cannot be that.

I have a global array of pointers to char 'candidate_names'. However, I haven't allocated any memory for it.

Many thanks for any advice,

The message I get in valgrind is the following. 

HEAP SUMMARY:
==4021==     in use at exit: 840 bytes in 7 blocks
==4021==   total heap usage: 22 allocs, 15 frees, 1,332 bytes allocated
==4021== 
==4021== Searching for pointers to 7 not-freed blocks
==4021== Checked 48,412 bytes
==4021== 
==4021== 840 bytes in 7 blocks are still reachable in loss record 1 of 1
==4021==    at 0x4005BDC: malloc (vg_replace_malloc.c:195)
==4021==    by 0xAAE38D: getdelim (iogetdelim.c:68)
==4021==    by 0xAAADD2: getline (getline.c:34)
==4021==    by 0x804892B: load_candidates (candidate.c:61)
==4021==    by 0x8048686: main (driver.c:24)

My source code:

#define NUMBER_OF_CANDIDATES 7
static char *candidate_names[NAME_SIZE] = {0};

int load_candidates()
{
    FILE *fp = NULL;
    size_t i = 0;
    ssize_t read = 0;
    size_t n = 0;
    char *found = NULL;

    fp = fopen("votes.txt", "r");

    /* open the votes file */
    if(fp == NULL) {
        fprintf(stderr, "Cannot open votes file [ %s ]\n", strerror(errno));
        return FALSE;
    }

    /* fill the structure with candidates */
    for(i = 0; i < NUMBER_OF_CANDIDATES; ) {
        read = getline(&candidate_names[i], &n ,fp);
        if(read == -1) {
            fprintf(stderr, "Cannot read candidate [ %d ] [ %s ]\n",
                    i, strerror(errno));
            candidate_names[i] = "Invalid candidate";
            i++;
            continue;
        }
        /* Just ignore the key work in the text file */
        if(strcmp("CANDIDATES\n", candidate_names[i]) != 0) {
            /* Find and remove the carriage return */
            found = strchr(candidate_names[i], '\n');
            if(found != NULL) {
                /* Remove the carriage return by nul terminating */
                *found = '\0';
            }
            i++;
        }
    }

    fclose(fp);

    return TRUE;
}

EDIT ========= FREEING candidate_names ======

All heap blocks were freed -- no leaks are possible
==4364== 
==4364== ERROR SUMMARY: 84 errors from 2 contexts (suppressed: 12 from 8)
==4364== 
==4364== 42 errors in context 1 of 2:
==4364== Invalid free() / delete / delete[]
==4364==    at 0x40057F6: free (vg_replace_malloc.c:325)
==4364==    by 0x8048A95: destroy_candidate (candidate.c:107)
==4364==    by 0x8048752: main (driver.c:44)
==4364==  Address 0x401e1b8 is 0 bytes inside a block of size 120 free'd
==4364==    at 0x40057F6: free (vg_replace_malloc.c:325)
==4364==    by 0x8048A95: destroy_candidate (candidate.c:107)
==4364==    by 0x8048752: main (driver.c:44)
==4364== 
==4364== 
==4364== 42 errors in context 2 of 2:
==4364== Invalid read of size 1
==4364==    at 0x400730E: strcmp (mc_replace_strmem.c:426)
==4364==    by 0x8048A7F: destroy_candidate (candidate.c:106)
==4364==    by 0x8048752: main (driver.c:44)
==4364==  Address 0x401e1b8 is 0 bytes inside a block of size 120 free'd
==4364==    at 0x40057F6: free (vg_replace_malloc.c:325)
==4364==    by 0x8048A95: destroy_candidate (candidate.c:107)
==4364==    by 0x8048752: main (driver.c:44)


void destroy_candidate()
{
    size_t i = 0;
    for(i = 0; i < NUMBER_OF_CANDIDATES; i++) {
        if(strcmp(candidate_names[i], "Invalid candidate") != 0) {
            free(candidate_names[i]);
        }
    }
}

EDIT with code from main.c =====================

typedef struct Candidate_data_t {
    size_t candidate_data_id;
    Candidates_t *candidate;
} Candidate_data;

static Candidate_data* create_candidate_data(Candidates_t *candidate, size_t i);
static void destroy_candidata_data(Candidate_data *cand_data);

int main(void)
{
    Candidates_t *candidate = NULL;
    Candidate_data *cand_data[NUMBER_OF_CANDIDATES] = {0};
    size_t i = 0;

    load_candidates();

    for(i = 0; i < NUMBER_OF_CANDIDATES; i++) {
         candidate = create_candidates(i);
         if(candidate == NULL) {
             fprintf(stderr, "Cannot failed to initalize candidate [ %d ]\n", i);
         }

         /* Create array of candidates */
         cand_data[i] = create_candidate_data(candidate, i);
         fill_candidates(cand_data[i]->candidate);
    }

    /* Display each candidate */
    for(i = 0; i < NUMBER_OF_CANDIDATES; i++) {
        display_candidate(cand_data[i]->candidate);
        printf("\n");
    }

    for(i = 0; i < NUMBER_OF_CANDIDATES; i++) {
        destroy_candidata_data(cand_data[i]);
    }

    return 0;
}

static Candidate_data* create_candidate_data(Candidates_t *candidate, size_t id)
{
    Candidate_data *cand_data = NULL;

    cand_data = malloc(sizeof *cand_data);

    if(cand_data == NULL) {
        fprintf(stderr, "Failed to allocate memory [ %s ]\n",
                strerror(errno));

        return NULL;
    }
    cand_data->candidate_data_id = id;
    cand_data->candidate = candidate;

    return cand_data;
}

static void destroy_candidata_data(Candidate_data *cand_data)
{
    destroy_candidate(cand_data->candidate);
    free(cand_data);
}

Upvotes: 5

Views: 4411

Answers (7)

Andy Johnson
Andy Johnson

Reputation: 8149

You are allocating memory inside getline(). You are never freeing that memory. This is what valgrind is telling you: that you have seven (== NUMBER_OF_CANDIDATES) blocks that you have not freed.

Closing the file pointer does not free the memory that getline() allocated.

You need to do something like this at the end of load_candidates():

for(int i = 0; i < NUMBER_OF_CANDIDATES; i++)
{
    free(candidate_names[i]);
}

EDIT

In your edit you are freeing null pointers. Try:

void destroy_candidate()
{
    size_t i = 0;
    for(i = 0; i < NUMBER_OF_CANDIDATES; i++) {
        if ( (candidate_names[i] != NULL) && (strcmp(candidate_names[i], "Invalid candidate") != 0) ){
            free(candidate_names[i]);
        }
    }
}

Upvotes: 1

pmg
pmg

Reputation: 108967

What is candidate_names? It's an array of pointers.
When you do

candidate_names[i] = "Invalid candidate";

you assign the pointer to a string literal. Maybe later in the program you want to free it. That's a NO-NO!

In any case, the previous value of candidate_names[i] is lost. If the value was not NULL, you just leaked some memory.

Upvotes: 6

Karl Bielefeldt
Karl Bielefeldt

Reputation: 48998

getline allocates memory which you store in your candidate_names array of pointers. Those pointers are not getting freed. When you are done with them, you should call:

for(i = 0; i < NUMBER_OF_CANDIDATES; i++)
{
    if (strcmp(candidate_names[i], "Invalid candidate") != 0)
        free(candidate_names[i]);
}

Additionally, that array should be declared as:

static char *candidate_names[NUMBER_OF_CANDIDATES];

And right before your getline you need:

candidate_names[i] = NULL;

NAME_SIZE isn't needed because that memory is dynamically allocated, unless you are using it elsewhere for input validation or something.

Upvotes: 2

Gregory Pakosz
Gregory Pakosz

Reputation: 70204

Have a look at the getline() man page.

If *lineptr is NULL, then getline() will allocate a buffer for storing the line, which should be freed by the user program. (In this case, the value in *n is ignored.)

At the end of your program, you need to loop over your candidate_names array and call free on non NULL entries but in that case you must not do candidate_names[i] = "Invalid candidate";as @pmg pointed in his answer as you would try to free a string literal.

Pay also attention to:

Alternatively, before calling getline(), *lineptr can contain a pointer to a malloc(3)-allocated buffer *n bytes in size. If the buffer is not large enough to hold the line, getline() resizes it with realloc(3), updating *lineptr and *n as necessary.

In either case, on a successful call, *lineptr and *n will be updated to reflect the buffer address and allocated size respectively.

Upvotes: 7

Jens Gustedt
Jens Gustedt

Reputation: 78903

I see that you have two different macros NUMBER_OF_CANDIDATES and NAME_SIZE. Looks like trouble.

Upvotes: 1

Novikov
Novikov

Reputation: 4489

I don't think this is correct.

getline(&candidate_names[i], &n ,fp);

No reason to pass a pointer to an integer.

Upvotes: -2

bdonlan
bdonlan

Reputation: 231063

getline() allocates space for the line it just read, calling malloc() for you behind the scenes. You store these line buffers in the candidate_names array, but never free it. The leak isn't the file pointer - you free that just fine. It's the lines you read from the file, which need to be freed elsewhere, when you're done using them.

Upvotes: 4

Related Questions