CODEWITHSUNDEEP
cmalloc
edd91
edd91

Reputation: 185

What is wrong with my malloc/realloc here?

Update: Per the feedback below which I thought I understood, I've amended the code as follows but it is still troublesome:

unsigned int count = 0;
    char* filebuffer;
    filebuffer = malloc(sizeof(char));
    if (!filebuffer)
    { 

        error(500);
        return false;
    }

    while (fread(filebuffer, sizeof(char), 1, file) == 1)
    {
    count++;
    filebuffer = realloc(filebuffer, count * sizeof(char));
    printf("%lu\n", (count + 1) * sizeof(char));
    }
    if (feof(file))
    {
    *content = filebuffer;
    *length = count;
    }

Below is some code which is meant to go through a file which is piped through to the function by popen (it's a php file), and store it into a buffer, and then give content* the same pointer and *length the number of bytes read.

However it's not working. Valgrind says:

==7608== Conditional jump or move depends on uninitialised value(s)
==7608==    at 0x4C31FCE: strstr (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7608==    by 0x4036C0: interpret (server.c:513)
==7608==    by 0x401D66: main (server.c:259)
==7608==  Uninitialised value was created by a heap allocation
==7608==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7608==    by 0x4C2CF1F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7608==    by 0x40418C: load (server.c:662)
==7608==    by 0x403672: interpret (server.c:502)
==7608==    by 0x401D66: main (server.c:259)

The code is:

unsigned int count = 0;
    char* filebuffer;
    filebuffer = malloc(sizeof(char));

    if (!filebuffer)
    { 
       printf("oh noes\n");
        error(500);
        return false;
    }

    while (fread(filebuffer, sizeof(char), 1, file) == 1)
    {
        count++;
        filebuffer = realloc(NULL, sizeof(filebuffer) + sizeof(char));

    }
    if (feof(file))
    {
    *content = filebuffer;
    *length = count;
    }

Any feedback welcome and thanks in advance.

Upvotes: 2

Views: 146

Answers (2)

R Sahu
R Sahu

Reputation: 206717

The argument to realloc is wrong.

sizeof(filebuffer) is equal to sizeof(char*). It does not evaluate to the the size of the array allocated.

You need to keep track of the size using another variable and use that variable. count seems to be that variable but it's not clear from your code what you are doing and what those variables stand for.

Also, when you use

 filebuffer = realloc(NULL, some_size);

it is equivalent to

 filebuffer = malloc(some_size);

which leads to a lot of leaked memory. To stop the memory leaks, you need to use 

 filebuffer = realloc(filebuffer, some_size);

Upvotes: 2

fluter
fluter

Reputation: 13846

Your realloc does not take the buffer of previously allocated one, also you need to track the the size of the buffer.

filebuffer = realloc(NULL, sizeof(filebuffer) + sizeof(char));

It should be

filebuffer = realloc(filebuffer, <the new size>);

But filebuffer = malloc(sizeof(char)); is just looks as bad as it is, you are allocating ONE byte each type. If you don't know the size of the file in advance, I suggest you allocate block by block.

#define BLOCKSIZE 1024
char* filebuffer;
size_t current;
filebuffer = malloc(BLOCKSIZE);
current = BLOCKSIZE;
// in the loop
filebuffer = realloc(filebuffer, BLOCKSIZE + current);
current = BLOCKSIZE + current;

Upvotes: 0

Related Questions