# [PATCH] ext/snmp/snmp.c memleaks

- Thread: https://externals.io/message/30301
- Original archive: https://news-web.php.net/php.internals/30301
- Started by Gustaf Gunnarsson on 2007-06-20T09:40:51+00:00
- Messages: 4
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #30301 — Gustaf Gunnarsson — 2007-06-20T09:40:51+00:00

URL: https://externals.io/message/30301#30301

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

---

## #30311 — sean finney — 2007-06-21T11:28:08+00:00

URL: https://externals.io/message/30301#30311 · In reply to: #30301

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

---

## #30441 — Antony Dovgal — 2007-07-04T12:52:34+00:00

URL: https://externals.io/message/30301#30441 · In reply to: #30301

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

---

## #30453 — Gustaf Gunnarsson — 2007-07-05T07:18:41+00:00

URL: https://externals.io/message/30301#30453 · In reply to: #30441

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