[PATCH] ext/snmp/snmp.c memleaks

On Wednesday 20 June 2007 10:40:51 Gustaf Gunnarsson wrote:

Hi,
the following patch fixes memory leaks in the snmp module.

ftr, i've had memory leaks in snmp reported in debian as well:

http://bugs.debian.org/423296

though i'm not familiar enough with the code to comment on the patch.

sean

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?

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