php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #45405 snmp extension memory leak
Submitted: 2008-07-01 14:52 UTC Modified: 2008-09-06 08:00 UTC
Votes:10
Avg. Score:4.2 ± 1.0
Reproduced:7 of 7 (100.0%)
Same Version:3 (42.9%)
Same OS:5 (71.4%)
From: Federico Cuello <fedux at lugmen dot org dot ar> Assigned:
Status: Closed Package: SNMP related
PHP Version: 5.2.6 OS: Linux
Private report: No CVE-ID:
 [2008-07-01 14:52 UTC] Federico Cuello <fedux at lugmen dot org dot ar>
Description:
------------
The snmp extension leaks memory.

Reproduce code:
---------------
<?php
while(1) {
   $oid = "HOST-RESOURCES-MIB::hrSystemUptime.0";
   $data = snmpget('localhost', 'public' , $oid);
   print "\n";
   var_export($data);
}
?>

Expected result:
----------------
Memory use should not increment continuously.

Actual result:
--------------
Memory use increases.

Valgrind output:

==21733== 2,280 (432 direct, 1,848 indirect) bytes in 3 blocks are definitely lost in loss record 64 of 67
==21733==    at 0x4022998: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==21733==    by 0x45F01A3: _clone_pdu_header (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x45F0374: _clone_pdu (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x45F0595: snmp_synch_input (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x4617F0B: _sess_process_packet (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x461A2DD: _sess_read (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x461B1F8: snmp_sess_read (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x461B25B: snmp_read (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x45EF7C1: snmp_synch_response_cb (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x45EF8A4: snmp_synch_response (in /usr/lib/libnetsnmp.so.15.0.0)
==21733==    by 0x818BBAA: php_snmp_internal (in /usr/lib/php5/bin/php)
==21733==    by 0x818D910: php_snmp (in /usr/lib/php5/bin/php)
==21733==    by 0x82CDC17: zend_do_fcall_common_helper_SPEC (in /usr/lib/php5/bin/php)
==21733==    by 0x82CCA2B: execute (in /usr/lib/php5/bin/php)
==21733==    by 0x82ABE0B: zend_execute_scripts (in /usr/lib/php5/bin/php)
==21733==    by 0x8264941: php_execute_script (in /usr/lib/php5/bin/php)
==21733==    by 0x83397C2: main (in /usr/lib/php5/bin/php) 

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2008-07-01 14:56 UTC] Federico Cuello <fedux at lugmen dot org dot ar>
Leak fix patch:

--- ext/snmp/snmp.c.orig        2008-07-01 11:21:10.000000000 -0300
+++ ext/snmp/snmp.c     2008-07-01 11:21:18.000000000 -0300
@@ -417,13 +417,13 @@
        while (keepwalking) {
                keepwalking = 0;
                if ((st == SNMP_CMD_GET) || (st == SNMP_CMD_GETNEXT)) {
-                       pdu = snmp_pdu_create((st == SNMP_CMD_GET) ? SNMP_MSG_GET : SNMP_MSG_GETNEXT);
                        name_length = MAX_OID_LEN;
                        if (!snmp_parse_oid(objid, name, &name_length)) {
                                php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid object identifier: %s", objid);
                                snmp_close(ss);
                                RETURN_FALSE;
                        }
+                       pdu = snmp_pdu_create((st == SNMP_CMD_GET) ? SNMP_MSG_GET : SNMP_MSG_GETNEXT);
                        snmp_add_null_var(pdu, name, name_length);
                } else if (st == SNMP_CMD_SET) {
                        pdu = snmp_pdu_create(SNMP_MSG_SET);
@@ -434,6 +434,7 @@
                                sprint_objid(buf, name, name_length);
 #endif
                                php_error_docref(NULL TSRMLS_CC, E_WARNING, "Could not add variable: %s %c %s", buf, type, value);
+                               snmp_free_pdu(pdu);
                                snmp_close(ss);
                                RETURN_FALSE;
                        }
@@ -455,6 +456,7 @@
                                for (vars = response->variables; vars; vars = vars->next_variable) {
                                        if (st >= SNMP_CMD_WALK && st != SNMP_CMD_SET &&
                                                (vars->name_length < rootlen || memcmp(root, vars->name, rootlen * sizeof(oid)))) {
+                                               snmp_free_pdu(response);
                                                continue;       /* not part of this subtree */
                                        }

@@ -467,11 +469,13 @@
                                                *return_value = *snmpval;
                                                zval_copy_ctor(return_value);
                                                zval_ptr_dtor(&snmpval);
+                                               snmp_free_pdu(response);
                                                snmp_close(ss);
                                                return;
                                        } else if (st == SNMP_CMD_GETNEXT) {
                                                *return_value = *snmpval;
                                                zval_copy_ctor(return_value);
+                                               snmp_free_pdu(response);
                                                snmp_close(ss);
                                                return;
                                        } else if (st == SNMP_CMD_WALK) {
@@ -510,23 +514,28 @@
                                        }
                                        if (st == SNMP_CMD_GET) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_GET)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st == SNMP_CMD_SET) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_SET)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st == SNMP_CMD_GETNEXT) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_GETNEXT)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st >= SNMP_CMD_WALK) { /* Here we do walks. */
                                                if ((pdu = snmp_fix_pdu(response, ((session->version == SNMP_VERSION_1)
                                                                                ? SNMP_MSG_GETNEXT
                                                                                : SNMP_MSG_GETBULK))) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        }
+                                       snmp_free_pdu(response);
                                        snmp_close(ss);
                                        if (st == SNMP_CMD_WALK || st == SNMP_CMD_REALWALK) {
                                                zval_dtor(return_value);
 [2008-07-15 19:22 UTC] rodrigocc at gmail dot com
The patch published by Federico Cuello produces:
*** glibc detected *** /usr/sbin/apache2: double free or corruption (!prev): 0x08644d70 ***
======= Backtrace: =========
/lib/libc.so.6[0xb7c95cf0]
/lib/libc.so.6(cfree+0x89)[0xb7c97379]
/usr/lib/libnetsnmp.so.15(snmp_free_pdu+0xfd)[0xb710231d]

(that's not the entire backtrace, but enough to suspect from the snmp extension :)

Looking this chunk of the patch (modified the tabs to "look" better. But its awful anyway :)

for (vars = response->variables; vars; vars = vars->next_variable) {

 if (st >= SNMP_CMD_WALK && st != SNMP_CMD_SET && vars->name_length < rootlen || memcmp(root, vars->name, rootlen * sizeof(oid)))) {

+ snmp_free_pdu(response);
   continue;       /* not part of this subtree */


So, if the for does more than one iteration, response is beeing freed more than one time (the for does not change response). So I think that adding that free is not correct.

Looking at the code, that "for" is inside an "if" statement. When that if statement ends, the response it's beeing freed. So there's no need to free it before.

The other "frees" added are before a return statement, that seems correct, and before "goto retry". The first instruction executed in retry is "status = snmp_synch_response(ss, pdu, &response);". So if we dont free response before calling that, we lost the reference and we have a leak.

We have done some basic tests to the code with this patch and seems to be OK (test the reproduce-code published by Federico with valgrind and using php apache module for a web-interface that uses php-snmp extension)

Federico's patch modified (just removed that "free" inside the "for") results in this (sorry, i didn't find a way to upload a file. If there's any, please let me know ):

--- ext/snmp/snmp.c.orig        2008-07-15 10:49:14.000000000 -0300
+++ ext/snmp/snmp.c     2008-07-15 15:01:48.000000000 -0300
@@ -417,13 +417,13 @@
        while (keepwalking) {
                keepwalking = 0;
                if ((st == SNMP_CMD_GET) || (st == SNMP_CMD_GETNEXT)) {
-                       pdu = snmp_pdu_create((st == SNMP_CMD_GET) ? SNMP_MSG_GET : SNMP_MSG_GETNEXT);
                        name_length = MAX_OID_LEN;
                        if (!snmp_parse_oid(objid, name, &name_length)) {
                                php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid object identifier: %s", objid);
                                snmp_close(ss);
                                RETURN_FALSE;
                        }
+                       pdu = snmp_pdu_create((st == SNMP_CMD_GET) ? SNMP_MSG_GET : SNMP_MSG_GETNEXT);
                        snmp_add_null_var(pdu, name, name_length);
                } else if (st == SNMP_CMD_SET) {
                        pdu = snmp_pdu_create(SNMP_MSG_SET);
@@ -434,6 +434,7 @@
                                sprint_objid(buf, name, name_length);
 #endif
                                php_error_docref(NULL TSRMLS_CC, E_WARNING, "Could not add variable: %s %c %s", buf, type, value);
+                               snmp_free_pdu(pdu);
                                snmp_close(ss);
                                RETURN_FALSE;
                        }
@@ -467,11 +468,13 @@
                                                *return_value = *snmpval;
                                                zval_copy_ctor(return_value);
                                                zval_ptr_dtor(&snmpval);
+                                               snmp_free_pdu(response);
                                                snmp_close(ss);
                                                return;
                                        } else if (st == SNMP_CMD_GETNEXT) {
                                                *return_value = *snmpval;
                                                zval_copy_ctor(return_value);
+                                               snmp_free_pdu(response);
                                                snmp_close(ss);
                                                return;
                                        } else if (st == SNMP_CMD_WALK) {
@@ -510,23 +513,28 @@
                                        }
                                        if (st == SNMP_CMD_GET) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_GET)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st == SNMP_CMD_SET) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_SET)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st == SNMP_CMD_GETNEXT) {
                                                if ((pdu = snmp_fix_pdu(response, SNMP_MSG_GETNEXT)) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        } else if (st >= SNMP_CMD_WALK) { /* Here we do walks. */
                                                if ((pdu = snmp_fix_pdu(response, ((session->version == SNMP_VERSION_1)
                                                                                ? SNMP_MSG_GETNEXT
                                                                                : SNMP_MSG_GETBULK))) != NULL) {
+                                                       snmp_free_pdu(response);
                                                        goto retry;
                                                }
                                        }
+                                       snmp_free_pdu(response);
                                        snmp_close(ss);
                                        if (st == SNMP_CMD_WALK || st == SNMP_CMD_REALWALK) {
                                                zval_dtor(return_value);
 [2008-07-27 23:36 UTC] Rodrigo Campos <rodrigocc at gmail dot com
We have been running the patch I post before (Federico's modified patch) with no problem since we post it here.

Without the patch it is really unusable the snmp extension for us, it consumes (leaks ;) a LOT of memory. It would be nice to have your (upstream) opinion about the patch, so distributions could consider applying it.

If there are any tests or something that we could do, please let me know.


Thanks a lot,
Rodrigo
 [2008-09-06 08:00 UTC] indeyets@php.net
This bug has been fixed in CVS.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Sun Apr 20 20:02:01 2014 UTC