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()

Submitter:  Iordan Neshev <iordan_neshev>
Submitted:  Wed 22 Jul 2009 03:42:57 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group administrator
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>
Group Member
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>

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2009-08-23 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code