[lwip-devel] [bug #27079] Yet another leak in PPP

[Iordan Neshev]

URL:
  <http://savannah.nongnu.org/bugs/?27079>

                 Summary: Yet another leak in PPP
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: iordan_neshev
            Submitted on: Wed 22 Jul 2009 03:46:45 PM GMT
                Category: None
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: 
            lwIP version: CVS Head

    _______________________________________________________

Details:

In ppp.c, err_t pppInit(void):

Recently I posted a mail and filed a bug about a possible leak
in pppInit() caused by

outpacket_buf[i] = (u_char *)mem_malloc(PPP_MRU+PPP_HDRLEN);

My further investigation shows that outpacket_buf is never deallocated, at
least I could not find where. Correct me if I am wrong.

I beleive the right place to do this is just before the end of the ppp thread
function:

static void
pppMain(void *arg)
{
...
  p = pbuf_alloc(PBUF_RAW, PPP_MRU+PPP_HDRLEN, PBUF_RAM);
  if (!p) {
    LWIP_ASSERT("p != NULL", p);
    pc->errCode = PPPERR_ALLOC;
    goto out;
  }
  ...
   while (lcp_phase[pd] != PHASE_DEAD) {
   ...
   sys_msleep(1); /* give other tasks a chance to run */
   ...
   }
   
  PPPDEBUG((LOG_INFO, "pppMain: unit %d: PHASE_DEADn", pd));
  pppDrop(pc); /* bug fix #17726 */
  pbuf_free(p);

out:
  PPPDEBUG(...);
  if(pc->linkStatusCB) {
    pc->linkStatusCB(pc->linkStatusCtx, pc->errCode ? pc->errCode :
PPPERR_PROTOCOL, NULL);
  }

// ++++++++++++++++++++++  --->
  LWIP_ASSERT("outpacket_buf is valid", (outpacket_buf[pd] != NULL));
 
//if (outpacket_buf[pd])// this should be here if I am not right about the
ASSERT.
    mem_free(outpacket_buf[pd]);
    
// +++++++++++++++++++++++  <---

  pc->openFlag = 0;
}

I was thinking about adding mem_free somewhere in the callback function
linkStatusCB,but it is not safe to do it because:

1. this may break someone's code, which assumes this buffer is valid and

2. this callback function is optional, the user has the freedom to define it
or not. When such function is not present (grep for if(!linkStatusCB)   and
if(!pc->linkStatusCB)), usually (90%) the stack waits for (lcp_phase[pd] ==
PHASE_DEAD) and then calls  pppClose(pd).

I thought about placing this mem_free in pppClose(), but then it may miss the
PPPoE case and, worse, it will not work if the first pbuf_alloc in pppMain()
fails.

I.e. this must be after the out:  label.

Unfortunately, I can not test this since we do not use OS.




    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27079>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/