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

Submitter:  Fatih Aşıcı <fatih>
Submitted:  Fri 28 Jun 2013 12:31:44 PM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  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>
Group administrator
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>
Group administrator
Fri 28 Jun 2013 12:31:44 PM UTC, original submission:  

Attached patch fixes the problem.

Fatih Aşıcı <fatih>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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

    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 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.13-d3ae.
    Corresponding source code