Re: [PATCH] ext/snmp/snmp.c memleaks
| From: | Antony Dovgal | Date: | Wed, 04 Jul 2007 12:52:34 +0000 |
| Subject: | Re: [PATCH] ext/snmp/snmp.c memleaks | ||
| References: | 1 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-30441@lists.php.net to get a copy of this message | ||
Are you sure this part of the patch is correct?
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;
}
}
First you free the pdu and then go to the retry label which uses it and then frees it again on failure.
I'd say it should look like this:
if (st == SNMP_CMD_GET) {
+ snmp_free_pdu(response);
if ((pdu = snmp_fix_pdu(response, SNMP_MSG_GET)) != NULL) {
goto retry;
}
Could you please test & confirm it?
On 20.06.2007 13:40, Gustaf Gunnarsson wrote:
Hi, the following patch fixes memory leaks in the snmp module. Diff against PHP 5.2 CVS branch. I'd also like to supply a patch later adding support for multiple set/get operations in one PDU. I'd like not to use php_snmp_internal() function for my new operations because I think that function is to complex and hard to audit. Is that acceptable? The new function definitions would be something like: array snmp_mget(string $version, string $hostname, mixed $authparameters, array $variables, [int $timeout, [int $retries]]) Where $authparameters may be (possibly) one of string <community> array (<community>) array (snmpv3param1, snmpv3param2,....) $variables = array('var1','var2',...) Result will be returned in same fashion as realwalk(). and boolean snmp_mset(string $version, string $hostname, mixed $authparameters, array $variables, [int $timeout, [int $retries]]) $variables = array( array('oid','type','value'), array('oid2','type2','value2') ); //Gustaf-- Wbr, Antony Dovgal