mirrors/gstreamer
mirrors/gstreamer
1
1
Fork 0
mirror of https://gitlab.freedesktop.org/gstreamer/gstreamer.git synced 2024-12-01 22:21:13 +00:00
Code Issues 1 Projects Releases Wiki Activity

rtspsrc: Don't invoke close when stopping if we've started cleanup

Browse source 
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/6330>
This commit is contained in:
Nirbheek Chauhan 2024-01-31 00:32:22 +05:30 committed by GStreamer Marge Bot
parent 18399d9fac
commit bcb016d6d2
1 changed files with 9 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
subprojects/gst-plugins-good/gst/rtsp/gstrtspsrc.c
View file

@ -8630,9 +8630,8 @@ close:
} }
/* cleanup */ /* cleanup */
gst_rtspsrc_cleanup (src);
src->state = GST_RTSP_STATE_INVALID; src->state = GST_RTSP_STATE_INVALID;
gst_rtspsrc_cleanup (src);
if (async) if (async)
gst_rtspsrc_loop_end_cmd (src, CMD_CLOSE, res); gst_rtspsrc_loop_end_cmd (src, CMD_CLOSE, res);
@ -9556,8 +9555,10 @@ gst_rtspsrc_stop (GstRTSPSrc * src)
GST_DEBUG_OBJECT (src, "stopping"); GST_DEBUG_OBJECT (src, "stopping");
/* also cancels pending task */ /* If we've already started cleanup, we only need to stop the task */
gst_rtspsrc_loop_send_cmd (src, CMD_WAIT, CMD_ALL); 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); GST_OBJECT_LOCK (src);
if ((task = src->task)) { if ((task = src->task)) {
@ -9580,8 +9581,10 @@ gst_rtspsrc_stop (GstRTSPSrc * src)
} }
GST_OBJECT_UNLOCK (src); GST_OBJECT_UNLOCK (src);
/* ensure synchronously all is closed and clean */ /* ensure synchronously all is closed and clean if we haven't started
gst_rtspsrc_close (src, FALSE, TRUE); * cleanup yet */
if (src->state != GST_RTSP_STATE_INVALID)
gst_rtspsrc_close (src, FALSE, TRUE);
return TRUE; return TRUE;
} }