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

src/include/private/switch_core_pvt.h

@@ -138,6 +138,12 @@ struct switch_core_session {
 	switch_queue_t *private_event_queue_pri;
 	switch_thread_rwlock_t *bug_rwlock;
 	switch_media_bug_t *bugs;
+	/* "All media bugs are native taps" hint, maintained and read under
+	 * bug_rwlock. Lives here instead of an SSF_* bit because session->flags is
+	 * mutated with non-atomic RMW under various unrelated locks - updating a
+	 * bit under bug_rwlock could concurrently clobber flags written under the
+	 * session rwlock (e.g. SSF_DESTROYED). */
+	int bug_tap_only;
 	switch_app_log_t *app_log;
 	uint32_t stack_count;
 

src/switch_core_io.c

@@ -79,7 +79,9 @@ SWITCH_DECLARE(switch_status_t) switch_core_session_read_frame(switch_core_sessi
 
 	switch_assert(session != NULL);
 
-	tap_only = switch_test_flag(session, SSF_MEDIA_BUG_TAP_ONLY);
+	switch_thread_rwlock_rdlock(session->bug_rwlock);
+	tap_only = session->bug_tap_only;
+	switch_thread_rwlock_unlock(session->bug_rwlock);
 
 	switch_os_yield();
 
@@ -417,21 +419,33 @@ SWITCH_DECLARE(switch_status_t) switch_core_session_read_frame(switch_core_sessi
 
 
 		goto done;
-	} else if (session->bugs && !need_codec) {
-		do_bugs = 1;
-		need_codec = 1;
+	} else if (!need_codec) {
+		switch_thread_rwlock_rdlock(session->bug_rwlock);
+		if (session->bugs) {
+			do_bugs = 1;
+			need_codec = 1;
+		}
+		switch_thread_rwlock_unlock(session->bug_rwlock);
 	}
 
 	if (switch_test_flag(*frame, SFF_CNG)) {
-		if (!session->bugs && !session->plc) {
+		int have_bugs;
+
+		switch_thread_rwlock_rdlock(session->bug_rwlock);
+		have_bugs = (session->bugs != NULL);
+		switch_thread_rwlock_unlock(session->bug_rwlock);
+
+		if (!have_bugs && !session->plc) {
 			/* Check if other session has bugs */
 			unsigned int other_session_bugs = 0;
 			switch_core_session_t *other_session = NULL;
 			if (switch_channel_test_flag(switch_core_session_get_channel(session), CF_BRIDGED) &&
 				switch_core_session_get_partner(session, &other_session) == SWITCH_STATUS_SUCCESS) {
-				if (other_session->bugs && !switch_test_flag(other_session, SSF_MEDIA_BUG_TAP_ONLY)) {
+				switch_thread_rwlock_rdlock(other_session->bug_rwlock);
+				if (other_session->bugs && !other_session->bug_tap_only) {
 					other_session_bugs = 1;
 				}
+				switch_thread_rwlock_unlock(other_session->bug_rwlock);
 				switch_core_session_rwunlock(other_session);
 			}
 

src/switch_core_media.c

@@ -9566,11 +9566,13 @@ static switch_status_t perform_write(switch_core_session_t *session, switch_fram
 		}
 	}
 
-	if (session->bugs && !(frame->flags & SFF_NOT_AUDIO)) {
+	if (!(frame->flags & SFF_NOT_AUDIO)) {
 		switch_media_bug_t *bp;
 		switch_bool_t ok = SWITCH_TRUE;
 		int prune = 0;
 
+		/* The list head is only read under bug_rwlock; the old bare peek
+		 * raced link/unlink on the transfer path (TELCORE-339). */
 		switch_thread_rwlock_rdlock(session->bug_rwlock);
 
 		for (bp = session->bugs; bp; bp = bp->next) {
@@ -19335,9 +19337,16 @@ SWITCH_DECLARE(switch_status_t) switch_core_session_write_frame(switch_core_sess
 		need_codec = TRUE;
 	}
 
-	if (session->bugs && !need_codec && !switch_test_flag(session, SSF_MEDIA_BUG_TAP_ONLY)) {
-		do_bugs = TRUE;
-		need_codec = TRUE;
+	if (!need_codec) {
+		/* Gate under bug_rwlock: the bare-pointer peek raced bug link/unlink
+		 * (switch_core_media_bug_add/transfer_callback) and the tap-only flag
+		 * update; TSan flagged both pairs (TELCORE-339). */
+		switch_thread_rwlock_rdlock(session->bug_rwlock);
+		if (session->bugs && !session->bug_tap_only) {
+			do_bugs = TRUE;
+			need_codec = TRUE;
+		}
+		switch_thread_rwlock_unlock(session->bug_rwlock);
 	}
 
 	if (frame->codec->implementation->actual_samples_per_second != session->write_impl.actual_samples_per_second) {
@@ -19554,10 +19563,12 @@ SWITCH_DECLARE(switch_status_t) switch_core_session_write_frame(switch_core_sess
 
 
 
-	if (session->bugs) {
+	{
 		switch_media_bug_t *bp;
 		int prune = 0;
 
+		/* No bare peek at session->bugs: the head is only read under
+		 * bug_rwlock (raced link/unlink on the transfer path, TELCORE-339). */
 		switch_thread_rwlock_rdlock(session->bug_rwlock);
 		for (bp = session->bugs; bp; bp = bp->next) {
 			switch_bool_t ok = SWITCH_TRUE;

src/switch_core_media_bug.c

@@ -1093,15 +1093,13 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_add_ex(switch_core_session
 		}
 	}
 
+	/* Written under bug_rwlock (unlike the SSF_ flag bit it replaces, which
+	 * was set after the unlock with a non-atomic RMW on session->flags). */
+	session->bug_tap_only = tap_only;
+
 	switch_thread_rwlock_unlock(session->bug_rwlock);
 	*new_bug = bug;
 
-	if (tap_only) {
-		switch_set_flag(session, SSF_MEDIA_BUG_TAP_ONLY);
-	} else {
-		switch_clear_flag(session, SSF_MEDIA_BUG_TAP_ONLY);
-	}
-
 	if (switch_test_flag(bug, SMBF_READ_VIDEO_PATCH) && session->video_read_codec) {
 		switch_set_flag(session->video_read_codec, SWITCH_CODEC_FLAG_VIDEO_PATCHING);
 	}
@@ -1168,6 +1166,15 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_transfer_callback(switch_c
 			if ((switch_core_media_bug_add(new_session, cur->function, cur->target, cur->callback,
 										   user_data_dup_func(new_session, cur->user_data),
 										   cur->stop_time, cur->flags, &new_bug) == SWITCH_STATUS_SUCCESS)) {
+				/* Move the channel-private handle along with the bug. The old
+				 * bug is destroyed below, so a handle left on the old channel
+				 * would dangle (and block re-recording the same file there),
+				 * while the new owner would have no handle at all - making the
+				 * recording unaddressable by name (stop/pause/mask). */
+				if (!zstr(cur->target)) {
+					switch_channel_set_private(orig_session->channel, cur->target, NULL);
+					switch_channel_set_private(new_session->channel, cur->target, new_bug);
+				}
 				switch_core_media_bug_destroy(&cur);
 				total++;
 			} else {
@@ -1205,24 +1212,24 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_pop(switch_core_session_t
 {
 	switch_media_bug_t *bp;
 
-	if (orig_session->bugs) {
-		switch_thread_rwlock_wrlock(orig_session->bug_rwlock);
-		for (bp = orig_session->bugs; bp; bp = bp->next) {
-			if (!strcmp(bp->function, function)) {
-				switch_set_flag(bp, SMBF_LOCK);
-				break;
-			}
+	/* No bare peek at the list head - it is only read under bug_rwlock
+	 * (raced link/unlink on the transfer path, TELCORE-339). */
+	switch_thread_rwlock_wrlock(orig_session->bug_rwlock);
+	for (bp = orig_session->bugs; bp; bp = bp->next) {
+		if (!strcmp(bp->function, function)) {
+			switch_set_flag(bp, SMBF_LOCK);
+			break;
 		}
-		switch_thread_rwlock_unlock(orig_session->bug_rwlock);
+	}
+	switch_thread_rwlock_unlock(orig_session->bug_rwlock);
 
-		if (bp) {
-			*pop = bp;
-			return SWITCH_STATUS_SUCCESS;
-		} else {
-			*pop = NULL;
-		}
+	if (bp) {
+		*pop = bp;
+		return SWITCH_STATUS_SUCCESS;
 	}
 
+	*pop = NULL;
+
 	return SWITCH_STATUS_FALSE;
 }
 
@@ -1469,11 +1476,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove(switch_core_session
 		}
 	}
 
-	if (tap_only) {
-		switch_set_flag(session, SSF_MEDIA_BUG_TAP_ONLY);
-	} else {
-		switch_clear_flag(session, SSF_MEDIA_BUG_TAP_ONLY);
-	}
+	session->bug_tap_only = tap_only;
 
 	switch_thread_rwlock_unlock(session->bug_rwlock);
 
@@ -1531,6 +1534,7 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_callback(switch_cor
 {
 	switch_media_bug_t *cur = NULL, *bp = NULL, *last = NULL, *closed = NULL, *next = NULL;
 	int total = 0;
+	int no_bugs_left;
 
 	switch_thread_rwlock_wrlock(session->bug_rwlock);
 	if (session->bugs) {
@@ -1557,16 +1561,19 @@ SWITCH_DECLARE(switch_status_t) switch_core_media_bug_remove_callback(switch_cor
 			}
 		}
 	}
+	/* Note whether the list drained while still under the lock - the head is
+	 * only read under bug_rwlock (TELCORE-339). */
+	no_bugs_left = (session->bugs == NULL);
 	switch_thread_rwlock_unlock(session->bug_rwlock);
-	
+
 	if (closed) {
 		for (bp = closed; bp; bp = next) {
 			next = bp->next;
 			switch_core_media_bug_destroy(&bp);
 		}
 	}
 
-	if (!session->bugs && switch_core_codec_ready(&session->bug_codec)) {
+	if (no_bugs_left && switch_core_codec_ready(&session->bug_codec)) {
 		switch_core_codec_destroy(&session->bug_codec);
 	}