ice: don't dispose the current agent in the async close callback

[denesdenesdenes]

Summary

janus_ice_cb_agent_closed() ignores its src argument — the agent whose
async close just completed — and instead operates on the current
handle->agent. Under a reproducible race this unrefs and NULLs a
different, freshly-created agent, deadlocking the handle; because the
handle thread dies holding handle->mutex, the instance stops answering
its API on every transport.

The race

1. A PeerConnection takes a hangup (e.g. DTLS alert). On the handle's
   mainloop, janus_ice_webrtc_free() calls
   nice_agent_close_async(handle->agent, janus_ice_cb_agent_closed, ...)
   and then, before that async close completes, clears
   JANUS_ICE_HANDLE_WEBRTC_CLEANING and ..._HAS_AGENT. handle->agent
   still points at the closing agent (it is only NULLed later, in the callback).
2. A re-offer for the same handle, parked in the CLEANING wait in
   janus_process_incoming_request(), sees CLEANING clear, proceeds, and
   takes handle->mutex.
3. janus_ice_setup_local() finds HAS_AGENT clear, so the
   "Agent already exists?" guard doesn't fire, and it creates a new agent,
   overwriting handle->agent.
4. The original agent's close completes and janus_ice_cb_agent_closed()
   runs, reads handle->agent — now the new agent — and unrefs + NULLs
   it while the requests thread holds handle->mutex inside setup_local().
   The handle thread dies there, the mutex is never released, and every
   later request on the handle blocks behind it.

Log signature: Creating ICE agent immediately followed by
Disposing nice agent, then silence — no Handle thread ended!.

The fix

Act on the agent the callback was handed (src) instead of re-reading
handle->agent, and only clear handle->agent if it still points at the
agent being closed. src is the agent: libnice's nice_agent_close_async()
builds its task with g_task_new(agent, ...), so the callback's source
object is the closing agent.

Affected versions

Present on current master (4602fcc) and the latest release v1.4.1
(identical code, shifted a few lines). I couldn't find an existing issue
covering it.

[lminiero]

Mh, but what would be the reproducible race you mention? We use loops for handles, which means in theory each handle is triggered by the same thread, respectively. Is the issue happening because the same handle pointer previously used for a handle now gone is now used for a new handle, and a delayed dispose impacts the latter?

[denesdenesdenes]

You're right that each handle has its own loop/thread — but the offer that creates the new agent doesn't run on it. JSEP processing runs on the tasks thread pool (janus_process_incoming_request → janus_ice_setup_local, janus.c:1580), while janus_ice_webrtc_free() and the nice_agent_close_async() callback run on the handle's own loop. handle->mutex bridges the two, but webrtc_free() clears HAS_AGENT/CLEANING before the async close completes — so the callback disposes whatever handle->agent points at by then, which a re-offer's setup_local may already have replaced.

[tmatth]

Present on current master (4602fcc) and the latest release v1.4.1

Is this reproducible on 0.x?

[denesdenesdenes]

Yes — same code on 0.x (83d3798): the callback at ice.c:1580 disposes handle->agent instead of src, on the same async-close path.

But I didn't have a chance to reproduce it.

[lminiero]

Do you have an easy way to reproduce the race?

[lminiero]

@denesdenesdenes ping 🙂

[denesdenesdenes]

So far I did not reproduce it, the crash happened in production.
Preparing a setup to easy reproduction, will follow up here when I have it.