[Bug 773532] New: The segfault happens when many clients work with rtsp server with GST_RTSP_LOWER_TRANS_TCP option on.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Bug ID: 773532
Summary: The segfault happens when many clients work with rtsp
server with GST_RTSP_LOWER_TRANS_TCP option on.
Classification: Platform
Product: GStreamer
Version: 1.8.2
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-rtsp-server
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

Created attachment 338506
--> https://bugzilla.gnome.org/attachment.cgi?id=338506&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

--- Comment #1 from Kseniya Vasilchuk <***@gmail.com> ---
Created attachment 338507
--> https://bugzilla.gnome.org/attachment.cgi?id=338507&action=edit
patch

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Kseniya Vasilchuk <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338507|none |needs-work
status| |

--- Comment #2 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 338507:
--> (https://bugzilla.gnome.org/review?bug=773532&attachment=338507)

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)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Kseniya Vasilchuk <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338507|needs-work |none
status| |
Attachment #338507|0 |1
is obsolete| |

--- Comment #3 from Kseniya Vasilchuk <***@gmail.com> ---
Created attachment 338601
--> https://bugzilla.gnome.org/attachment.cgi?id=338601&action=edit
new patch

Post by "GStreamer" (GNOME Bugzilla)
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.

Post by "GStreamer" (GNOME Bugzilla)
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?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Kseniya Vasilchuk <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@zen.co.uk
Attachment #338601|0 |1
is patch| |
Attachment #338601|application/mbox |text/plain
mime type| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Kseniya Vasilchuk <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338601|0 |1
is obsolete| |

--- Comment #4 from Kseniya Vasilchuk <***@gmail.com> ---
Created attachment 338727
--> https://bugzilla.gnome.org/attachment.cgi?id=338727&action=edit
new-patch-2

Post by "GStreamer" (GNOME Bugzilla)
(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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338727|none |needs-work
status| |

--- Comment #5 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 338727:
--> (https://bugzilla.gnome.org/review?bug=773532&attachment=338727)

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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

--- Comment #6 from Kseniya Vasilchuk <***@gmail.com> ---
Created attachment 338835
--> https://bugzilla.gnome.org/attachment.cgi?id=338835&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

--- Comment #7 from Kseniya Vasilchuk <***@gmail.com> ---
ping

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
There are two patches here, one being marked as needs-work. Is that one
obsolete and only the new one is needed now?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338835|none |reviewed
status| |

--- Comment #9 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 338835:
--> (https://bugzilla.gnome.org/review?bug=773532&attachment=338835)

::: 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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

--- Comment #10 from Kseniya Vasilchuk <***@gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #8)

Post by "GStreamer" (GNOME Bugzilla)
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)

Post by "GStreamer" (GNOME Bugzilla)
::: 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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

Kseniya Vasilchuk <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #338727|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773532

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #11 from GStreamer system administrator <***@gstreamer.net> ---
-- 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.