Race in clear_ice(): engine->ice_in zeroed without ice_mutex, NULL con_addr crashes handle_ice()

[Yasholma]

Describe the bug

clear_ice() in src/switch_core_media.c zeroes engine->ice_in without holding
the RTP session's ice_mutex, while the RTP receive thread reads that exact
memory under ice_mutex in handle_ice(). The reader can therefore observe a
half-cleared candidate table and dereference a NULL con_addr, segfaulting at
address 0 inside libc's strcmp.

Writer — src/switch_core_media.c, clear_ice(), holding no lock:

engine = &smh->engines[type];

engine->ice_in.chosen[0] = 0;
...
engine->ice_in.cand_idx[0] = 0;
engine->ice_in.cand_idx[1] = 0;
memset(&engine->ice_in, 0, sizeof(engine->ice_in));   /* <-- unlocked */
engine->remote_rtcp_port = 0;

if (engine->rtp_session) {
    switch_rtp_reset(engine->rtp_session);
}

Reader — src/switch_rtp.c, handle_ice(), holding rtp_session->ice_mutex:

for (i = 0; i < ice->ice_params->cand_idx[ice->proto]; i++) {
    if (!strcmp(ice->ice_params->cands[i][ice->proto].con_addr, from_host) &&
        ice->ice_params->cands[i][ice->proto].con_port == from_port) {

ice->ice_params is &engine->ice_in, so the two touch the same object. Crucially
con_addr is declared char * (src/include/switch_rtp.h), so the memset
sets it to NULL rather than to an empty string, and strcmp(NULL, from_host)
faults.

The interleaving required is: the reader evaluates cand_idx[proto] as non-zero,
then the writer's memset lands, then the reader dereferences
cands[i][proto].con_addr — now NULL.

switch_rtp_reset(), called from clear_ice(), also mutates rtp_session->ice
outside the lock. Same path, smaller exposure.

Trigger path

verto.attach → switch_core_media_clear_ice() → clear_ice(), i.e. a client
re-attaching to an existing session while media is flowing. Any caller of
switch_core_media_clear_ice() on a live session with an active RTP thread can
hit it.

Expected behavior

clear_ice() should hold the same ice_mutex that handle_ice() holds before
mutating the candidate tables, so the reader never observes a partially cleared
ice_in.

Package version or git hash

1.11.1-release. The code is unchanged on current master, and identical in
v1.10.10 and v1.10.12, so this is long-standing rather than a regression.

Impact

Observed twice in five days on production WebRTC media nodes carrying ~150
concurrent channels, each occurrence dropping every call on the affected node
(57 and 68 calls respectively). Because the faulting thread dies holding
ice_mutex, other threads touching that session block behind it and the process
limps for several minutes before exiting, rather than dying immediately.

Trace logs / backtrace from core file

I have a core dump but the distro libc is stripped in the region the top frame
falls in, so I cannot provide a usefully symbolised thread apply all bt — the
top frame resolves only to libc.so.6 + 0x167132, which sits in the IFUNC string
-routine area. What I can state from the crash handler output:

Received signal 11 SEGV_MAPERR 000000000000

Fault address is exactly 0, consistent with strcmp(NULL, ...).

Happy to attempt a better backtrace if there is a preferred way to get symbols
for the string routines.

Suggested fix

Same shape as #2915 ("[core] Fix - add missing ice_mutex to protect dtls"): export
lock helpers from switch_rtp.c (the mutex is SWITCH_MUTEX_NESTED, so
recursion is safe) and take the lock across clear_ice(), which also covers the
switch_rtp_reset() exposure.

SWITCH_DECLARE(void) switch_rtp_ice_lock(switch_rtp_t *rtp_session)
{
	if (rtp_session && rtp_session->ice_mutex) {
		switch_mutex_lock(rtp_session->ice_mutex);
	}
}

SWITCH_DECLARE(void) switch_rtp_ice_unlock(switch_rtp_t *rtp_session)
{
	if (rtp_session && rtp_session->ice_mutex) {
		switch_mutex_unlock(rtp_session->ice_mutex);
	}
}

then in clear_ice():

	engine = &smh->engines[type];

	if (engine->rtp_session) {
		switch_rtp_ice_lock(engine->rtp_session);
	}

	engine->ice_in.chosen[0] = 0;
	...
	if (engine->rtp_session) {
		switch_rtp_reset(engine->rtp_session);
		switch_rtp_ice_unlock(engine->rtp_session);
	}

I'm happy to open a PR with this if the approach looks right. One thing worth a
maintainer's eye: this introduces a new acquisition of ice_mutex from
switch_core_media.c. I found no lock-order inversion on the verto.attach
path, but I don't have full visibility of every caller of
switch_core_media_clear_ice().

[Yasholma]

I now have a deterministic reproduction of this, plus a before/after that confirms the proposed fix removes it. Posting the detail in case it is useful for review — and because the timing turned out to be less obvious than I expected.

Reproduction

Two temporary patches against 1.11.1, in an integration environment driving a real WebRTC call:

1. In handle_ice(), immediately after cand_idx[proto] is read as non-zero and before the strcmp, park the reader until signalled.
2. In clear_ice(), signal that parked reader immediately after memset(&engine->ice_in, 0, ...).

Then: establish a call, park the reader, and trigger verto.attach (a network-switch reattach) so clear_ice() runs while the reader is inside the loop.

Unpatched — segfaults on most runs (three of three in the latest matrix; about two in three overall):

armed      ice_params=0x7fff8003d160 cand_idx=2
clear_ice  ice_in=0x7fff8003d160
resuming after 1329 ms (released=1) cand_idx=0 con_addr=(nil)
Received signal 11 SEGV_MAPERR 000000000000

ice_params on the reader and ice_in on the writer are the same address, and con_addr is NULL at the point of dereference. The process exits 139, matching the two production crashes that led me here.

With the lock added — survives:

armed      ice_params=0x7fff54070980 cand_idx=4
resuming after 15000 ms (released=0) cand_idx=4 con_addr=0x7fff5406f3e8
clear_ice  ice_in=0x7fff54070980

released=0 is the interesting part: clear_ice() never signalled across the reader's full 15s hold because it was blocked on ice_mutex, and it logs at the same instant the reader resumes. con_addr is valid rather than NULL.

Two things that made this hard to trigger, which may matter to anyone else trying

A longer injected window makes it less likely to fire, not more. With a fixed sleep, the reader wakes ~1.4s after the memset and check_ice() has already repopulated cands[], so con_addr is valid again and nothing faults. The reader has to reach the dereference while the array is still zeroed — hence signalling on the memset rather than sleeping.

Disconnecting the client stops the consent-freshness STUN, so a reader armed after the disconnect cannot park until the new connection is up — by which time clear_ice() has already run. The arm has to be placed while media is still flowing.

Neither is a property of the bug itself, only of trying to hit it on purpose. In production the window is sub-microsecond and hit by chance; we saw it three times in eleven days across four nodes carrying ~150 concurrent channels each, hitting every node that carries WebRTC media.

Regression

The full integration suite (254 tests, including reattach, DTLS, TURN-relay and ICE-gathering paths) passes on the patched build with no deadlock or hang. That is evidence rather than proof, and the lock-ordering question in my original post still stands — I would value a maintainer's eye on whether taking ice_mutex from switch_core_media.c is safe across every caller of switch_core_media_clear_ice().

Happy to open a PR with the fix if the approach looks right.

---

A PR with this fix is now open: #3178.