Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:30441
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.162 as permitted sender)
Message-ID: <468B9812.7000600@zend.com>
Date: Wed, 04 Jul 2007 16:52:34 +0400
User-Agent: Thunderbird 2.0.0.0 (X11/20070326)
MIME-Version: 1.0
To: Gustaf Gunnarsson <gustaf.gunnarsson@netadmin.se>
CC: Rasmus Lerdorf <rasmus@php.net>,  internals@lists.php.net
References: <4678F623.2080308@netadmin.se>
In-Reply-To: <4678F623.2080308@netadmin.se>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] ext/snmp/snmp.c memleaks
From: antony@zend.com (Antony Dovgal)


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