Gaash Hazan (gahazan) wrote:
>Hi codes,
>
>I am new to net-snmp. Please understand if I do thing not in the right
>way.
>
>I believe there is an issue at netsnmp_wrap_up_request)). An issue that
>causes memory corruption and double free.
>
>I saw the problem with get sysup time OID at snmpd on net-snmp 5.4.2.1
>
>For the record, uname -a output is: Linux SCE8000 2.6.26.2 #32 SMP Sun
>Apr 26 15:55:49 IDT 2009 ppc GNU/Linux
>
>It goes like that:
>
>1. SNMP get sys up time request is received and processed.
>2. An 'asp' object is created and processed.
>3. netsnmp_wrap_up_request() at snmp_agent.c is called with an 'asp'
>object.
>4. The 'asp' object points to 'pdu' and 'requests' objects. Both 'pdu'
>and 'requests' point to the *same* varbind objects.
>5. netsnmp_wrap_up_request() calls snmp_send() to send the pdu.
>6. if successful, snmp_send() frees asp->pdu. This also frees the pdu
>varbinds.
>7. netsnmp_wrap_up_request(), sets asp->pdu to NULL to indicate it is no
>longer valid.
>8. netsnmp_wrap_up_request() calls
>netsnmp_remove_and_free_agent_snmp_session() with the asp pointer 9.
>netsnmp_remove_and_free_agent_snmp_session() frees the entire asp object
> 9a. asp->pdu - nothing to do since it was set to NULL is step 7.
> 9b. asp->requests and their varbind are freed.
> 9c. More things are freed and finally the asp object itself is freed.
>
>The problem is in step 9b, that frees the requests' varbinds. The thing
>is that those varbinds were already freed as part of the pdu free by
>snmp_send in step #6. Recall that asp->pdu varbinds and asp->requests
>varbinds are actually the same (step 4).
>
>
snmp_free_pdu(asp->pdu) in netsnmp_wrap_up_request() will
call snmp_free_varbind() to free asp->pdu->variables netsnmp_variable_list
memroy including allocated memory in name, val, and data field
SNMP_FREE(asp->requests) in the free_agent_snmp_session function
frees the memory (asp->requests) allocated in handle_pdu() function NOT
free asp->requests[i].requestvb (netsnmp_variable_list pointer) memory
again.
thanks,
Daniel
>I think, a possible solution to this problem is to clone the asp->pdu
>object and pass the copy to snmp_send().
>
>
>Another similar problem in the same function,
>netsnmp_remove_and_free_agent_snmp_session(), happens in case of error
>status (status != SNMP_ERR_NOERROR). In that case asp->pdu is replaced
>with asp->orig_pdu and asp->pdu is freed and set to NULL. From that
>point, the same things as in steps 8 & 9 above takes place. Those lead
>to similar double free of the varbinds.
>
>I think the solution to this problem is not to free asp->pdu in case of
>error status and not or replace asp->orig_pdu with asp->pdu. To keep the
>code functional, an variable called 'pdu' was added. The variables point
>to asp->pdu (in normal operation) or to asp->orig_pdu (in case of
>error). The rest of the code will use that variable instead of asp->pdu.
>Later on, when the asp is freed, both asp->pdu and asp->orig_pdu will be
>freed, hence no memory leak was introduced.
>
>I hope the patch below fixes those problems.
>
>Any comment is more than welcomed. Pls let me know if I should open a
>bug and submit the patch.
>
>Thanks,
>Gaash
>
>
>
>
>
>>diff snmp_agent.c.orig snmp_agent.c
>>
>>
>
>1643a1644,1646
>
>
>> /* GH - use the main or original PDU */
>> netsnmp_pdu *pdu;
>>
>>
>>
>1648a1652,1653
>
>
>> /* SCE/GH - Normal operation, use the main PDU */
>> pdu = asp->pdu;
>>
>>
>1654,1656c1659,1660
>< snmp_free_pdu(asp->pdu);
>< asp->pdu = asp->orig_pdu;
>< asp->orig_pdu = NULL;
>---
>
>
>> /* GH - Error, use the original PDU. Don't free (yet) the main
>>
>>
>PDU */
>
>
>> pdu = asp->orig_pdu;
>>
>>
>1658,1662c1662,1677
>< if (asp->pdu) {
>< asp->pdu->command = SNMP_MSG_RESPONSE;
>< asp->pdu->errstat = asp->status;
>< asp->pdu->errindex = asp->index;
>< if (!snmp_send(asp->session, asp->pdu)) {
>---
>
>
>> if (pdu) {
>> pdu->command = SNMP_MSG_RESPONSE;
>> pdu->errstat = asp->status;
>> pdu->errindex = asp->index;
>>
>> /*
>> * GH - we are going to transmit a cloned PDU because the TX
>>
>>
>function frees the pdu.
>
>
>> * Freeing the PDU also frees the variables it helds. However,
>>
>>
>later on, when the
>
>
>> * asp (including its requests) are freed, the (freed)
>>
>>
>varbinds it will used and double freed.
>
>
>> * To provent this, we clone the PDU before we transmit it.
>> * TODO: handle clone failure (pdu2 == NULL)
>> */
>> netsnmp_pdu *pdu2 = snmp_clone_pdu(pdu);
>>
>> if (!snmp_send(asp->session, pdu2)) {
>>
>>
>1665c1680
>< for (var_ptr = asp->pdu->variables; var_ptr != NULL;
>---
>
>
>> for (var_ptr = pdu2->variables; var_ptr != NULL;
>>
>>
>1681,1682c1696
>< snmp_free_pdu(asp->pdu);
>< asp->pdu = NULL;
>---
>
>
>> snmp_free_pdu(pdu2);
>>
>>
>1686d1699
>< asp->pdu = NULL; /* yyy-rks: redundant, no? */
>
>------------------------------------------------------------------------------
>Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
>trial. Simplify your report design, integration and deployment - and focus on
>what you do best, core application coding. Discover what's new with
>Crystal Reports now. http://p.sf.net/sfu/bobj-july
>_______________________________________________
>Net-snmp-coders mailing list
>[email protected]
>https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
>
>
>
>
------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
