buglwIP - A Lightweight TCP/IP stack - Bugs: bug #39355, SNMP Memory Leak

 
 

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

bug #39355: SNMP Memory Leak

Submitted by:  Fatih Aşıcı <fatih>
Submitted on:  Fri 28 Jun 2013 12:31:44 PM UTC  
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Wed 03 Sep 2014 06:10:32 AM UTC, comment #3: 

Fixed, thanks for the patch.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sat 29 Jun 2013 05:11:19 PM UTC, comment #2: 

The problem is in the following code in snmp_error_response():

  /* move names back from outvb to invb */
  int v;
  struct snmp_varbind *vbi = msg_ps->invb.head;
  struct snmp_varbind *vbo = msg_ps->outvb.head;
  for (v=0; v<msg_ps->vb_idx; v++) {
    vbi->ident_len = vbo->ident_len;
    vbo->ident_len = 0;
    vbi->ident = vbo->ident;
    vbo->ident = NULL;
    vbi = vbi->next;
    vbo = vbo->next;
  }

snmp_msg_get_event() moves the names (ident fields) from the input
variable bindings to output variable bindings since they are same
for GET requests. In case of an error, it tries to send a response
identical to the request. The only differences are the error status
and error index values. This is the reason why the lines above move
the names back. So, overriding vbi->ident does not cause a memleak
since it does not point to a buffer.

But for GETNEXT and SET requests the situation is different.
snmp_msg_getnext_event() and snmp_msg_set_event() do not move the
names from input to output. Instead, they allocate a new memory area
for output variable bindings. This means ident fields are of input
variable bindings are NOT stale and they must be free'd. If everything
goes well, snmp_ok_response() releases them via snmp_varbind_list_free().
But, in case of an error, these functions also call snmp_error_response()
and ident fields of input variable bindings are overriden by output
variable bindings'. This problem does not occur if the error index is
zero (does not step into 'for' loop because msg_ps->vb_idx == 0).

My patch solves this problem by free'ing vbi->ident if it is not NULL.
Another solution would be to move the "rewind" code from snmp_error_response()
into snmp_msg_get_event() since it is specific to there.

Fatih Aşıcı <fatih>
Fri 28 Jun 2013 08:18:51 PM UTC, comment #1: 

Unfortunately, I don't know the SNMP code too well (and I always thought it's not too readable), so could you please explain what the memory leak is and why your patch fixes it?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Jun 2013 12:31:44 PM UTC, original submission:  

Attached patch fixes the problem.

Fatih Aşıcı <fatih>

 

Attached Files
file #28436:  0001-snmp-Fix-a-memory-leak.patch added by fatih (938B - text/x-patch - Patch to plug leak)

 

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 fatih (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-09-03 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2013-06-28 fatih Attached File- => Added 0001-snmp-Fix-a-memory-leak.patch, #28436

    Back to the top


    Powered by Savane 3.5