Potential memmory leak in my code?

slzckboy · 04-19-2006, 05:02 AM

My question relates to the function below

Code:

int ProcessHTTPHEAD(HTTPHEAD *hd,char *origin)
{
	int n;
	char *value;
	char *name;
	char *q;
	char *p;
	HTTPFIELD *field;
	HTTPFIELD **fields;
	fields=(HTTPFIELD **)malloc(sizeof(HTTPFIELD *) * 1);	

	for(n=0;strcmp(origin,"\r\n");n++){
	  q=stringsep(&origin,"\r\n");
	  p=stringsep(&q,":");

	  if(p && q){

	  fields[n]=(HTTPFIELD *)malloc(sizeof(HTTPFIELD));
	  fields[n]->field_name=p;
	  fields[n]->field_value=q;
	  fields=(HTTPFIELD **)realloc(fields,sizeof(HTTPFIELD *) * (n+2));
	  }
	}
	fields[n]='\0';
	hd->fields=fields;
	if(fields)
	return 1;
}

stringsep works like strsep except if the token is not found it returns NULL,but dosn't NULL the entire original string;it just leaves it as it is.

The origin varibale is a complete http header.
This function breaks the header down into header field name and header field value pairs which are stored in a HTTPFIELD structure as type char *.
The problem I have created for myself is making a function to free all this memmory once the calling fuction has finished with the resources.

The origin parameter was created dynamically on the heap.
It is then broken up as previously described in this function.
By breaking it up like that,am I making the memmory unretrievable or
is it just a case of calling free on fields[n]->field_value and fields[n]->field_name ?? and then fields[n]??
These values are just pointers to different parts of the original origin variable allbeit with nulls inserted in between them so I guess not.!?
I guess what I am asking is,if i call free on the memmory address at the start of char *origin will that do the trick,or will the nulls inserted amongst it by stringsep stop it from freeing all the memmory.

Sorry if this is fuzzy.Any questions pls just ask.

Thanks as always.

ioerror · 04-19-2006, 06:26 AM

You need to free anything that you malloc. If fields[n]->whatever are just pointers into some other memory then you don't free those, just the origin. I would suggest that it's a good idea to have separate create/delete functions for each structure. That keeps memory management contained to a few functions and makes it easier to relate each malloc to it's corresponding free.

MichaelZ · 04-19-2006, 06:28 AM

Hello,

To check potential memory leaks in your code, you can use valgrind.

Best wishes,
Michael

primo · 04-19-2006, 06:40 AM

In no way the free() implementation can associate that both fields and fields[n] contains pointers that must be freed as well. You must free() each fields[n] manually, possibly _after_ fields[n]->field_name and fields[n]->field_value in the case they were malloc'ed as well. Then you may free() fields.

There's a memory leak in the way you're using realloc(). When realloc() returns NULL, it doesn't free() the original pointer. It does only if it was successful. So if you rewrite the original pointer variable with a NULL, you won't be able to use the original value to either continue using it or free() it. So check return values from malloc(), calloc(), realloc(), etc. To avoid the leak, you may use a wrapper around realloc() that frees the pointer if realloc() failed. It exists on FreeBSD and is called "reallocf()"

Code:

void *realloc(void *ptr, size_t size)
{
    void *addr = realloc(ptr, size);
    if (addr == NULL)
        free(ptr);
    return addr;
}

addy86 · 04-19-2006, 06:41 AM

You made two serious mistakes:
1. If (p && q) is false, fields[n] is not initialized. This results in wild pointers.
2. You didn't check the return value of malloc/realloc. Your program will segfault if these functions fail silently.

slzckboy · 04-19-2006, 03:31 PM

Thnks everyone for the excellent advice,especially primo as I would not have picked up on the realloc issue.

Another naive question.
Or maybe the same question put more simply.!?

if buffer=(char *)malloc(sizeof(char *) * 100);
if

Code:

buffer[2]=buffer[22]=buffer[44]=buffer[99]=NULL;

a call to free(buffer) will still free all 100 bytes right?
Essentially that is all the structures mentioned b4 are.Pointers to areas within one larger memory area on the heap,call it buffer;which are segmented,(No pun intended) into smaller chunks with NULLS.It is these smaller chunks that make up field_name,or field_value.

ioerror · 04-19-2006, 03:50 PM

Quote:

a call to free(buffer) will still free all 100 bytes right?

Yes. You have 1 malloc, so you only need 1 free. Any other pointers into that memory should not be freed (since what they point to is not an allocated block). Obviously though, once the memory is freed, all those pointers become wild and must not be referenced any further.

slzckboy · 04-19-2006, 04:20 PM

It will be in a cleanup function so that shouldn't be an issue.
Thnks again.

slzckboy · 05-16-2006, 05:05 AM

taken from

http://www.cprogramming.com/debugging/valgrind.html

Code:

If you have a memory leak, then the number of
 allocs and the number of frees will differ 
(you can't use one free to release the memory 
belonging to more than one alloc).

my leaky code..

Code:

main()

...
...
.. 
 char *buffer=NULL;
 char *recvbuffer=NULL;
  int n;
  int bytes;
  bytes=n=0;

..........
      initialise recvbuffer;

     while(condition){
        n=read(fd,recvbuffer,bytes);

         bytes+=n;

       if(n> 0)
         if(!(resize(buffer,recvbuffer,bytes))){;
           error condition act apropriately;
        
         }
      }

....
   free(buffer);
}



char *resize(char *content,char *recvBuff,int i){

        if(!(content=(char *)realloc_safe(content,sizeof(char *) * (i+1))));
          return NULL;
/* dodgy use of strncat ,I have fix.ignore*/

        content=strncat(content,recvBuff,i);
	content[i]='\0';
        return content;
}

void *realloc_safe(void *ptr,size_t size)
{

      void *addr=realloc(ptr,size0;
      
      if (addr == NULL)
        free(ptr);

      return addr; 
}

Unless i have misunderstood the article(probably)
A call to free(buffer) will not free all the memory obtained in this fashion ?!?

Thnks for your patience and taking the time to read.

slzckboy · 05-16-2006, 01:07 PM

i have written some small dummy programs using resize in a loop that runs a small amount of times

It seems that realloc calls free each time it is invoked,hence the comment that I found on the weblink.

ioerror · 05-16-2006, 03:41 PM

Quote:

It seems that realloc calls free each time it is invoked,

Not exactly, if there is enough room to extend to block without reallocating, it will do that. If more memory has been malloc'd in the meantime, then there will likely not be enough room, in which case it mallocs fresh memory, copies the old block, and then frees it.