Realloc Double Free

[ma.mazmaz]

Hello,

I have been writing a library in C. For the strings part, I use realloc. I have been testing it and, when adding characters to a string, the first few characters work. But, after a few characters, realloc gives me a double free error. Being a C newbie I am probably just misusing the function. Some help here please.

[laserlight]

Provide the smallest and simplest compilable program that demonstrates the error.

[ma.mazmaz]

Provide the smallest and simplest compilable program that demonstrates the error.

typedef String char;
String* String_new(){
	String *self=calloc(1, sizeof(char));
	self[0]='\0';
	return self;
}

void String_addChar(String *self, char ch){
	if(ch=='\0'){//If the character is NULL
		return;//Exit function
	}
	int len=strlen(self);
	self=realloc(self, sizeof(char)*len+2);
	char buffer[]={ch,'\0'};
	strcat(self,buffer);
}

String* String_readFile(FILE* file){
	String *buffer=String_new();
	char current=fgetc(file);
	while(current!=EOF){
		String_addChar(buffer,current);
		current=fgetc(file);
	}
	return buffer;
}

FILE* file=fopen("hello.txt","r");
String *f=String_readFile(file);
fclose(file);
free(f);

[tabstop]

It's "typedef char String", but I guess you've got that right in your code. You're also missing headers and a main(), without which nothing much happens.

But when I add those in, nothing happens. That is, I don't get any runtime errors. So, try, try again.

[Salem]

