buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27078, Possible memory leak in pppInit()

 
 

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

bug #27078: Possible memory leak in pppInit()

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Wed 22 Jul 2009 03:42:57 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Change RequestStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

Sun 23 Aug 2009 05:40:55 PM UTC, comment #2:

I think this one is a matter of looking at the code, not verifying it by runing PPP. This is definitively a memory leak and I agree to the fix. And I've checked in.

> Since pppInit() is called by the user, we should leave a note
> about this change because this may break someone's code.


I doubt that will break someone's code: if someone accessed an lwIP-internal variable (outpacket_buf), and freed it, there should already be a check for != NULL, since you cannot know up to which point it worked.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 20 Aug 2009 02:13:09 PM UTC, comment #1:

As with the other PPP bugs, can anyone verify this fix?

Kieran Mansley <kieranm>
Project Administrator
Wed 22 Jul 2009 03:42:57 PM UTC, original submission:

In ppp.c:
pppInit() tries to allocate (PPP_MRU+PPP_HDRLEN) bytes for every ppp interface. If the interface is only one there is no problem. But if they are, let's say 3, consider this situation:
outpacket_buf[0] - allocated,
outpacket_buf[1] - allocated,
outpacket_buf[2] - not allocated.

Then pppInit() returns ERR_MEM, leaving 2 buffers allocated.
// -------------------------------------------
int i, j;
...
for (i = 0; i < NUM_PPP; i++) {
pppControl[i].openFlag = 0;

subnetMask = htonl(0xffffff00); // why is this here?

outpacket_buf[i] = (u_char *)mem_malloc(PPP_MRU+PPP_HDRLEN);
if(!outpacket_buf[i]) {
return ERR_MEM;
}

/*
* Initialize to the standard option set.
*/
for (j = 0; (protp = ppp_protocols[j]) != NULL; ++j) {
(*protp->init)(i);
}
}
// -------------------------------------------

I propose to change it like this:

// -------------------------------------------
subnetMask = htonl(0xffffff00);

for (i = 0; i < NUM_PPP; i++) {
pppControl[i].openFlag = 0;

outpacket_buf[i] = (u_char *)mem_malloc(PPP_MRU+PPP_HDRLEN);
if (!outpacket_buf[i]) {
// +++ -->
for (j = 0; j < i; j++) {
/* deallocate all preceding buffers */
mem_free(outpacket_buf[j]);
}
// +++ <--

return ERR_MEM;
}

/*
* Initialize to the standard option set.
*/
for (j = 0; (protp = ppp_protocols[j]) != NULL; ++j) {
(*protp->init)(i);
}
}
// -------------------------------------------

// offtopic:
// subnetMask should not be initialized a couple of times
// in the loop, once is enough.That's why I pulled it out.
// What do you think?

I doubt that anybody uses more than one ppp interface, so this
is a minor problem.

Since pppInit() is called by the user, we should leave a note
about this change because this may break someone's code.

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

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 23 Aug 2009 05:40:55 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1