Memory leak with malloc and fgetc

Question

I'm having some trouble reading a like from user using malloc and getchar. I get the result, however, I get memory leaks using valgrind. I am pretty clueless in this and have asked my classmates and mentors, but none seem to find out why.

char *ReadLineFile(FILE *infile){
   int i=0;
   char c;
   char *newStringLine;
   newStringLine = (char *) malloc(sizeof(char));
   while( (c = fgetc(infile)) != '\n' ){
        newStringLine[i++] = c;
        realloc(newStringLine, (sizeof(char) * (i+1)));
   }
   newStringLine[i] = '\0';
   return newStringLine;
}

Valgrind gives me several errors, including Invalid write/read of 1, and invalid realloc.

[Please see this discussion on why not to cast the return value of malloc() and family in C..](https://stackoverflow.com/q/605845/2173917) — Sourav Ghosh, Aug 29 '17 at 20:42
That was a fast reply! I tried not casting it, however, I kept getting the same leaks and errors. — ricknaght, Aug 29 '17 at 20:43
You should use the return value of `realloc`. You throw it away. — BLUEPIXY, Aug 29 '17 at 20:44
@ricknaght Totally ignore the reference about casting the return of malloc. There is an answer of a low-qualified programmer that does not know how to write code. — Vlad from Moscow, Aug 29 '17 at 21:06

Sourav Ghosh · Accepted Answer · 2017-08-29T20:52:54.267

5

Your usage of realloc() is erroneous.

realloc(), if successful, frees the passed pointer and returns a new pointer with the allocated memory. You need to

catch the return value of realloc() in a temporary pointer,
check against NULL to ensure success and then
- If returned pointer is not NULL, i.e., reallocation is successful, use the new pointer.
- If the returned pointer is NULL, make some decisions and you can keep using the old pointer (passed as argument).

Related, quoting C11, chapter §7.22.3.5

The realloc function deallocates the old object pointed to by ptr and returns a pointer to a new object that has the size specified by size. [....]

and,

[...] If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged.

Otherwise, in case, realloc() is successful, you are (most probably) trying to use an already free-d memory, which , of course, causes undefined behavior.

Uh-oh, and did I mention, please see this discussion on why not to cast the return value of malloc() and family in C?

edited Aug 29 '17 at 20:52

answered Aug 29 '17 at 20:44

Sourav Ghosh

133,132
16
183
261

1

" in case, realloc() is successful, you are trying to use an already free-d memory": not necessarily, if the system just increases the size of the block without moving the memory (which often happens). But it's roulette. – Jean-François Fabre Aug 29 '17 at 20:47
@Jean-FrançoisFabre well, that's not guaranteed, right? better play safe. :) – Sourav Ghosh Aug 29 '17 at 20:49
of course! but I've seen other questions like this where OP could run his code up to some point where memory is moved. False impression that it's working! – Jean-François Fabre Aug 29 '17 at 20:50
@Jean-FrançoisFabre and that is precisely sir, what I want to avoid. Added a note, though. :) – Sourav Ghosh Aug 29 '17 at 20:51
1

That worked! As suggested, I used a temp variable, and then simply assigned it back to my newStringLine pointer. This definitely taught me a lesson! Thanks! – ricknaght Aug 29 '17 at 22:42

Memory leak with malloc and fgetc

1 Answers1