why 50% of allocated memory is not freed?

[MN]

Hi there,
I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.


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

const int max = 15;
const int s = 2;

int* table(int max)
{
int i = 0;
int* table = NULL;

if (table != NULL)
free(table);

table = malloc (sizeof(int*)* max);

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}
return (table);
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

for (i = 0; i< max; ++i)
strcpy((char*)&test[0], "22");

return (test);
}


int main ()
{
int* table_tmp = NULL;
char** test_tmp = NULL;

int j = 0;

if (table_tmp != NULL)
free(table_tmp);

table_tmp = malloc (sizeof (int*) * max);
table_tmp = table (max);

for (j = 0; j < max; ++j)
{
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
}
free (table_tmp);

printf("\n");

test_tmp = malloc (sizeof (char**) * max);
for(j = 0; j < max; ++j)
test_tmp[j] = malloc (sizeof (char*) * s+1);

test_tmp = test_foo(max, s);

for (j = 0; j < max; ++j)
{
printf("test_tmp [%d] = %s\n", j, (char*)&test_tmp[j][0]);
}

for (j = 0; j < max; ++j)
{
free (test_tmp[j]);
}
free (test_tmp);

return 0;
}

I'm using valgrind tool to detect memory leak, like this:
valgrind --tool=memcheck --leak-check=full --leak-resolution=high --
num-callers=20 ./free_variable&> dump; gedit dump

as result:


==28879== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 5 from
1)
==28879== malloc/free: in use at exit: 495 bytes in 17 blocks.
==28879== malloc/free: 34 allocs, 17 frees, 990 bytes allocated.
==28879== For counts of detected errors, rerun with: -v
==28879== searching for pointers to 17 not-freed blocks.
==28879== checked 65,528 bytes.
==28879==
==28879== 120 bytes in 1 blocks are definitely lost in loss record 1
of 3
==28879== at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879== by 0x40071F: main (test_free_variable.c:60)
==28879==
==28879==
==28879== 375 (120 direct, 255 indirect) bytes in 1 blocks are
definitely lost in loss record 2 of 3
==28879== at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879== by 0x400793: main (test_free_variable.c:71)
==28879==
==28879== LEAK SUMMARY:
==28879== definitely lost: 240 bytes in 2 blocks.
==28879== indirectly lost: 255 bytes in 15 blocks.
==28879== possibly lost: 0 bytes in 0 blocks.
==28879== still reachable: 0 bytes in 0 blocks.
==28879== suppressed: 0 bytes in 0 blocks.
==28879== Reachable blocks (those to which a pointer was found) are
not shown.
==28879== To see them, rerun with: --show-reachable=yes

 

[Joachim Schmitz]

Hi there,
I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.


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

const int max = 15;
const int s = 2;

int* table(int max)
{
int i = 0;
int* table = NULL;

if (table != NULL)
free(table);

Dead code. You just initialized it to NULL, so how should it have a differnt
value?
Unless you mant ti hav it as
static int*table = NULL

table = malloc (sizeof(int*)* max);

Better:
table = malloc (sizeof(table)* max);

Also you'd better check for success/failure here to not dereference NULL a
few lines down

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}
return (table);
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

Why the casts?

for (i = 0; i< max; ++i)
strcpy((char*)&test[0], "22");

return (test);
}


