Re: double-free crash in agentx/master.c (bug #2943)

Hello,

Can we just add something like AGENTX_MSG_FLAG_DISCONNECT_IN_PROGRESS
in close_agents_session() ?

   sesssion->subsession->flags = AGENTX_MSG_FLAG_DISCONNECT_IN_PROGRESS;

So that later in agentx_master_handler() we can check
 ax_session->subsession->flags so we can check this:

   if (!(ax_session->subsession->flags &
AGENTX_MSG_FLAG_DISCONNECT_IN_PROGRESS)) {
       cb_data = NULL;
   }

And we do not have the double free at the end.

Thanks,
Sam

On Wed, Jun 26, 2019 at 2:42 PM Sam Tannous <[email protected]>
wrote:

> Bill, Bart,
>
> Shouldn't there be a check (netsnmp_handler_check_cache)
> just before snmp_async_send to make sure the cb_data is still
> valid (or that the session hasn't been disconnected)?
>
> I'm assuming (with good evidence) that our subagent
> session was disconnected (our subagent has a problem)
> and something was freed...and we attempt to free cb_data
> again and core dump.  So at the bottom of agentx_got_response():
>
>
>     /*
>      * When the master sends a CleanupSet PDU, it will never get a response
>      * back from the subagent. So we shouldn't allocate the
>      * netsnmp_delegated_cache structure in this case.
>      */
>     if (pdu->command != AGENTX_MSG_CLEANUPSET)
>         cb_data = netsnmp_create_delegated_cache(handler, reginfo,
>                                                  reqinfo, requests,
>                                                  (void *) ax_session);
>     else
>         cb_data = NULL;
>
>
> */* some checks */*
>
>
>
>
>
>
>
>
> * cache = netsnmp_handler_check_cache(cb_data);    if (!cache) {
> DEBUGMSGTL(("agentx/master", "session may be disconnected %8p\n",
>           session));        /* response is too late, perhaps session was
> disconnected? */        if (cb_data)
> netsnmp_free_delegated_cache((netsnmp_delegated_cache*) magic);
> return 1;    }*
> */* end checks */*
>
>    /*
>      * send the requests out.
>      */
>     DEBUGMSGTL(("agentx/master", "sending pdu
> (req=0x%x,trans=0x%x,sess=0x%x)\n",
>                 (unsigned)pdu->reqid, (unsigned)pdu->transid,
> (unsigned)pdu->sessid));
>
>    result = snmp_async_send(ax_session, pdu, agentx_got_response, cb_data);
>     if (result == 0) {
>         snmp_free_pdu(pdu);
>         if (cb_data)
>             netsnmp_free_delegated_cache((netsnmp_delegated_cache*)
> cb_data);
>     }
>
>     return SNMP_ERR_NOERROR;
>
> --Sam
>
> On Tue, Jun 25, 2019 at 3:38 PM Sam Tannous <[email protected]>
> wrote:
>
>>
>> I'm still not able to recreate this bug (#2943) where we
>> double free cb_data at the bottom of agentx_master_handler()
>> (with the netsnmp_free_delegated_cache()).
>>
>> Just in looking at the code logic, it seems like we allocate
>> the netsnmp_delegated_cache structure only if the master
>> sent a CleanupSet PDU.   In the case I'm looking at, I can
>> see the master has already disconnected the subagent:
>>
>> bgpd[5180]: snmp[info]: AgentX master disconnected us, reconnecting in 15
>> ip[8042]: *** Error in `/usr/sbin/snmpd': double free or corruption
>> (fasttop): 0x0000000001d15420 ***
>> zebra[21794]: snmp[info]: AgentX master disconnected us, reconnecting in
>> 15
>>
>> So when this happens, the master attempts to close_agentx_session().
>> My best guess (without being able to recreate this) is that this structure
>> is freed here first.   Is it possible to somehow protect the session
>> (and all subsessions) from being freed if there the master is in the
>> process
>> of allocating  netsnmp_delegated_cache?
>>
>> Can we set something like AGENTX_MSG_CLEANUPSET if we have
>> disconnected (or timed out) any/all subagents?  This is just to prevent
>> the
>> double free that happens at the end of agentx_master_handler().
>>
>> Thanks,
>> Sam
>>
>>
>>
>>
>
>

-- 
*Sam Tannous*
Engineering
Cumulus Networks®
+1 650 383 6700 x 1106
<http://www.cumulusnetworks,com>www.cumulusnetworks.com

Evaluate Cumulus® Linux®
https://cumulusnetworks.com/product/secure/evaluate/

Become a Partner
http://cumulusnetworks.com/partners/become-a-partner/

_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders