0

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;
}
nope
  • 223
  • 4
  • 15
  • This is unrelated to the question specifically, but the code does not appear to put a NULL terminator (byte value of 0) at the end of `buffer`. The call to `printf` will have undefined behavior without the null terminator (it will print characters until it hits a 0 byte, which could be past the end of allocated memory). – Mark Wilkins Jan 24 '13 at 19:06
  • 1
    Run valgrind with option `--track-origins=yes`. Valgrind itself will answer your question! – askmish Jan 24 '13 at 19:12

5 Answers5

3

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.

melpomene
  • 84,125
  • 8
  • 85
  • 148
1

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.

user295691
  • 7,108
  • 1
  • 26
  • 35
  • i made the changes that you suggested, but valgrind is still throwing me the same error... – nope Jan 24 '13 at 19:08
  • People have mixed views about casting return from malloc and calloc. Its fine if they cast them if they are using older version of C. If c++ or C+03, it recommended to skip casting. – askmish Jan 24 '13 at 19:09
  • also if i don't cast from malloc/realloc what is the best practice for memory allocation then? – nope Jan 24 '13 at 19:10
  • @nope Uh. Just don't cast. – melpomene Jan 24 '13 at 19:10
  • http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc has good coverage of some of the reasons not to cast. – user295691 Jan 24 '13 at 19:27
0

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.

askmish
  • 6,464
  • 23
  • 42
0

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.

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227
  • i made the changes that you suggested, but valgrind is still throwing me the same error... – nope Jan 24 '13 at 19:07
  • also if i don't cast from malloc/realloc what is the best practice for memory allocation then? – nope Jan 24 '13 at 19:08
  • Using malloc is fine - you just shouldn't use a cast to convert the result from malloc - malloc returns a "void *", which can be converted to any other pointer type in C. In C++ you need explicit casts to convert void * into something else, which is why I guess that you are trying to compile as C++. I will edit my answer with some further information. – Mats Petersson Jan 24 '13 at 19:10
0

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).

SebGR
  • 293
  • 2
  • 10