int main ()
{
int* table_tmp = NULL;
char** test_tmp = NULL;

int j = 0;

if (table_tmp != NULL)
free(table_tmp);

Dead code again

table_tmp = malloc (sizeof (int*) * max);
table_tmp = table (max);

here you leak memory allocated in main()

for (j = 0; j < max; ++j)
{
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
}
free (table_tmp);

here you free mem from table(), but lost access to the mem from main()

printf("\n");

test_tmp = malloc (sizeof (char**) * max);
for(j = 0; j < max; ++j)
test_tmp[j] = malloc (sizeof (char*) * s+1);

test_tmp = test_foo(max, s);

here you leak the merory allocated in main()

for (j = 0; j < max; ++j)
{
printf("test_tmp [%d] = %s\n", j, (char*)&test_tmp[j][0]);
}

for (j = 0; j < max; ++j)
{
free (test_tmp[j]);
}
free (test_tmp);

here you free memory allocated in test_foo(). But you already lost access to
the one allocatted in main()

 

[sathya]

Hi there,
I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.

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

const int max = 15;
const int s = 2;

int* table(int max)
{
        int i = 0;
        int* table = NULL;

        if (table != NULL)
                free(table);

        table = malloc (sizeof(int*)* max);

        for (i = 0; i < max; i++)
        {
                if (i == 0)
                        table = 0;
                else
                {
                        if (i == 1)
                                table= 1;
                        else
                                table = table[i-1]*2;
                }
        }
        return (table);

}

char** test_foo(int max, int s)
{
        int i = 0;
        char** test = NULL;
        test = (char**)malloc(sizeof(char**) * max);

        for (i = 0; i< max; ++i)
                test = (char*)malloc(sizeof(char*) * s+1);

        for (i = 0; i< max; ++i)
                strcpy((char*)&test[0], "22");

        return (test);

}

int main ()
{
        int* table_tmp = NULL;
        char** test_tmp = NULL;

        int j = 0;

        if (table_tmp != NULL)
                free(table_tmp);

        table_tmp = malloc (sizeof (int*) * max);
        table_tmp = table (max);

        for (j = 0; j < max; ++j)
        {
                printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
        }
        free (table_tmp);

        printf("\n");

        test_tmp = malloc (sizeof (char**) * max);
        for(j = 0; j < max; ++j)
                test_tmp[j] = malloc (sizeof (char*) * s+1);

        test_tmp = test_foo(max, s);

        for (j = 0; j < max; ++j)
        {
                printf("test_tmp [%d] = %s\n", j, (char*)&test_tmp[j][0]);
        }

        for (j = 0; j < max; ++j)
        {
                free (test_tmp[j]);
        }
        free (test_tmp);

        return 0;

}

I'm using valgrind tool to detect memory leak, like this:
valgrind --tool=memcheck --leak-check=full --leak-resolution=high --
num-callers=20 ./free_variable&> dump; gedit dump

as result:

==28879== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 5 from
1)
==28879== malloc/free: in use at exit: 495 bytes in 17 blocks.
==28879== malloc/free: 34 allocs, 17 frees, 990 bytes allocated.
==28879== For counts of detected errors, rerun with: -v
==28879== searching for pointers to 17 not-freed blocks.
==28879== checked 65,528 bytes.
==28879==
==28879== 120 bytes in 1 blocks are definitely lost in loss record 1
of 3
==28879==    at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879==    by 0x40071F: main (test_free_variable.c:60)
==28879==
==28879==
==28879== 375 (120 direct, 255 indirect) bytes in 1 blocks are
definitely lost in loss record 2 of 3
==28879==    at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879==    by 0x400793: main (test_free_variable.c:71)
==28879==
==28879== LEAK SUMMARY:
==28879==    definitely lost: 240 bytes in 2 blocks.
==28879==    indirectly lost: 255 bytes in 15 blocks.
==28879==      possibly lost: 0 bytes in 0 blocks.
==28879==    still reachable: 0 bytes in 0 blocks.
==28879==         suppressed: 0 bytes in 0 blocks.
==28879== Reachable blocks (those to which a pointer was found) are
not shown.
==28879== To see them, rerun with: --show-reachable=yes

Hi

you are free'ing the memory which is allocated in the function
1. table(int max)
2. test_foo()
but not free'ing the Memory allocated in the function main()


Thanks
Sathya

 

[Jens Thoms Toerring]

I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.

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

const int max = 15;
const int s = 2;

int* table(int max)

Having an argument with the same name as a global variable
is rather bad style (and it hides the global variable).

{
int i = 0;
int* table = NULL;

Having a variable with the same name as a function is at least
bad style.

if (table != NULL)
free(table);

What's that for? One line uo you initialized 'table' to NULL.

table = malloc (sizeof(int*)* max);

You allocate int pointers here instead of ints and use that
memory afterwards as if it would be made for ints.

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}
return (table);

Just a hint: 'return' is not a function, so the parentheses
are unnecessary.

}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

Same problem as everywhere else, the pointer you assign the
address returned by malloc() to doesn't fit the type of the
memory you allocated. And drop the cast of the return value
of malloc() - it won't buy you anything and might bite you

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

The same problem again. And another one is that the '*' operator
binds stronger that '+' (as you learn in primary school;-) I
guess you meant

test = malloc(sizeof(char) * (s+1));

for (i = 0; i< max; ++i)
strcpy((char*)&test[0], "22");

Wouldn't
strcpy( test, "22" );

be a bit easier to read? It does the same. And having to use
casts is often a sign that there's a problem that hasn't been
understood and instead the compiler has been gagged to keep
it from pointing out the problem...

return (test);
}

int main ()

Better:

int main( void )

