Does this C code leak memory?

Question

So here is my code:

typedef struct {
    int x;
    int *t;
}vector;

vector fct (vector v){
    vector w;
    int i;
    w.x=v.x;
    w.t=malloc(w.x *sizeof(int));
    for (i=0; i<w.x; i++)
        w.t[i]=2*v.t[i];
    return w;
}


int main (){
    int i;
    vector v;
    v.x=2;
    v.t=malloc(2*sizeof(int));
    v.t[0]=1; v.t[1]=5;
    v=fct(v);
    for (i=0; i<2; i++)
        printf("%d \t",v.t[i]);
    puts("");
    free(v.t);
    return 0;
}

I'm quite worried about whether or not it causes a memory leak, and how I can fix that in case it does. Oh, and I know that if I define another vector let's say w, such that w = fct(v) it would clear the problem, but I need a different method, one that would work even if the function is returned to the original vector.

Answer 1

v.t=malloc(2*sizeof(int));

Here you assign allocated memory to v.t.

v=fct(v);

Then here you overwrite all fields of v with what was returned from fct. This discards the old value of v.t, causing a memory leak.

The call to free at the end of main is freeing the memory that was allocated inside of fct.

You can fix this leak by saving v.t and calling free on the saved pointer:

vector v;
int *vt_sav;
v.x=2;
v.t=malloc(2*sizeof(int));
vt_sav = v.t;
v.t[0]=1; v.t[1]=5;
v=fct(v);
free(vt_sav);

Answer 2

Sure it does. vector t is allocated twice, and freed once. The whole architecture looks problematic.

Answer 3

Yes, you are leaking memory. w.t is never released.
Also pass the structures via pointers not by value. Returning vector is also expensive.

Your new program can look like this:

#include <stdio.h>


typedef struct {
    int x;
    int *t;
}vector;

void fct (vector *v, vector * w){

    int i;

    w->x = v->x;

    for (i=0; i<w->x; i++)
        w->t[i]=2*v->t[i];
}


int main (){
    int i;
    vector v;
    vector w;

    v.x=2;

    v.t = malloc(2*sizeof(int));
    w.t = malloc(2*sizeof(int));

    v.t[0]=1;
    v.t[1]=5;

    fct(&v,&w);

    for (i=0; i<2; i++)
        printf("%d \t",w.t[i]);


    puts("");

    free(v.t);
    free(w.t);

    return 0;
}

OUTPUT:

2       10

Answer 4

I'm quite worried about whether or not it causes a memory leak, and how I can fix that in case it does.

It does. In main(), you allocate memory and point v.t to it; then you overwrite that pointer with one from the return value of fct(). The original pointer is lost, and you've no other copy of it, so the pointed-to memory is leaked.

Oh, and I know that if I define another vector let's say w, such that w = fct(v) it would clear the problem, but I need a different method, one that would work even if the function is returned to the original vector.

With every dynamic memory allocation comes an obligation to free that memory. You can consider that obligation to be associated with exactly one copy of the pointer to that memory, as a kind of notional tag. You can freely (notionally) transfer the tag to a different copy of the pointer. If the copy then bearing the tag ever goes out of scope or is overwritten with a different value, however, then you leak the pointed-to memory.

Consider how that applies to your case:

1. You allocate memory and assign a pointer to v.t. This is initially the only copy of the pointer to the allocated memory, so the obligation to free that memory is associated with it.

2. You pass a copy of v, including a copy of v.t, to function fct(). You have two alternatives: the obligation to free can stay with the original copy of v.t, or it can be transferred to the copy received by the function. That is an inherent characteristic of the definition of fct(), and should be documented with it -- you cannot choose on a call-by-call basis. Since fct() does not free the pointer, you have implicitly chosen for the obligation to free to remain with the original copy.

3. fct() performs its own allocation and returns a copy of the resulting pointer; that's the only copy that survives the execution of the function, so the obligation to free must go with it.

4. By overwriting the original v.t with a different value while it holds an obligation to free, you leak the original memory. There is still an obligation to free the (new) value of v.t, but there is no longer any way to access the old dynamically-allocated memory.

Now you had one option in there: whether to transfer the obligation to free as part of the function call. Suppose that you did transfer it; in that case, fct() must not fail to perform that free() before it returns. In that case, however, main() must not use the original value of v.t after calling fct(), whether it overwrites v with that function's result or not.

So you have a choice: either fct() takes responsibility for freeing v.t (which it could pass on to another function if it wished, or which it could return to the caller via its return value), or fct() does not take that responsibility. Its callers need to behave accordingly. You cannot have it both ways.