ID: 45405
Comment by: Rodrigo Campos <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:
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
Previous Comments:
------------------------------------------------------------------------
[2008-07-15 19:22:27] 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-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);
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
http://bugs.php.net/45405
--
Edit this bug report at http://bugs.php.net/?id=45405&edit=1
