GNOME Bugzilla – Bug 773532
The segfault happens when many clients work with rtsp server with GST_RTSP_LOWER_TRANS_TCP option on.
Last modified: 2018-11-03 15:41:03 UTC
Created attachment 338506 [details] stacktrace I've added a stacktrace of segfault in attachment. As you can see from it, the segfault happens inside "do_send_data" function mutex lock, and "do_send_data" function calls from "gst_rtsp_stream_transport_send_rtp" function. So the reason of segfault is that GstRTSPClient finalizing happens before or in the time of "do_send_data" work. So "do_send_data" uses already destroyed client mutex. I've made a patch to fix that problem. Please review it. P.S. To reproduce this bug I took rtsp server from examples (test-readme.c) and add next line to it: gst_rtsp_media_factory_set_protocols (factory, GST_RTSP_LOWER_TRANS_TCP); Then I used many clients that were connected/disconnected simultaneously. Also I added sleep(1) inside "gst_rtsp_stream_transport_send_rtp" to reproduce it more often.
Created attachment 338507 [details] [review] patch
Review of attachment 338507 [details] [review]: This should probably be solved differently somehow, either by using a mutex or by ensure that the callback is removed early enough. ::: gst/rtsp-server/rtsp-stream-transport.c @@ +208,3 @@ + if (priv->user_data) { + priv->weak_user_data = g_slice_new (GWeakRef); + g_weak_ref_init (priv->weak_user_data, priv->user_data); GWeakRef only works on GObjects, this can't work here (also allocate it directly inside the struct instead of using another g_new() here)
Created attachment 338601 [details] [review] new patch > GWeakRef only works on GObjects, this can't work here (also allocate it > directly inside the struct instead of using another g_new() here) Why not? We have 3 cases of using "gst_rtsp_stream_transport_set_callbacks": 1) gst_rtsp_stream_transport_set_callbacks (trans, (GstRTSPSendFunc) do_send_data, (GstRTSPSendFunc) do_send_data, client, NULL); with GstRTSPClient 2) gst_rtsp_stream_transport_set_callbacks (context->stream_transport, GstRTSPSendFunc) do_send_data, GstRTSPSendFunc) do_send_data, context, NULL); with GstRTSPStreamContext and 3) gst_rtsp_stream_transport_set_callbacks (trans, NULL, NULL, NULL, NULL); GstRTSPClient and GstRTSPStreamContext are both GObjects, NULL is just a NULL. I've added an assertion in new patch that checks that user_data is a GObject. And I've changed the allocator as you said. > This should probably be solved differently somehow, either by using a mutex > or by ensure that the callback is removed early enough. I thought about unsetting callbacks, but I don't know how to guarantee that "do_send_data" won't be in progress (even after unsetting callbacks) at the time of client finalizing. And if mutex should be used, where should it be placed? Can we protect send_rtp, send_rtcp and set_callbacks functions by mutex? Will it work fast enough?
Created attachment 338727 [details] [review] new-patch-2 > (also allocate it > directly inside the struct instead of using another g_new() here) Sorry, I've just read it again. New patch with ok allocated GWeakRef was added.
Review of attachment 338727 [details] [review]: I think the only way how to fix it without breaking API would be to protect the callbacks/user_data usage with a mutex everywhere... which is less than ideal as we would have to keep the mutex locked while sending out data ::: gst/rtsp-server/rtsp-stream-transport.c @@ +196,2 @@ g_return_if_fail (GST_IS_RTSP_STREAM_TRANSPORT (trans)); + g_return_if_fail (user_data == NULL || G_IS_OBJECT (user_data)); This breaks API though, and conventions about user_data/destroy_notify
Created attachment 338835 [details] [review] remove callbacks in client finalizing function and protect it by mutex I thought that GstRTSPStreamTransport could be shared somehow between clients, because we have shared media, so that the reason why I've tried to not use mutex on it. But I found that there is just one GstRTSPSessionMedia for client, which keeps transports, so solution with mutex should work fast enough. Ok. Looks like it is the best solution for now. Please review the new patch. I've removed callbacks in client finalizing function and protected it by mutex inside GstRTSPStreamTransport.
ping
There are two patches here, one being marked as needs-work. Is that one obsolete and only the new one is needed now?
Review of attachment 338835 [details] [review]: ::: gst/rtsp-server/rtsp-stream-transport.c @@ +463,2 @@ priv = trans->priv; + g_mutex_lock (&priv->lock); Keeping this mutex locked all the time while sending data to the server seems suboptimal, but I also don't see any other way right now.
(In reply to Sebastian Dröge (slomo) from comment #8) > There are two patches here, one being marked as needs-work. Is that one > obsolete and only the new one is needed now? The new-patch-2 is obsolete. (In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 338835 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-stream-transport.c > @@ +463,2 @@ > priv = trans->priv; > + g_mutex_lock (&priv->lock); > > Keeping this mutex locked all the time while sending data to the server > seems suboptimal, but I also don't see any other way right now. Thanks, to reply.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-rtsp-server/issues/31.