{
int* table_tmp = NULL;
char** test_tmp = NULL;

int j = 0;

if (table_tmp != NULL)
free(table_tmp);

table_tmp = malloc (sizeof (int*) * max);

Same problem as above, 'table_tmp' is a pointer to int
and then you allocate memory for a number of int pointers
and assign that to 'table_tmp'. But for that to work
correctly 'table_tmp' would have to have type pointer to
pointer to int.

table_tmp = table (max);

And here you throw away the memory you just allocated without
freeing it since you overwrite the pointer you got from malloc().

for (j = 0; j < max; ++j)
{
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
}
free (table_tmp);

Here you only free the memory that was allocated in
function table().

printf("\n");

test_tmp = malloc (sizeof (char**) * max);

Obviously you want to allocate an array of char pointers
here, but you allocate memory for pointer to pointer to
char. Avoid such mistakes by using

test_tmp = malloc (max * sizeof *test_tmp);

for(j = 0; j < max; ++j)
test_tmp[j] = malloc (sizeof (char*) * s+1);

You obviously want to allocate arrays of chars but you're
actually allocate an arrray of char pointers.

test_tmp = test_foo(max, s);

And here you forget what 'test_tmp' was pointing to (and
what the pointers stored in 'test_tmp' were pointing to,
too), that's another place where you can't free the memory
you allocated anymore.

for (j = 0; j < max; ++j)
{
printf("test_tmp [%d] = %s\n", j, (char*)&test_tmp[j][0]);
}

for (j = 0; j < max; ++j)
{
free (test_tmp[j]);
}
free (test_tmp);

Ad here you only free the memory allocated in test_foo().

return 0;
}

I'm using valgrind tool to detect memory leak, like this:
valgrind --tool=memcheck --leak-check=full --leak-resolution=high --
num-callers=20 ./free_variable&> dump; gedit dump

You don't need to use valgrind to see that your program is
leaking memory like a sinking ship;-) Rule: If you obtained
memory from malloc() and friends never ever forget that
value before you have free'ed the memory again.

Regards, Jens

 

[Ben Bacarisse]

I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.


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

const int max = 15;
const int s = 2;

int* table(int max)
{
int i = 0;
int* table = NULL;

if (table != NULL)
free(table);

table = malloc (sizeof(int*)* max);

All you malloc calls have the same problem so this is as good an
example as any to pick. You are allocating using the wrong size. An
int * points to the first of many ints. I.e. you need malloc(max *
sizeof(int)) rather than sizeof(int *). There is a better way to call
malloc that avoids this problem:

exp = malloc(size * sizeof *exp);

This way you can be sure the objects are of the right size -- that of
the kind of thing that 'exp' points to. It is sometimes neater to use
exp[0] rather than *exp -- it depends on the form of exp.

Of course, in this case, int * and int may have the same size, but
that is unlikely to be true for you other allocations involving
char/char * confusions! Now, there may be other problems with the
code, but I would fix this first.

char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

test = malloc(max * sizeof *test);

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

test = malloc(s * sizeof test[0] + 1);

but here, when the size is guaranteed to be 1, I don't use this form.
I'd write simply:

test = malloc(s + 1);

for (i = 0; i< max; ++i)
strcpy((char*)&test[0], "22");

return (test);
}

int* table_tmp = NULL;
char** test_tmp = NULL;

table_tmp = malloc (sizeof (int*) * max);

table_tmp = malloc(max * sizeof *table_tmp);

and so on...

 

[Joachim Schmitz]

Dead code. You just initialized it to NULL, so how should it have a
differnt value?
Unless you mant ti hav it as

Oh boy... 'meant to have' of course, I really should read again what I write
before hitting the send button...

static int*table = NULL


Better:
table = malloc (sizeof(table)* max);

Wrong, table = malloc (sizeof(*table)* max)

Also you'd better check for success/failure here to not dereference
NULL a few lines down

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}
return (table);
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

Why the casts?

for (i = 0; i< max; ++i)
strcpy((char*)&test[0], "22");

return (test);
}


int main ()
{
int* table_tmp = NULL;
char** test_tmp = NULL;

int j = 0;

if (table_tmp != NULL)
free(table_tmp);

Dead code again

table_tmp = malloc (sizeof (int*) * max);
table_tmp = table (max);

here you leak memory allocated in main()

for (j = 0; j < max; ++j)
{
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
}
free (table_tmp);

here you free mem from table(), but lost access to the mem from main()

printf("\n");

test_tmp = malloc (sizeof (char**) * max);
for(j = 0; j < max; ++j)
test_tmp[j] = malloc (sizeof (char*) * s+1);

test_tmp = test_foo(max, s);

here you leak the merory allocated in main()

for (j = 0; j < max; ++j)
{
printf("test_tmp [%d] = %s\n", j, (char*)&test_tmp[j][0]);
}

for (j = 0; j < max; ++j)
{
free (test_tmp[j]);
}
free (test_tmp);

here you free memory allocated in test_foo(). But you already lost
access to the one allocatted in main()

return 0;
}

