CODEWITHSUNDEEP
cmemoryvalgrindmemory-leaks
nope
nope

Reputation: 223

Memory leak when reading into buffer

This is a simple program that reads a file "hello.txt" into a dynamically allocated buffer, initially of size 10 (doubled in size whenever it is filled up)

When running valgrind, there appears to be a memory leak, but I'm not sure what the issue is. I free'd the buffer's memory after use.

The error appears to be "Conditional jump or move depends on uninitialised value(s)"

Can anyone help identify if there is a memory leak? And if not what is the issue?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define BUFFERSIZE 10

int main(int argc, char const *argv[])
{
    FILE *source;
    source = fopen("hello.txt","r");
    char *buffer = (char *)malloc(BUFFERSIZE);

    int current_size = BUFFERSIZE;
    int len = 0;
    char c;
    while((c = fgetc(source)) != EOF)
    {
        if(len == current_size-1)
        {
            current_size *= 2;
            buffer = (char *)realloc(buffer,current_size);
        }
        buffer[len] = c;
        len++;
    }                                                                
    printf("%s",buffer);
    free(buffer);

    return 0;
}

Upvotes: 0

Views: 1064

Answers (5)

SebGR
SebGR

Reputation: 293

Out of curiosity, I tried MemoryScape (TotalView) on your piece of code and it does not show any memory leak (source and buffer variables are referenced blocks, meaning address of those memory blocks can be found somewhere in the program’s current variable data).

I think that Valgrind is definitely showing false positive here (I don't know what leak detection algorithm is behind the scene).

Upvotes: 0

Mats Petersson
Mats Petersson

Reputation: 129374

You need to use a temporary pointer:

char *temp = (char *)realloc(buffer,current_size);
if (!temp)
{
   fprintf(stderr, "Out of memory...\n");
   free(buffer);
   exit(1);
}
buffer = temp;

Edit I also note that you are casting malloc and realloc - that is usually a sign that you are using C++ to compile C, which is not ideal. If you are using an IDE, you may want to set it so that it uses C rather than C++ to compile your code - one way that typically works is to rename the file to myfile.c instead of myfile.cpp - you may have to remove it and re-add it to the project for the change to take effect. You can get some pretty nasty bugs from casting malloc/realloc.

Edit: You are also not setting the end-marker for the string in your buffer - you should do something like 

buffer[len] = '\0';

after your while-loop. Bear in mind that you may get "nothing" in your fgetc() if the file is empty as well, so len may be zero.

Your original malloc should check for NULL too.

Upvotes: 0

melpomene
melpomene

Reputation: 85767

The error appears to be "Conditional jump or move depends on uninitialised value(s)"

Then why are you asking about a memory leak? The source of this error is most likely the call to printf("%s", buffer) where buffer is not a valid string (no '\0' terminator).

Another problem with your code is that you're assigning the return value of fgetc to a char. You should change char c to int c.

Upvotes: 3

user295691
user295691

Reputation: 7248

If this is valgrind complaining, it is doing so because you are assigning the return of realloc to the buffer you are reallocing. Since realloc will return NULL if it cannot grow the buffer, this can result in the new value of buffer being assigned to NULL and the old value being leaked.

The usual metaphor is something like:

char * new_buffer = realloc(buffer, current_size);
if (!new_buffer) {
    handle_error();
}
buffer = new_buffer;

Also best practice in C is to not cast the return from malloc or realloc. Not worth getting into here aside from mentioning it; SO has lots of resources dedicated to answering this.

Upvotes: 1

askmish
askmish

Reputation: 6674

You're missing NULL checks and initializing the pointers to NULL when declaring them.

Use the valgrind option --track-origins=yes to have it track the origin of uninitialized values. This will make it slower but will help you to track down the origin of the uninitialized value.

Hope this option will help you solve any such problems in future.

Upvotes: 0

Related Questions