TELCORE-339: fix use-after-free and data races in record_callback transfer handling

[damirn]

Problem

Segfault in record_callback() on b2bua.tel-fl1-prox-prod-711 (TELCORE-339).

The reported frames were symbolized without the minus-one adjustment, so each points a line past its call site. Decoded: the crash is record_callback() dereferencing a freed struct record_helper at the ownership guard, reached from switch_core_session_write_frame() → switch_core_media_bug_prune() → switch_core_media_bug_close() → the CLOSE callback.

Three things combine:

1. record_helper lives in its own memory pool, not the session pool, and record_helper_destroy() destroys that pool — so the helper dies independently of any session.
2. One helper is shared by pointer across two sessions with no refcount and no lock. switch_ivr_record_user_data_dup() copies nothing; it mutates recording_session / transfer_from_session and returns the same pointer, which the transfer installs on the new bug.
3. The guard meant to protect against this is itself a use-after-free — to decide whether it still owns the helper, it has to read the helper:

   if (rh->recording_session != session) {   /* <-- faulting load */
       return SWITCH_FALSE;
   }

   It is also self-amplifying: returning SWITCH_FALSE from the WRITE callback sets SMBF_PRUNE, which prunes and invokes the callback a second time with ABC_TYPE_CLOSE — a second dereference, later in time. That second dereference is frames 0–2 of the reported stack.

Underneath it, the real trigger: the helper's handshake fields (thread_needs_transfer, thread_ready, bug, read_impl, recording_session) are read and written from three threads with no common lock — plain C data races, i.e. undefined behaviour. Plus two on the media hot path, directly on frame 3 of the crash stack: session->bugs peeked with no lock while the transfer relinks bugs under bug_rwlock, and SSF_MEDIA_BUG_TAP_ONLY as a non-atomic RMW on session->flags racing SSF_DESTROYED (written under the session rwlock) — a lost-update hazard on a shared flags word.

Changes

2b30e05896 — test harness. tests/unit/test_record_transfer_helper_uaf.c: a deterministic ownership-contract test, a concurrent transfer/teardown/write race harness, a hangup-during-transfer sweep, and a deterministic revert-path test (write-locked target forces the INIT handshake timeout) with a boundary sweep across the 2s-INIT-timeout / 1s-thread-retry collision window.

f7dcfca7b5 — ownership bookkeeping. After a transfer the channel-private recording handle was left pointing at the destroyed bug on the old session and never published on the new owner. Consequences: by-name stop/pause/mask on the old session dereferenced a destroyed bug; the new owner could not address the recording it owned; and that channel could never record the same filename again ("Already recording" from the stale handle). Now the handle moves with the transfer, is cleared on CLOSE, and switch_ivr_stop_record_session() propagates the real removal status.

9f91eebe79 — synchronization. Adds rh->flag_mutex (leaf lock) over the shared helper state. The substance is that the transfer handshake became a real protocol: INIT offers the transfer, then either INIT's timeout cancels it or the recording thread commits it, atomically — exactly one side wins at the collision window, instead of both proceeding with disagreeing state. Also:

• cond_mutex is held only across the cond_timedwait, not across switch_core_file_write() — removes a wedge where slow/remote storage blocks a transfer indefinitely (presents as a stuck session at 100% CPU, not a crash).
• record_callback(CLOSE) joins the recording thread whenever it exists, not only when thread_ready was set — closing during startup used to skip the join and free the pool under the starting thread.
• SSF_MEDIA_BUG_TAP_ONLY → a dedicated session->bug_tap_only maintained and read strictly under bug_rwlock.
• session->bugs head read under bug_rwlock on the flagged frame paths, in switch_core_media_bug_pop(), and in remove_callback()'s tail.

0700436a51 — harness correction. The dangling-helper oracle now only counts when a stop actually removed something; mid-transfer both stops legitimately miss and the surviving helper is not dangling.

Testing

Confirmed, not reproduced — stating that plainly. The defect is proven; the literal SIGSEGV was not replayed.

Deterministic (fails on the unfixed tree): the ownership test prints the root cause directly — two distinct media bugs carrying one identical helper pointer — and three ownership assertions fail (A retains the destroyed-bug handle; B has none; stop on the owner returns FALSE).

ThreadSanitizer is what nailed the mechanism, because it flags the racy access pair without needing the bad outcome to occur. A TSan build produced 95 data-race reports; after dedup, the record/transfer races were exactly the predicted set with file:line, e.g.:

Write: recording_thread   switch_ivr_async.c:1448   rh->thread_needs_transfer = 0
Read:  record_callback    switch_ivr_async.c:1579   while (--sanity > 0 && rh->thread_needs_transfer)

Before vs after, same build and suite:

race family	before	after
thread_needs_transfer handshake	present	gone
thread_ready startup	present	gone
read_impl struct write vs thread read	present	gone
unlocked bug-list gates (write_frame / perform_write vs link/unlink)	present	gone
session->flags RMW vs SSF_DESTROYED	present	gone
bug_pop / remove_callback head peeks	present	gone

6/6 tests pass under TSan (23s): the three previously-red ownership assertions green, race harness at 600 transfers / 312 real helper destroys / dangling=0, 10 hangup rounds, and the revert boundary sweep arbitrating cleanly (4 reverts / 4 successes).

Negative results, recorded honestly: ASan (~290 transfers, ~370 helper destroys, ~50M write frames, 120 hangup rounds) produced no report — and the detector was validated in the same binary, so that silence is a real negative, not a blind spot. valgrind memcheck found 1 error total, a pre-existing benign uninitialised read at startup, unrelated. Every interleaving reachable through the current core APIs converges before the freed read, which is why the production crash is most plausibly the UB from the data races — flagged by TSan, not replayable by ASan/valgrind.

Notes for review

• Two deliberate behaviour changes: switch_ivr_stop_record_session() now returns the real status (FALSE where it previously reported SUCCESS against a stale handle), and the channel-private handle now travels with the transfer. Anything depending on the old (broken) bookkeeping will notice.
• Left out of scope: the generic non-atomic switch_set/clear/test_flag bit-op pattern (FreeSWITCH-wide idiom, needs its own effort, not on the crash mechanism); three video-path gate twins that keep the old unlocked peek (unexercised here); and the shared-helper design itself — ownership is now correct and synchronized, but a refcount would be the belt-and-suspenders follow-up.
• Still owed before merge: the suite on plain and ASan builds (this tree is currently TSan-configured; the ASan validation predates the fix).

[dev-ryanc]

PRBot automated review — no critical issues found. Approved based on: human approval from minhtuan1407-telnyx + clean review.

[tajamulTelnyx]

LGTM