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: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/6346>

1 files 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;
 }