From acd40e7852c5e2bf188159466009824aebc0bb3b Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Wed, 31 Jan 2024 00:32:22 +0530 Subject: [PATCH] rtspsrc: Don't invoke close when stopping if we've started cleanup When we're doing a state change from PLAYING to NULL, first we invoke gst_rtspsrc_loop_send_cmd_and_wait (..., CMD_CLOSE, ...) during PAUSED_TO_READY which will schedule a TEARDOWN to happen async on the task thread. The task thread will call gst_rtspsrc_close(), which will send the TEARDOWN and once it's complete, it will call gst_rtspsrc_cleanup() without taking any locks, which frees src->streams. At the same time however, the state change in the app thread will progress further and in READY_TO_NULL it will call gst_rtspsrc_stop() which calls gst_rtspsrc_close() a second time, which accesses src->streams (without a lock again), which leads to simultaneous access of src->streams, and a segfault. So the state change and the cleanup are racing, but they almost always complete sequentially. Either the cleanup sets src->streams to NULL or _stop() completes first. Very rarely, _stop() can start while src->streams is being freed in a for loop. That causes the segfault. This is unlocked access is unfixable with more locking, it just leads to deadlocks. This pattern has been observed in rtspsrc a lot: state changes and cleanup in the element are unfixably racy, and that foundational issue is being addressed separately via a rewrite. The bandage fix here is to prevent gst_rtspsrc_stop() from accessing src->streams after it has already been freed by setting src->state to INVALID. Part-of: --- .../gst-plugins-good/gst/rtsp/gstrtspsrc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/subprojects/gst-plugins-good/gst/rtsp/gstrtspsrc.c b/subprojects/gst-plugins-good/gst/rtsp/gstrtspsrc.c index fec122ce45..289582d78b 100644 --- a/subprojects/gst-plugins-good/gst/rtsp/gstrtspsrc.c +++ b/subprojects/gst-plugins-good/gst/rtsp/gstrtspsrc.c @@ -8552,9 +8552,8 @@ close: } /* cleanup */ - gst_rtspsrc_cleanup (src); - src->state = GST_RTSP_STATE_INVALID; + gst_rtspsrc_cleanup (src); if (async) gst_rtspsrc_loop_end_cmd (src, CMD_CLOSE, res); @@ -9478,8 +9477,10 @@ gst_rtspsrc_stop (GstRTSPSrc * src) GST_DEBUG_OBJECT (src, "stopping"); - /* also cancels pending task */ - gst_rtspsrc_loop_send_cmd (src, CMD_WAIT, CMD_ALL); + /* If we've already started cleanup, we only need to stop the task */ + if (src->state != GST_RTSP_STATE_INVALID) + /* also cancels pending task */ + gst_rtspsrc_loop_send_cmd (src, CMD_WAIT, CMD_ALL); GST_OBJECT_LOCK (src); if ((task = src->task)) { @@ -9502,8 +9503,10 @@ gst_rtspsrc_stop (GstRTSPSrc * src) } GST_OBJECT_UNLOCK (src); - /* ensure synchronously all is closed and clean */ - gst_rtspsrc_close (src, FALSE, TRUE); + /* ensure synchronously all is closed and clean if we haven't started + * cleanup yet */ + if (src->state != GST_RTSP_STATE_INVALID) + gst_rtspsrc_close (src, FALSE, TRUE); return TRUE; }