I'm using valgrind tool to detect memory leak, like this:
valgrind --tool=memcheck --leak-check=full --leak-resolution=high --
num-callers=20 ./free_variable&> dump; gedit dump

as result:


==28879== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 5 from
1)
==28879== malloc/free: in use at exit: 495 bytes in 17 blocks.
==28879== malloc/free: 34 allocs, 17 frees, 990 bytes allocated.
==28879== For counts of detected errors, rerun with: -v
==28879== searching for pointers to 17 not-freed blocks.
==28879== checked 65,528 bytes.
==28879==
==28879== 120 bytes in 1 blocks are definitely lost in loss record 1
of 3
==28879== at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879== by 0x40071F: main (test_free_variable.c:60)
==28879==
==28879==
==28879== 375 (120 direct, 255 indirect) bytes in 1 blocks are
definitely lost in loss record 2 of 3
==28879== at 0x4A05809: malloc (vg_replace_malloc.c:149)
==28879== by 0x400793: main (test_free_variable.c:71)
==28879==
==28879== LEAK SUMMARY:
==28879== definitely lost: 240 bytes in 2 blocks.
==28879== indirectly lost: 255 bytes in 15 blocks.
==28879== possibly lost: 0 bytes in 0 blocks.
==28879== still reachable: 0 bytes in 0 blocks.
==28879== suppressed: 0 bytes in 0 blocks.
==28879== Reachable blocks (those to which a pointer was found) are
not shown.
==28879== To see them, rerun with: --show-reachable=yes

 

[Kaz Kylheku]

Hi there,
I've done a small test for memory allocation/free and I can't
understand why I'm getting only 50 % freed.
In the next code the value of "max" and "s" can be changed during
runtime.


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

const int max = 15;
const int s = 2;

int* table(int max)
{
int i = 0;
int* table = NULL;

if (table != NULL)
free(table);

Pointless. The free function accepts null pointers. Note that in the above
statement, table is null regardless of the execution path by which the
statement is reached, so the statement is pointless.

table = malloc (sizeof(int*)* max);

The apparent intent here is to allocate a vector of pointers. If that is
the case, the pointer should be ``int **table''.

If that is not the intent, the expression should be sizeof (int) * max.

You could avoid such inconsistencies by using this idiom:

/* size derived from pointer's referenced type */
table = malloc(max * sizeof *table);

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;

Here you will run into undefined behavior (signed integer overflow) unless
the value of max is small.

Anyway, this table is useless as a lookup table, since powers of two are easy
to calculate.

Note that a compound if statement like this, where an integer value is
successively compared to some constant values, is better expressed as a switch:

switch (i) {
case 0:
table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}

Note how the redundancy in the two base cases now leaps out at you. When i is
0, the value is 0, and when i is 1, the value is 1. The switch statement lets
us condense this:

switch (i) {
case 0:
case 1:
table = i;
break;
default:
table = table[i-1] * 2;
break;
}

But of course you can replace the above switch with this:

table = (i == 0) ? 0 : 1 << (i - 1);

In either case, the behavior is undefined on overflow. If the multiplication by
two calls for a value that exceeds INT_MAX, or if the 1 << (i - 1) shift
tries to shift a 1 into the sign bit, the behavior is undefined.

}
}
return (table);
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;
test = (char**)malloc(sizeof(char**) * max);

It's pointless to initialize a variable if the next statement overwrites that
value. Such things only confuse people reading the code, because it is expected
that a value stored into a variable is live -- i.e. has a use somewhere.

Again, you have the wrong size calculation. It's a table of char * pointers.

char **test = (char **) malloc(sizeof *test * max);

for (i = 0; i< max; ++i)
test = (char*)malloc(sizeof(char*) * s+1);

Again, wrong calculation (more memory than needed). You're allocating
for an array of char, so the size is (sizeof(char)* (s + 1)). But sizeof(char)
is 1 by definition, so you just need to malloc(s + 1).

It's still a good idea to write malloc(sizeof *test * s + 1), because
it's easier to maintain if the code is changed to some other character type,
like wchar_t for internationalization.

Lastly, please take careful note that whitespace does not determine
order of operations in C arithmetic!!!

If you write

a * b+c

that does not cause the addition to be done first. It still means (a * b) + c.
The expression sizeof (T) * n + 1 does not mean ``space for an array of T, with
n + 1 elements'' it means ``space for an array of T of n elements, with 1 byte
of slack.''

