Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:30453
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain netadmin.se from 193.12.139.17 cause and error)
Message-ID: <468C9B51.8040504@netadmin.se>
Date: Thu, 05 Jul 2007 09:18:41 +0200
User-Agent: Thunderbird 1.5.0.12 (Windows/20070509)
MIME-Version: 1.0
To: Antony Dovgal <antony@zend.com>
Cc: Rasmus Lerdorf <rasmus@php.net>, internals@lists.php.net
References: <4678F623.2080308@netadmin.se> <468B9812.7000600@zend.com>
In-Reply-To: <468B9812.7000600@zend.com>
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: gustaf.gunnarsson@netadmin.se (Gustaf Gunnarsson)

Hi,
thank you for reviewing my patch.

Antony Dovgal wrote:
> 
> Are you sure this part of the patch is correct?

Yes

[[ cut away code ]]

> First you free the pdu and then go to the retry label which uses it and 
> then frees it again on failure.

You misunderstund.

         status = snmp_synch_response(ss, pdu, &response);

This line sends 'pdu' on 'ss' (a Net-SNMP session pointer) and sets 
'response' to point to a within snmp_sync_response() newly allocated PDU 
as a result of the response from the equipment we are communicating with.

> 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;
>                         }

No, snmp_free_pdu() is a function that free's memory. Snmp_fix_pdu() is 
a function that clones a pdu (response) into a new pdu of a specific 
type (SNMP_MSG_GET) while it does that it removes all variables in 
error, the return value from that function is a pointer to a new PDU 
that will be stored in 'pdu' (or NULL if no variables are left after all 
in error has been removed). Obviously you cannot clone memory that you 
just free()'d so my original patch is in fact correct.

> Could you please test & confirm it?

However, since the (php) snmp module currently do not support 
multi-variable-get/set operations we can never recieve an errored 
response with more than one variable in it, because of this fact 
snmp_fix_pdu() will always return NULL. And the entire codeblock is 
dead, that is atleast as far as I understund it.

So, you could say that specific part fixes a possible future problem 
that is not applicable today (atleast to my knowledge).

If you're still unsure you may take a look at:

Line 582 snmp_fix_pdu()
http://net-snmp.svn.sourceforge.net/viewvc/net-snmp/trunk/net-snmp/snmplib/snmp_client.c?view=markup

Line 5013 snmp_free_pdu()
http://net-snmp.svn.sourceforge.net/viewvc/net-snmp/trunk/net-snmp/snmplib/snmp_api.c?view=markup

//Gustaf