|
|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[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)
PatchesPull RequestsHistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||||||||||||
Copyright © 2001-2024 The PHP GroupAll rights reserved. |
Last updated: Thu Nov 21 08:01:29 2024 UTC |
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);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);