Anyway, your code is too tiresome to keep commenting on further.

Go through some tutorials, read books, the c.l.c FAQ, etc.

 

[CBFalconer]

.... snip ...


All you malloc calls have the same problem so this is as good an
example as any to pick. You are allocating using the wrong size.
An int * points to the first of many ints. I.e. you need
malloc(max * sizeof(int)) rather than sizeof(int *). There is a
better way to call malloc that avoids this problem:

Read the original line again, carefully.

 

[Keith Thompson]

Read the original line again, carefully.

I think he did. So did I. table was declared as type int*. I see
nothing wrong with what Ben wrote. If you do, please explain.

 

[CBFalconer]

I think he did. So did I. table was declared as type int*. I see
nothing wrong with what Ben wrote. If you do, please explain.

I see space for an array of int* pointers being assigned. So

malloc(sizeof (int*) * max)

is confusing, but identical to:

malloc(max * sizeof (int*))

of course all question is eliminated with

table = malloc(max * sizeof *table);

 

[Keith Thompson]

I see space for an array of int* pointers being assigned. So

malloc(sizeof (int*) * max)

is confusing, but identical to:

malloc(max * sizeof (int*))

Yes, but I ask you again: what was wrong with what Ben wrote?

The original code used "sizeof(int*)", which was incorrect. Ben
changed it to "sizeof(int)", which is correct (but not ideal; see
below). Ben also swapped the operands of the multiplication, which
IMHO makes the code a bit clearer.

So how exactly was your advice that Ben should "Read the original line
again, carefully" relevant or helpful?

of course all question is eliminated with

table = malloc(max * sizeof *table);

Of course. Just as Ben said in the part that you snipped, just after
"There is a better way to call malloc that avoids this problem:".

 

[Chris Dollin]

MN wrote:

(I've no idea why the newsreplier has trashed your spaces)

table = malloc (sizeof(int*)* max);

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}

The initialisation code seems unnecessarily complicated.

table[0] = 0, table[1] = 1;
for (i = 2; i < max; i += 1) table = table[i-1] * 2;

(The comma-expression and += are just hedgehog-stylistic.)

 

[MN]

Thanks to all of you for helping me to correct my code. Now it looks
like this:

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

const int max_length = 15;
const int s_length = 2;


int* table(int max)
{
int i = 0;
int* table = NULL;

table = malloc(max * sizeof *table);
for (i = 0; i < max; i++)
{
switch (i)
{
case 0: table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}
}

return table;
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;

test = malloc(max * sizeof(*test));

for (i = 0; i< max; ++i)
test = malloc((s+1) * sizeof(test));

for (i = 0; i< max; ++i)
strcpy(test, "22");

return test;
}


int main (void)
{
int* table_tmp = NULL;
char** test_tmp = NULL;
int j = 0;

table_tmp = malloc (max_length * sizeof (*table_tmp));
table_tmp = table (max_length);

for (j = 0; j < max_length; ++j)
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);

printf("\n");

test_tmp = malloc (max_length * sizeof *test_tmp);

for(j = 0; j < max_length; ++j)
test_tmp[j] = malloc ((s_length+1) * sizeof (*test_tmp[j]) );

test_tmp = test_foo(max_length, s_length);

for (j = 0; j < max_length; ++j)
printf("test_tmp [%d] = %s\n", j, test_tmp[j]);

// Now all allocated memory ----> here only 50 % is freed,
why ?
for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);
return 0;
}

But till now I don't know how to free memory allocated in the main
function.
I understand that writing code like this:

for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);

Means that I'm only freeing memory that was allocated in functions
"test_foo" and "table", but not the one in main function,
i.e"test_tmp" and "table_tmp".
This problem will reappear if I must functions "test_foo" and "table"
in other functions.

 

[Ike Naar]

Thanks to all of you for helping me to correct my code. Now it looks
like this:

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

const int max_length = 15;
const int s_length = 2;


