buglwIP - A Lightweight TCP/IP stack - Bugs: bug #25705, Memory Leak in HTTPD.C

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #25705: Memory Leak in HTTPD.C

Submitted by:  Edward Harris <eharris>
Submitted on:  Wed 25 Feb 2009 03:18:19 PM UTC  
 
Category: ContribSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

Sun 01 Mar 2009 05:51:21 PM UTC, comment #5:

I think both suggested patches are correct, I've checked in this:

if ((hs == NULL) && (pcb->state == ESTABLISHED)) {
tcp_abort(pcb);
return ERR_ABRT;
} else if (hs != NULL) {
++hs->retries;
if (hs->retries == 4) {
tcp_arg(pcb, NULL);
mem_free(hs);
tcp_abort(pcb);
return ERR_ABRT;
}
send_data(pcb, hs);
}

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 26 Feb 2009 05:12:40 AM UTC, comment #4:

Edward,

Sorry for the confusion. We've added a bunch of stuff to the HTTPD server including accessing a FAT file system so I guess that must have been code that we added but which didn't make its way back into the repository. Looking at your code, however, the rework I talked about to prevent tcp_abort being called if the PCB is in ESTABLISHED state still applies.

Dave Wilson <dawilson>
Wed 25 Feb 2009 10:39:08 PM UTC, comment #3:

I'm very confused now!

The HTTPD in the httpserver_raw contrib example has only a http_state structure like this:

struct http_state {
u32_t left;
const unsigned char *file;
u8_t retries;
};

There's no handle or buf in there at all?

Edward Harris <eharris>
Wed 25 Feb 2009 05:24:57 PM UTC, comment #2:

Sorry - I forgot to mention that I agree that the original code would appear to leak the hs structure if it's not freed when you abort after multiple retries. You should also clean up the hs structure before freeing it, though - there's a file handle and buffer pointer in there, for example. As far as I can tell, the correct thing to do would be:

if(hs->handle) {
fs_close(hs->handle);
}
if(hs->buf) {
mem_free(hs->buf);
}
mem_free(hs);

Dave Wilson <dawilson>
Wed 25 Feb 2009 05:20:17 PM UTC, comment #1:

There's actually a different bug here which caught me out last week and was causing connections to be improperly terminated. I've reworked the function as follows.

if ((hs == NULL) && (pcb->state == ESTABLISHED)) {
/* printf("Null, close\n");*/
tcp_abort(pcb);
return ERR_ABRT;
} else {
++hs->retries;
if (hs->retries == 4) {
tcp_abort(pcb);
return ERR_ABRT;
}

The case where hs == NULL can occur during termination of a connection and a race condition means that if http_poll is called at this point, the connection will be aborted when it is actually closing down cleanly already. This causes confusion to some browsers, notably Safari on MacOS. The fix I propose above only aborts the connection if the PCB is supposed to be in ESTABLISHED state, thus allowing normal termination even if http_poll is called during the window where the connection is closing.

I agree that there is still a possible memory leak here but arg/hs should never be NULL while the session is established so you should never hit this case unless something else has gone really badly wrong first (memory corruption?). If hs is NULL, you've lost track of where the http_state structure is anyway so freeing it is rather tricky.

Dave Wilson <dawilson>
Wed 25 Feb 2009 03:18:19 PM UTC, original submission:

The http_poll callback function does not free the http_state structure when aborts the current tcp.

Add a "mem_free(hs);" after line 254. Function now reads thus:

static err_t
http_poll(void arg, struct tcp_pcb pcb)
{
struct http_state *hs;

hs = arg;

/* printf("Polll\n");*/
if (hs == NULL) {
/* printf("Null, close\n");*/
tcp_abort(pcb);
return ERR_ABRT;
} else {
++hs->retries;
if (hs->retries == 4) {
mem_free(hs);
tcp_abort(pcb);
return ERR_ABRT;
}
send_data(pcb, hs);
}

return ERR_OK;
}

Edward Harris <eharris>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by dawilson (Posted a comment)
  • -unavailable- added by eharris (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 01 Mar 2009 05:51:21 PM UTCgoldsimonItem GroupNone=>Faulty Behaviour
      StatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1