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

Submitter:  Edward Harris <eharris>
Submitted:  Wed 25 Feb 2009 03:18:19 PM UTC
   
 
Category:  Contrib Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group administrator
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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-03-01 goldsimon Item GroupNone Faulty Behaviour
        StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code