int* table(int max)
{
int i = 0;
int* table = NULL;

table = malloc(max * sizeof *table);

Instead of initialising table with NULL and then immediately assigning
it a meaningful value, you might as well initialise it with the meaningful
value:

int *table = malloc(max * sizeof *table);

In case of failure, malloc retuns a null pointer; it is better
to check for this.

for (i = 0; i < max; i++)
{
switch (i)
{
case 0: table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}
}

If max is guaranteed to be >=2, this can be simplified to:

table[0] = 0;
table[1] = 1;
for (i = 2; i < max; i++)
{
table = table[i-1] * 2;
}

return table;
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;

test = malloc(max * sizeof(*test));

Again, you can initialise test with a meaningful value instead
of initialising it with NULL and then assigning a meaningful value.
And again, you should check if malloc returs a null pointer.

for (i = 0; i< max; ++i)
test = malloc((s+1) * sizeof(test));

This will allocate space for s+1 pointers to char.
You want to allocate space for s+1 chars:

test = malloc((s+1) * sizeof *test);

for (i = 0; i< max; ++i)
strcpy(test, "22");

This assumes s is at least 2.

return test;
}


int main (void)
{
int* table_tmp = NULL;
char** test_tmp = NULL;
int j = 0;

table_tmp = malloc (max_length * sizeof (*table_tmp));
table_tmp = table (max_length);

Memory leak; the only pointer to the block returned by malloc()
is overwritten by the result of the table() call.

for (j = 0; j < max_length; ++j)
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);

printf("\n");

test_tmp = malloc (max_length * sizeof *test_tmp);

for(j = 0; j < max_length; ++j)
test_tmp[j] = malloc ((s_length+1) * sizeof (*test_tmp[j]) );

test_tmp = test_foo(max_length, s_length);

Memory leak; the only pointer to the block returned by the previous
malloc() is now overwritten by the result of test_foo().

for (j = 0; j < max_length; ++j)
printf("test_tmp [%d] = %s\n", j, test_tmp[j]);

// Now all allocated memory ----> here only 50 % is freed,
why ?

The other 50% is leaked.

for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);
return 0;
}

 

[Jens Thoms Toerring]

Thanks to all of you for helping me to correct my code. Now it looks

Somewhat better, but still some problems...

like this:

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;

test = malloc(max * sizeof(*test));

for (i = 0; i< max; ++i)
test = malloc((s+1) * sizeof(test));

Here you forgot the '*' in front of 'test on the right hand side,
you want the sizeof what 'table' is pointing to (a char), not the
sizeof what 'table' is (a pointer to char). So make that

test = malloc((s+1) * sizeof *test);

for (i = 0; i< max; ++i)
strcpy(test, "22");

return test;
}

int main (void)
{
int* table_tmp = NULL;
char** test_tmp = NULL;
int j = 0;

table_tmp = malloc (max_length * sizeof (*table_tmp));
table_tmp = table (max_length);

Here's the problem you're mostly interested in, see below.

test_tmp = malloc (max_length * sizeof *test_tmp);

for(j = 0; j < max_length; ++j)
test_tmp[j] = malloc ((s_length+1) * sizeof (*test_tmp[j]) );

test_tmp = test_foo(max_length, s_length);

And here again.

for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);
return 0;
}

But till now I don't know how to free memory allocated in the main
function.

You can't the way you have written the program. In main() you have

table_tmp = malloc (max_length * sizeof (*table_tmp));
table_tmp = table (max_length);

That way you first assign a value to 'table_tmp' that is the
return value of malloc(), which points to allocated memory of
'max_length' ints. But in the next line you assign a different
value to 'table_tmp', irreversibly losing the first value.
Thus you aren't able anymore to free the memory you obtained
via your call of malloc() in main(). All you can still free
is the memory you allocated in function table().

It's like doing

int x;
x = 5;
x = rand( );

and afterwards wanting to get at the first value 'x' had (5).
That simply isn't possible.

I have no idea why you first allocate memory in main(), assign
the value to 'table_tmp' and then do another allocation in
table(), assigning the return value also to 'table_tmp'.
Since the memory allocated first in main() is never used for
anything I would guess that the simplest fix would be to
simple delete the line

table_tmp = malloc (max_length * sizeof (*table_tmp));

This doesn't change anything significant about your program
and will avoid the memory leak. Or, if you insist on the
rather useless allocation in main(), do

table_tmp = malloc (max_length * sizeof (*table_tmp)); free( table_tmp );
table_tmp = table (max_length);

to get rid of the allocated memory before you overwrite the
only pointer you hold to it.

The third possibiliy, of course, is to use different variables
for storing the the address of the memory allocated in main()
and the one allocated in table(). Then you can free both memory
areas later on:

table_tmp_1 = malloc (max_length * sizeof *table_tmp_1);
table_tmp_2 = table (max_length);

The same holds, of course, also for the second case where
you allocate memory for 'test_tmp' and then the elements
of 'test_tmp' and, in the next step, overwrite the value
of 'test_tmp' with the return value of test_foo(), again
making it impossible to free the memory you allocated in
main() first.

