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

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Wed 22 Jul 2009 03:46:45 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>

 

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 kieranm (Posted a comment)
  • -unavailable- added by iordan_neshev (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 06 Dec 2009 11:20:46 AM UTCgoldsimonOpen/ClosedOpen=>Closed
      Assigned toNone=>goldsimon
      StatusNeed Info=>Fixed
    Thu 15 Oct 2009 06:47:55 PM UTCgoldsimonStatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1