Everything

php

[PATCH] ext/snmp/snmp.c memleaks

php.internals

Gustaf Gunnarsson

19 years ago

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

sean finney

19 years ago

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

Antony Dovgal

19 years ago

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

Gustaf Gunnarsson

19 years ago

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