> typedef String char;
> String* String_new(){
> String *self=calloc(1, sizeof(char));
You create a typedef, then don't use it.
If you change the type away from char, then calloc will still allocate a char. At that point, the code is broke.

> self=realloc(self, sizeof(char)*len+2);
1. If realloc fails, it returns NULL. This trashes the original pointer. If that's your only copy of the pointer, you have a memory leak.
2. realloc can MOVE the memory. Which in this case is a problem for you, since the caller doesn't get to find out about the change. So the next time you call this function, you're passing the OLD value of self again, which realloc will try to free (and hence, this is your double free).

[rasta_freak]

> typedef String char;
> String* String_new(){
> String *self=calloc(1, sizeof(char));
You create a typedef, then don't use it.
If you change the type away from char, then calloc will still allocate a char. At that point, the code is broke.

> self=realloc(self, sizeof(char)*len+2);
1. If realloc fails, it returns NULL. This trashes the original pointer. If that's your only copy of the pointer, you have a memory leak.
2. realloc can MOVE the memory. Which in this case is a problem for you, since the caller doesn't get to find out about the change. So the next time you call this function, you're passing the OLD value of self again, which realloc will try to free (and hence, this is your double free).

If realloc moves memory, it DOESN'T affect anything at all - except OTHER pointers, if you have any, that point to data being realloc()ed. In this case, he has only one pointer, he always gives realloc() proper pointer and allways stores what realloc returns into same pointer. Realloc() & free() usage is fine here. This code works, I tested it on itself (renamed file.c into hello.txt). I only enclosed readFile(), fclose() & free() in main() into

if (file) {}

block - so you don't get to call free() if fopen() failed.

[ma.mazmaz]

Thanks for all of the help. I have implemented the changes and it works fine. Thanks!

[Salem]

@rasta_freak
> String *buffer=String_new();
This is the original pointer.

> String_addChar(buffer,current);
This makes a copy of the pointer.

> self=realloc(self, sizeof(char)*len+2);
If this moves the memory, then buffer in the caller will NOT CHANGE.

> This code works, I tested it on itself (renamed file.c into hello.txt).
I'm sure it does for you. But did you test the case when realloc moved the pointer?

Sure, if the last call to malloc is the one you keep realloc'ing in a simple program, then it's going to be able to extend it a long way before moving it. The illusion of "success" whilst still being bugged.

But put it in a larger program, where other things are using malloc, and watch what happens.

[rasta_freak]

@rasta_freak
> String *buffer=String_new();
This is the original pointer.

> String_addChar(buffer,current);
This makes a copy of the pointer.

> self=realloc(self, sizeof(char)*len+2);
If this moves the memory, then buffer in the caller will NOT CHANGE.

> This code works, I tested it on itself (renamed file.c into hello.txt).
I'm sure it does for you. But did you test the case when realloc moved the pointer?

Sure, if the last call to malloc is the one you keep realloc'ing in a simple program, then it's going to be able to extend it a long way before moving it. The illusion of "success" whilst still being bugged.

But put it in a larger program, where other things are using malloc, and watch what happens.

addChar() doesn't make a copy of pointer, it operates on SAME pointer you got from calloc() in newString(). It gives realloc old "self" pointer, and gets from it a new "self" pointer (old may get invalid but nobody cares - memory/buffer stays the same, even if moved, and it's address is in "self" pointer before and after realloc()). IN ANY CASE, THIS WORKS. Even in enterprise level software, THIS WILL WORK. This code is valid, and you Salem, recheck realloc() man pages, please... You can realloc() something malloc()ed or calloc()ed freely - they use same/similar allocation methods. Only important thing is to have a SINGLE pointer to data being malloc()-ed/realloc()-ed, which is normaly a case anyway (you generate other pointers if needed out of 1st which is always valid). AND to use "x=realloc(x, new_size);" form, of course (which he uses).

[anon]

What you don't realize is that addChar indeed receives a copy of the pointer. This copied pointer points to the same string, so modifications to the string are visible to the calling code. However, modifications to the pointer itself - when the address it is pointing to is changed - are not visible to the calling code.

By the way:

typedef String char;

Could there be a more awful and misleading typedef? Since when is char a String?

[Salem]

> addChar() doesn't make a copy of pointer, it operates on SAME pointer you got from calloc() in newString().
OK, quick lesson, since you obviously have no clue.

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

void func ( char *p ) {
    p = malloc( 12 );
    strcpy( p, "hello world" );
}

int main(void)
{
    char *p = NULL; // or calling String_new() to initialise it
    func( p );  // or calling String_addChar() to expand and add data
    printf( "&#37;s\n", p );    // you're saying this works?
    return 0;
}

At least the OP was switched on enough to realise that String_new() had to return a pointer. The only thing lacking was the bit of detail about realloc moving the whole block. So String_addChar() also needs to be capable of returning a modified pointer.

[rasta_freak]

> addChar() doesn't make a copy of pointer, it operates on SAME pointer you got from calloc() in newString().
OK, quick lesson, since you obviously have no clue.

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

void func ( char *p ) {
    p = malloc( 12 );
    strcpy( p, "hello world" );
}

int main(void)
{
    char *p = NULL; // or calling String_new() to initialise it
    func( p );  // or calling String_addChar() to expand and add data
    printf( "&#37;s\n", p );    // you're saying this works?
    return 0;
}

At least the OP was switched on enough to realise that String_new() had to return a pointer. The only thing lacking was the bit of detail about realloc moving the whole block. So String_addChar() also needs to be capable of returning a modified pointer.

Ok, don't need to test it - i get it know. Didn't see it on previous reading - I heavily use realloc() but store pointers in struct so they are all updated ok (for this same reason, ironicly...). Yes, addChar() works on local copy of pointer and it's bad. It doesn't update original. Sorry Salem, i was too quick on words...
Anyway, fix is:

void String_addChar(String **p, char ch){
	if(ch=='\0'){//If the character is NULL
		return;//Exit function
	}
	int len=strlen(*p);
	*p=realloc(*p, sizeof(char)*len+2);
	char buffer[]={ch,'\0'};
	strcat(*p,buffer);
}

- and call addChar(&buffer, current)

[Salem]

Much better

[rasta_freak]

Much better

Thanks. I guess i was hanging too much on Balkan forums...

[mingerso]

This was mentioned earlier but heres an example of realloc and not destroying your previous pointer ( so you can avoid bad free()'s and memory leaks ):

char * add_space(char ** str)
{
	int len = strlen(*str);
	char * tmp = (char *)realloc(*str, sizeof(char)*len+2);
	if(tmp)
	{
                /* allocation was ok so we have our new address to point to */
		*str = tmp;
	} else {
		/* out of memory, so do something... or not */
		free(*str);
	}

	/* now you can error check the function on NULL or not */
	return tmp;	
}

Hope that helps.