buglwIP - A Lightweight TCP/IP stack - Bugs: bug #32280, ppp: a pbuf is freed twice

 
 

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

bug #32280: ppp: a pbuf is freed twice

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Fri 28 Jan 2011 05:18:48 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

Sun 27 Mar 2011 02:04:15 PM UTC, comment #3:

Fixed after a code review (inHead and inTail may only be used in rx context).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 23 Mar 2011 07:17:18 PM UTC, comment #2:

P.S.: I'd love to fix this for 1.4.0, so Iordan, if you say it works with removing the pppDrop() call from pppLinkTerminated(), I'll just check it in.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 23 Mar 2011 07:16:18 PM UTC, comment #1:

I'm pretty sure the call to pppDrop() is invalid there: the whole pc->rx struct is meant to only be used in pppInProc(), and the inHead/inTail pbufs get freed/removed in that function. After being passed to pppInput(), inHead/inTail should not be touched again. As such, pppDrop() should not be called from any other function than pppInProc() and certainly not from pppLinkTerminated().

After all, I put that call there in version 1.35 of the file and it seems I was wrong :-)

Anyway, I'm not even sure if pppDrop() works correctly: I think it shouldn't free the ppp->inTail pbuf seperately...?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Jan 2011 05:18:48 PM UTC, original submission:

Consider the function pppLinkTerminated()

There is the following line:
pppDrop(&pc->rx); /* bug fix #17726 */

It fixes an ancient bug from the era when the ppp code was very old.
Recently Simon did a great work and ported back the changes from pppd 2.3.1.
I suspect that since then this line is not needed any more.

The problem I see is that an assert fires at the end of pppInput():
"Assertion "pbuf_free: p->ref > 0" failed at line 563 in E:/Work/proj1/lwIP/src/core/pbuf.c"

This happens while I am closing the link when I get a TERM ACK from the peer. When such packet is received
all protocols are lowered down and all ppp timeouts get cancelled. During this ride
pppLinkTerminated() is called too.
Then, after "goto out;" we try to free again the same pbuf, which fires the assertion.

I'm not brave enough to delete the suggested line because I'm not sure that
all paths to pppLinkTerminated go through pppInput(). I prefer to live with the
firing assertion. I added a bit of code just to suppress the firing.

There are no harmful consequences, so this can be left for lwip 1.4.1
Any suggestions what should be done?

static void pppInput(void *arg)
{
.....

switch(protocol) {
.....

default: {
struct protent *protp;
int i;

/*
* Upcall the proper protocol input routine.
*/
for (i = 0; (protp = ppp_protocols[i]) != NULL; ++i) {
if (protp->protocol == protocol && protp->enabled_flag) {
PPPDEBUG(LOG_INFO, ("npppInput[%d]: %s len=%dn", pd, protp->name, nb->len));
nb = pppSingleBuf(nb);
(*protp->input)(pd, nb->payload, nb->len); // here the packet is passed to its handler
PPPDEBUG(LOG_DETAIL, ("pppInput[%d]: packet processedn", pd));

#if 1
if (nb->ref == 0) // if the pbuf is already free-d by LinkTerminated, don't try to free it again
{
goto justreturn;
}
#endif

goto out; // if this is TERMACK, pppDrop has already been called for this pbuf (nb)
}
}

/* No handler for this protocol so reject the packet. */
PPPDEBUG(LOG_INFO, ("pppInput[%d]: rejecting unsupported proto 0x%"X16_F" len=%dn", pd, protocol, nb->len));
if (pbuf_header(nb, sizeof(protocol))) {
LWIP_ASSERT("pbuf_header failedn", 0);
goto drop;
}
.....
}
break;
} // switch

drop:
LINK_STATS_INC(link.drop);
snmp_inc_ifindiscards(&pppControl[pd].netif);
out:
pbuf_free(nb); // Assertion "pbuf_free: p->ref > 0" failed at line 563 in E:/Work/proj1/lwIP/src/core/pbuf.c

justreturn:
return;
}

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 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 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 27 Mar 2011 02:04:15 PM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Wed 23 Mar 2011 07:17:18 PM UTCgoldsimonStatusNone=>In Progress
      Assigned toNone=>goldsimon
      Planned Release=>1.4.0

    Back to the top


    Powered by Savane 3.1-cleanup1