Since it's unclear to me what you intend with your program I
can't say which of these alternatives is the best .

I understand that writing code like this:

for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);

Means that I'm only freeing memory that was allocated in functions
"test_foo" and "table", but not the one in main function,
i.e"test_tmp" and "table_tmp".

Fine. And the only solution to avoid that is one of

a) don't allocate memory in main()
b) free the memory you allocated in main() before you over-
write the only pointer you're having to that memory
c) store the pointer to the memory allocated in main() in
a different variable, so you can still free it later on.

Regards, Jens

 

[MN]

Thanks to all of you for helping me to correct my code. Now it looks

Somewhat better, but still some problems...

like this:
char** test_foo(int max, int s)
{
        int i = 0;
        char** test = NULL;
        test = malloc(max * sizeof(*test));
        for (i = 0; i< max; ++i)
                test = malloc((s+1) * sizeof(test));

Here you forgot the '*' in front of 'test on the right hand side,
you want the sizeof what 'table' is pointing to (a char), not the
sizeof what 'table' is (a pointer to char). So make that

                  test = malloc((s+1) * sizeof *test);

        for (i = 0; i< max; ++i)
                strcpy(test, "22");
        return test;
}
int main (void)
{
        int* table_tmp = NULL;
        char** test_tmp = NULL;
        int j = 0;
        table_tmp = malloc (max_length * sizeof (*table_tmp));
        table_tmp = table (max_length);

Here's the problem you're mostly interested in, see below.

        test_tmp = malloc (max_length * sizeof *test_tmp);
        for(j = 0; j < max_length; ++j)
                test_tmp[j] = malloc ((s_length+1) * sizeof (*test_tmp[j]) );
        test_tmp = test_foo(max_length, s_length);

And here again.

        for (j = 0; j < max_length; ++j)
                free (test_tmp[j]);
        free (test_tmp);
        free (table_tmp);
        return 0;
}
But till now I don't know how to free memory allocated in the main
function.

You can't the way you have written the program. In main() you have

        table_tmp = malloc (max_length * sizeof (*table_tmp));
        table_tmp = table (max_length);

That way you first assign a value to 'table_tmp' that is the
return value of malloc(), which points to allocated memory of
'max_length' ints. But in the next line you assign a different
value to 'table_tmp', irreversibly losing the first value.
Thus you aren't able anymore to free the memory you obtained
via your call of malloc() in main(). All you can still free
is the memory you allocated in function table().

It's like doing

int x;
x = 5;
x = rand( );

and afterwards wanting to get at the first value 'x' had (5).
That simply isn't possible.

I have no idea why you first allocate memory in main(), assign
the value to 'table_tmp' and then do another allocation in
table(), assigning the return value also to 'table_tmp'.
Since the memory allocated first in main() is never used for
anything I would guess that the simplest fix would be to
simple delete the line

        table_tmp = malloc (max_length * sizeof (*table_tmp));

This doesn't change anything significant about your program
and will avoid the memory leak. Or, if you insist on the
rather useless allocation in main(), do

        table_tmp = malloc (max_length * sizeof (*table_tmp));

          free( table_tmp );

        table_tmp = table (max_length);

to get rid of the allocated memory before you overwrite the
only pointer you hold to it.

The third possibiliy, of course, is to use different variables
for storing the the address of the memory allocated in main()
and the one allocated in table(). Then you can free both memory
areas later on:

         table_tmp_1 = malloc (max_length * sizeof *table_tmp_1);
         table_tmp_2 = table (max_length);

The same holds, of course, also for the second case where
you allocate memory for 'test_tmp' and then the elements
of 'test_tmp' and, in the next step, overwrite the value
of 'test_tmp' with the return value of test_foo(), again
making it impossible to free the memory you allocated in
main() first.

Since it's unclear to me what you intend with your program I
can't say which of these alternatives is the best .

I understand that writing code like this:
for (j = 0; j < max_length; ++j)
      free (test_tmp[j]);
free (test_tmp);
free (table_tmp);
Means that I'm only freeing memory that was allocated in functions
"test_foo" and "table", but not the one in main function,
i.e"test_tmp" and "table_tmp".

Fine. And the only solution to avoid that is one of

a) don't allocate memory in main()
b) free the memory you allocated in main() before you over-
   write the only pointer you're having to that memory
c) store the pointer to the memory allocated in main() in
   a different variable, so you can still free it later on.

                              Regards, Jens

Hi again,
Thanks for your explanation. I decided to use solution a).
I reworte my code and even addanother function, which call to
"test_foo" function.
Now there is no memory leak in my program. Thanks again to of you.

The new code is:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

