malloc char* inside a function, valgrind reports memory leak

Question

I am pretty new to C, and am doing toy programs to learn it. The following code compiles, outputs correctly, but Valgrind reports a memory leak:

#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include "graph.h"

void add_vertex(vertex *v_p, char *name) {
    if(strlen(name) == 0) { 
        printf("error");
    }
    v_p->name = (char *)malloc(strlen(name) + 1);
    if(v_p->name == NULL) {
        printf("error");
    }
    strcpy(v_p->name, name);
    v_p->cluster = -1;
    v_p->deleted = 0;
    printf("added vertex.\n");
}

void free_vertex(vertex *ver) {
    if(ver->name) {free(ver->name);printf("free'd name\n");}
    free(ver);

}

int main() {
    int Nu = 0;
    vertex *arr = (vertex *)malloc(2 * sizeof(vertex));
    vertex *_temp;
    int i =0;
    add_vertex(arr, "Hello");
    add_vertex(arr+1, "World");
    _temp = (vertex *)realloc(arr, 4*sizeof(vertex));
    printf("reallocated\n");
    if (_temp != NULL) {
        arr = _temp;
        add_vertex(arr+2, "this");
        add_vertex(arr +3, "worked");
        Nu=4;
    }
    else{
        printf("FAIL\n");
        Nu=2;
    }
    for (; i <Nu; i++) {
        printf("%s\n",(arr+i)->name);
    }
    for (; i <Nu; i++) {
       free_vertex(arr+i);
    }
    free(arr);
    return 0;
}

The vertex is coded in a header file,

typedef struct vertex_t
{
    char*   name;
    int     cluster;
    int    deleted;
}vertex

The output is:

added vertex.
added vertex.
reallocated
added vertex.
added vertex.
Hello
World
this
worked

It doesn't print "free'd memory", so where was it free'd? here is what Valgrind had to say about it:

==1436== HEAP SUMMARY:
==1436==     in use at exit: 24 bytes in 4 blocks
==1436==   total heap usage: 6 allocs, 2 frees, 120 bytes allocated
==1436== 
==1436== 5 bytes in 1 blocks are definitely lost in loss record 1 of 4
==1436==    at 0x4C2893D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1436==    by 0x4006C3: add_vertex (graph.c:10)
==1436==    by 0x4007EF: main (graph.c:37)
==1436== 
==1436== 6 bytes in 1 blocks are definitely lost in loss record 2 of 4
==1436==    at 0x4C2893D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1436==    by 0x4006C3: add_vertex (graph.c:10)
==1436==    by 0x400797: main (graph.c:31)
==1436== 
==1436== 6 bytes in 1 blocks are definitely lost in loss record 3 of 4
==1436==    at 0x4C2893D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1436==    by 0x4006C3: add_vertex (graph.c:10)
==1436==    by 0x4007AC: main (graph.c:32)
==1436== 
==1436== 7 bytes in 1 blocks are definitely lost in loss record 4 of 4
==1436==    at 0x4C2893D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1436==    by 0x4006C3: add_vertex (graph.c:10)
==1436==    by 0x400804: main (graph.c:38)
==1436== 
==1436== LEAK SUMMARY:
==1436==    definitely lost: 24 bytes in 4 blocks
==1436==    indirectly lost: 0 bytes in 0 blocks
==1436==      possibly lost: 0 bytes in 0 blocks
==1436==    still reachable: 0 bytes in 0 blocks
==1436==         suppressed: 0 bytes in 0 blocks

What is the problem with the code? Should I allocate the name of the vertex differently? Thanks for the help!

EDIT: Thanks to all of you! I've fixed the code, now I am not freeing individual elements, i.e.

void free_vertex(vertex *ver) {
    if(ver->name) {free(ver->name);printf("free'd name\n");}
}

and of course re-setting the i to 0. can't believe I overlooked it. Thanks a lot!

Francis Upton IV · Accepted Answer · 2011-12-24T11:49:03.620

5

You are not resetting i in the loop that frees the code. Those loops should look like this:

for (i = 0; i <Nu; i++) {
    printf("%s\n",(arr+i)->name);
}
for (i = 0; i <Nu; i++) {
   free_vertex(arr+i);
}

Added another point:

free_vertex() should not have the:

free(ver)

line, which is what others are saying.

free_vertex() and add_vertex() should be parallel in the sense that free frees only what was allocated in add.

edited Dec 24 '11 at 11:49

answered Dec 24 '11 at 11:41

Francis Upton IV

19,322
3
53
57

2

That's true - but fixing that will break malloc badly - as the program will be freeing things that weren't malloced. – Douglas Leeder Dec 24 '11 at 11:47

score 4 · Answer 2 · answered Dec 24 '11 at 11:42

4

The realloc is irrelevant here. You're doing this:

arr = malloc(4 * sizeof(vertex));
for (int i = 0; i < 4; ++i) {
    free(arr[i]);
}
free(arr);

You shouldn't be freeing single elements of the array.

answered Dec 24 '11 at 11:42

2

Francis's answer is correct, but this is a second bug lurking. – Dec 24 '11 at 11:43

score 2 · Answer 3 · edited May 23 '17 at 11:55

You can't free any pointer other than the exact same one you get back from malloc, realloc, etc. That's undefined behavior, and you're asking for heap corruption or worse if you do it.

(Fun part: Since arr+0 is equal to arr, once you reinit i correctly as mentioned in Francis's answer, the first iteration through the loop frees the whole array. From then on, you're iterating over an array that no longer exists, which is undefined behavior in itself.)

score 2 · Answer 4 · answered Dec 24 '11 at 11:44

2

You have to malloc and free the same things!

You are malloc and reallocing an array, so you must free the array. You are freeing the individual vertexes.

answered Dec 24 '11 at 11:44

Douglas Leeder

52,368
9
94
137

score 2 · Answer 5 · answered Dec 24 '11 at 11:45

2

The problem could be because of the below code where i is not initialized back to zero!

for (; i <Nu; i++) {
    printf("%s\n",(arr+i)->name);
}
for (; i <Nu; i++) {
   free_vertex(arr+i);
}

So it should be

for (; i <Nu; i++) {
    printf("%s\n",(arr+i)->name);
}
for (i=0 ; i <Nu; i++) {
   free_vertex(arr+i);
}

answered Dec 24 '11 at 11:45

Sangeeth Saravanaraj

16,027
21
69
98

@cHao That's a good point! I like the "it should be thrown away" part!! :) – Sangeeth Saravanaraj Dec 24 '11 at 12:15

malloc char* inside a function, valgrind reports memory leak

5 Answers5