ID: 45405
Comment by: rodrigocc at gmail dot com
Reported By: Federico Cuello <fedux at lugmen dot org dot ar>
Status: Open
Bug Type: SNMP related
Operating System: Linux
PHP Version: 5.2.6
New Comment:
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);
Previous Comments:
------------------------------------------------------------------------
[2008-07-01 14:56:17] 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-01 14:52:40] 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)
------------------------------------------------------------------------
--
Edit this bug report at http://bugs.php.net/?id=45405&edit=1
