buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27079, Yet another leak in PPP

 
 

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

bug #27079: Yet another leak in PPP

Submitter:  Iordan Neshev <iordan_neshev>
Submitted:  Wed 22 Jul 2009 03:46:45 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Sun 06 Dec 2009 11:20:46 AM UTC, comment #4: 

Although this isn't a real bug, strictly spoken (it's an init function and lwIP doesn't provide a counterpart for that), I've fixed the bug by allocating outpacket_buf statically.

This is what ucip (the source of our ppp code) does and what we did up to lwIP 1.2.0. I really don't know why that has been changed in the first place... (introduced when adding PPPoE support in ppp.c 1.14) - the code is even smaller with static allocation.

Marc: if you read this and disagree, please let me know!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Oct 2009 06:47:55 PM UTC, comment #3: 

We really do need a developer caring about PPP or these bugs will never be closed...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 23 Aug 2009 05:47:49 PM UTC, comment #2: 

Hmm, you're right that it is never deallocated, but since I cannot test it, too, inserting a mem_free somewhere seems kind of risky.

Anyway, I think it is not deallocated as there is no function like pppFinish().

pppMain, at the end of which you wanted to deallocate it, is the end of an attempt to open a connection - through pppOpen. I doubt this is the correct place to free it. I think the lack of mem_free is due to the lack of a graceful shutdown of the complete lwIP stack. Therefore, I'd regard this one as invalid...?


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


So you do not use PPP at all?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 20 Aug 2009 02:12:40 PM UTC, comment #1: 

I'd be happy for this to be fixed, but really need someone who can verify it.  Any volunteers?

Kieran Mansley <kieranm>
Group Member
Wed 22 Jul 2009 03:46:45 PM UTC, original submission:  

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.

Iordan Neshev <iordan_neshev>

 

(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 kieranm (Posted a comment)
  • -email is unavailable- added by iordan_neshev (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-12-06 goldsimon Open/ClosedOpen Closed
        Assigned toNone goldsimon
        StatusNeed Info Fixed
    2009-10-15 goldsimon StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code