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 <stann...@cumulusnetworks.com>
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 <stann...@cumulusnetworks.com>
> 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
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
