Fix heap-use-after-free when processing incoming RTP/RTCP after stream destroy

[nanangizz]

As reported by ASan:

==10715==ERROR: AddressSanitizer: heap-use-after-free on address 0x52500003b168 at pc 0x7f0066e7d96f bp 0x7f0062ffcb00 sp 0x7f0062ffc2a8
READ of size 19 at 0x52500003b168 thread T1
    #0 0x7f0066e7d96e in strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:391
    #1 0x55aaa84c67df in pj_log ../src/pj/log.c:406
    #2 0x55aaa84c7a36 in pj_log_5 ../src/pj/log.c:562
    #3 0x55aaa7f92355 in parse_rtcp_bye ../src/pjmedia/rtcp.c:830
    #4 0x55aaa7f92906 in pjmedia_rtcp_rx_rtcp ../src/pjmedia/rtcp.c:915
    #5 0x55aaa7f0894d in on_rx_rtcp ../src/pjmedia/stream_imp_common.c:384
    #6 0x55aaa7f4f2be in srtp_rtcp_cb ../src/pjmedia/transport_srtp.c:1768
    #7 0x55aaa7f53017 in call_rtcp_cb ../src/pjmedia/transport_udp.c:535
    #8 0x55aaa7f53ba6 in on_rx_rtcp ../src/pjmedia/transport_udp.c:728
    #9 0x55aaa84ade9c in ioqueue_dispatch_read_event ../src/pj/ioqueue_common_abs.c:686
    #10 0x55aaa84b3d5b in pj_ioqueue_poll ../src/pj/ioqueue_select.c:1093
    #11 0x55aaa7eca4d5 in worker_proc ../src/pjmedia/endpoint.c:352
    #12 0x55aaa84a5c9d in thread_main ../src/pj/os_core_unix.c:765
    #13 0x7f0066e5ea41 in asan_thread_start ../../../../src/libsanitizer/asan/asan_interceptors.cpp:234
    #14 0x7f006609caa3 in start_thread nptl/pthread_create.c:447
    #15 0x7f0066129c6b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

0x52500003b168 is located 6248 bytes inside of 8000-byte region [0x525000039900,0x52500003b840)
freed by thread T3 here:
    #0 0x7f0066efc4d8 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x55aaa84fa736 in default_block_free ../src/pj/pool_policy_malloc.c:77
    #2 0x55aaa84ca926 in reset_pool ../src/pj/pool.c:278
    #3 0x55aaa84caa6c in pj_pool_destroy_int ../src/pj/pool.c:318
    #4 0x55aaa84cbb4c in cpool_release_pool ../src/pj/pool_caching.c:254
    #5 0x55aaa84c9bfe in pj_pool_release ../include/pj/pool_i.h:155
    #6 0x55aaa84c9c5b in pj_pool_safe_release ../include/pj/pool_i.h:164
    #7 0x55aaa7f0ae9a in on_destroy ../src/pjmedia/stream_imp_common.c:678
    #8 0x55aaa84c4ce2 in grp_lock_destroy ../src/pj/lock.c:397
    #9 0x55aaa84c55f5 in grp_lock_dec_ref ../src/pj/lock.c:563
    #10 0x55aaa84c5670 in pj_grp_lock_dec_ref ../src/pj/lock.c:654
    #11 0x55aaa7f1c57c in pjmedia_stream_destroy ../src/pjmedia/stream.c:2595
   ...

This is perhaps related to the new behavior introduced by #4721, media transport detach is now asynchronous.

[Copilot]

Pull request overview

This PR attempts to prevent a heap-use-after-free when late incoming RTCP/RTP packets are processed after pjmedia_stream_destroy() (likely exposed by the async-callback behavior enabled by #4721).

Changes:

• Modifies the common stream destroy handler to defer stream cleanup by registering on_destroy() onto the transport’s grp_lock.
• Removes the direct transport grp_lock deref previously done during stream cleanup.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pjmedia/src/pjmedia/stream_imp_common.c:675

• on_destroy() ultimately releases c_strm->own_pool. With the stream now registering on_destroy on the transport grp_lock, this cleanup won’t run until the transport grp_lock is destroyed, so the stream’s pool/name strings remain alive long after pjmedia_stream_destroy() returns. If this is intentional to avoid the callback race, it would be good to restructure so the heavy stream teardown happens during pjmedia_stream_destroy() and only the final pool release is deferred, to avoid holding codec/JB resources for the lifetime of the transport.

    /* Call specific stream destroy handler. */
    on_stream_destroy(arg);

    /* Free mutex */
    if (c_strm->jb_mutex) {
        pj_mutex_destroy(c_strm->jb_mutex);
        c_strm->jb_mutex = NULL;
    }

    /* Destroy jitter buffer */
    if (c_strm->jb) {
        pjmedia_jbuf_destroy(c_strm->jb);
        c_strm->jb = NULL;
    }

    /* Destroy media synchronizer */
    if (c_strm->av_sync && c_strm->av_sync_media)
        pjmedia_stream_common_set_avsync(c_strm, NULL);

#if TRACE_JB
    if (TRACE_JB_OPENED(c_strm)) {
        pj_file_close(c_strm->trace_jb_fd);
        c_strm->trace_jb_fd = TRACE_JB_INVALID_FD;
    }
#endif

    PJ_LOG(4,(c_strm->port.info.name.ptr, "Stream destroyed"));
    pj_pool_safe_release(&c_strm->own_pool);
}

[sauwming]

It seems like we're just going in circles. We have used this approach before: #4184

[sauwming]

IMO I think we should sync ioqueue_dispatch_read_event_no_lock() and pj_ioqueue_clear_key(), to make sure that callbacks are not being executed.

[nanangizz]

It seems like we're just going in circles. We have used this approach before: #4184

Oops, I forgot we've been there before.

[nanangizz]

IMO I think we should sync ioqueue_dispatch_read_event_no_lock() and pj_ioqueue_clear_key(), to make sure that callbacks are not being executed.

Good idea. Adding busy waiting in pj_ioqueue_clear_key(), or any other ideas?
Btw, IIRC ICE destroy is always async, for deallocating TURN etc, this can be another issue.

[sauwming]

Yes, I suppose if read_callback_thread is not NULL, we need to wait.