Printable View
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.
Provide the smallest and simplest compilable program that demonstrates the error.
Quote:
Originally Posted by laserlight
Code: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);
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.
> 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() intoQuote:
Originally Posted by Salem
block - so you don't get to call free() if fopen() failed.Code:if (file) {}
Thanks for all of the help. I have implemented the changes and it works fine. Thanks!
@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).Quote:
Originally Posted by Salem
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:
Could there be a more awful and misleading typedef? Since when is char a String?Code:typedef String char;
> 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.
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.Code:#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( "%s\n", p ); // you're saying this works?
return 0;
}
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...Quote:
Originally Posted by Salem
Anyway, fix is:
- and call addChar(&buffer, current)Code: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);
}
Much better :)
Thanks. I guess i was hanging too much on Balkan forums... :DQuote:
Originally Posted by Salem
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 ):
Hope that helps.Code: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;
}
There's "*str=NULL;" missing after "free(*str);" - in case free() will be used, and return value of function ignored...Quote:
Originally Posted by mingerso
That is the typical method but depends on what you're doing. It is not required and I see your point, but if you're going to be using pointer allocations this way you might want to make a wrapper for free() that sets it for you:
Code:void * safe_free(void * ptr)
{
if(ptr)
free(ptr);
return NULL;
}
var = safe_free(var);
It is safe to free a null pointer, so one could write:
But then since this requires one to remember to assign the return value of safe_free() back to the pointer, I see no benefit over just writing:Code:void * safe_free(void * ptr)
{
free(ptr);
return NULL;
}
Code:free(var);
var = NULL;
Yes, but for me that's a 2-step operation (function code, and checking return value).Quote:
Originally Posted by mingerso
I just call a function and it sets pointer to NULL if something goes wrong, so checking return value is optional (and pointer even if free() got called, remains valid).
I would prefer something like this:
--Code:void safe_free(void **ptr)
{
if (ptr) // Check for stupid programmer errors
{
free(*ptr);
*ptr = NULL;
}
}
// And to complete it...
#define free(x) safe_free(&x)
Mats
My style (preference?) exactly ;)Quote:
Originally Posted by matsp
Yes all very good points of preference... and back to what was being said:
"... so do something... or not"
or better yet, you could just:Rather than having an IF, in that case.Code:assert(ptr);
Yes, I agree.Quote:
Originally Posted by iMalc
--
Mats