const int max_length = 15;
const int s_length = 2;


int* table(int max)
{
int i = 0;
int* table = NULL;

table = malloc(max * sizeof *table);
if (!table)
{
fprintf(stderr, "table is Out of memory\n");
exit (8);
}

for (i = 0; i < max; i++)
{
switch (i)
{
case 0: table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}
}
return table;
}

char** test_foo(int max, int s)
{
int i = 0;
char** test = NULL;

test = malloc(max * sizeof(*test));
if (!test)
{
fprintf(stderr, "test is Out of memory\n");
exit (8);
}

for (i = 0; i< max; ++i)
{
test = malloc((s+1) * sizeof(*test));
if (!test)
{
fprintf(stderr, "test[%d] is Out of memory\n", i);
exit (8);
}
}

for (i = 0; i< max; ++i)
sprintf(test, "%d", i);
return test;
}

void test_bar(int max, int s)
{
char** bar = NULL;
char** ptr = NULL;

int k = 0;

bar = malloc(max * sizeof(*bar));
if (!bar)
{
fprintf(stderr, "bar is Out of memory\n");
exit (8);
}

for (k = 0; k < max; ++k)
{
bar[k] = malloc((s+1) * sizeof(*bar[k]));
if (!bar[k])
{
fprintf(stderr, "bar[%d] is Out of memory\n", k);
exit (8);
}
}

ptr = test_foo(max, s);

for (k = 0; k < max; ++k)
{
strcpy(bar[k], ptr[k]);
printf("bar[%d] = %s\n",k, bar[k]);
}
for (k = 0; k < max; ++k)
{
free(ptr[k]);
free(bar[k]);
}
free(ptr);
free(bar);
return ;
}

int main (void)
{
int* table_tmp = NULL;
char** test_tmp = NULL;
int j = 0;

table_tmp = table (max_length);

for (j = 0; j < max_length; ++j)
printf("table_tmp [%d] = %d\n", j, table_tmp[j]);
printf("\n");

test_tmp = test_foo(max_length, s_length);

for (j = 0; j < max_length; ++j)
printf("test_tmp [%d] = %s\n", j, test_tmp[j]);

test_bar(max_length, s_length);

// Now all allocated memory is freed,
for (j = 0; j < max_length; ++j)
free (test_tmp[j]);

free (test_tmp);
free (table_tmp);

return 0;
}

 

[Ben Bacarisse]

for (i = 0; i < max; i++)
{
switch (i)
{
case 0: table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}
}

I would write the switch as an if because there are really only two
cases:

if (i < 2)
table = i;
else table = table[i-1] * 2;

and I would really want to write:

table = i < 2 ? i : table[i-1] * 2;

but coding style in force might prevent me. I like conditional
expressions because they often seem to make more explicit what the
pattern really is but they seem to be out of favour with bosses.

 

[Ben Bacarisse]

MN wrote:

(I've no idea why the newsreplier has trashed your spaces)

table = malloc (sizeof(int*)* max);

for (i = 0; i < max; i++)
{
if (i == 0)
table = 0;
else
{
if (i == 1)
table= 1;
else
table = table[i-1]*2;
}
}

The initialisation code seems unnecessarily complicated.

table[0] = 0, table[1] = 1;

The OP is presumably concerned about the case of max < 2.

for (i = 2; i < max; i += 1) table = table[i-1] * 2;

 

[Richard]

for (i = 0; i < max; i++)
{
switch (i)
{
case 0: table = 0;
break;
case 1:
table = 1;
break;
default:
table = table[i-1] * 2;
break;
}
}

I would write the switch as an if because there are really only two
cases:

if (i < 2)
table = i;
else table = table[i-1] * 2;

and I would really want to write:

table = i < 2 ? i : table[i-1] * 2;

but coding style in force might prevent me. I like conditional
expressions because they often seem to make more explicit what the
pattern really is but they seem to be out of favour with bosses.

Which is strange because your use of "?:" is identical and if someone
can not read that as C then, IMO, they have no place modifying the code.

It reminds me of the time someone got told off for using *d++=*s++
because someone thought it was a misuse of C. For me it *IS* C.

 

[Chris Dollin]

The initialisation code seems unnecessarily complicated.

table[0] = 0, table[1] = 1;

The OP is presumably concerned about the case of max < 2.

Could be.

if (max > 0) table[0] = 0;
if (max > 1) table[1] = 1;

Still better than entangling the initialisation of the first two elements
into the loop.

(I'd be tempted to always mallocate at least two elements to ensure things
always worked, and risk the possibility that malloc would fail with those
extra two.)