GNOME Bugzilla – Bug 729099
rtmpsrc: Crash due to unsafe thread handling in librtmp when setting state from PLAYING->PAUSED
Last modified: 2018-11-03 13:22:54 UTC
Created attachment 275318 [details] Log including relevant GST_DEBUG output and the backtrace I've stumbled across a crash in rtmpsrc which happens due to RTMP_Close() being called in gst_rtmp_src_unlock() while RTMP_Read() is being run in gst_rtmp_src_create() in another thread. librtmp isn't thread safe so this case is bad. The only solution I've managed to come up with is to call RTMPSockBuf_Close() in gst_rtmp_src_unlock() to close the socket and block until RTMP_Read() returns in error. This is not ideal as RTMP_Close() sends some packets out before closing the socket, which would be missed with the above solution. Hopefully there's a better solution I'm not aware of.
I was playing with this more and another option I've come up with is calling shutdown() in gst_rtmp_src_unlock() on the read side of the RTMP socket to make RTMP_Read() error out, then once RTMP_Read() has returned (by waiting with a mutex), call RTMP_Close() like it already is. This still seems like a bit of a hack, but it does let librtmp close more cleanly.
Captured in valgrind: ==2061== Thread 2 rtmpsrc0:src: ==2061== Invalid read of size 8 ==2061== at 0x326460B59C: RTMP_ReadPacket (rtmp.c:3730) ==2061== by 0x326460F676: RTMP_GetNextMediaPacket (rtmp.c:1179) ==2061== by 0x326460F787: Read_1_Packet (rtmp.c:4515) ==2061== by 0x3264610236: RTMP_Read (rtmp.c:5079) ==2061== by 0xBDC1D20: gst_rtmp_src_create (gstrtmpsrc.c:333) ==2061== by 0xC020454: gst_push_src_create (gstpushsrc.c:130) ==2061== by 0xC003DDF: gst_base_src_get_range (gstbasesrc.c:2455) ==2061== by 0xC004DC4: gst_base_src_loop (gstbasesrc.c:2731) ==2061== by 0x4CCA5AD: gst_task_func (gsttask.c:331) ==2061== by 0x4CCB689: default_func (gsttaskpool.c:68) ==2061== by 0x3ABB671147: g_thread_pool_thread_proxy (gthreadpool.c:307) ==2061== by 0x3ABB6707B4: g_thread_proxy (gthread.c:764) ==2061== Address 0x38 is not stack'd, malloc'd or (recently) free'd
It seems librtmp advertises only one means of interrupting operations in progress: Setting the global variable RTMP_ctrlC to 1, then interrupting the operation using a signal w/o restart, which will cause librtmp to error out on EINTR. Obviously, having to change global state is baaad when multiple independent rtmp elements are being used. I guess shutting down the socket at rtmp->m_sb.sb_socket is the easiest thing to do in order to unlock the streaming thread. However, as mentioned above, screwing with the socket prevents the session from being terminated cleanly.
There's also some new, GIO-based, RTMP code here: http://cgit.freedesktop.org/~ds/gst-rtmp/ This should fix all the API weirdness in librtmp and hopefully provides a more future-proof implementation.
That does look nice, and works well enough to play streams. However, given that the build system needed fixing and the amount of ERROR-level debugging messages I get, it's obviously not yet in any releasable state.
any news on this? Athanasios Oikonomou attempted to fix this a while ago https://bugzilla.gnome.org/show_bug.cgi?id=739263 but of course, it's a terrible idea to remove the unlock vfunc. the result is a deadlock when trying to interrupt prerolling of a broken stream e.g. gst-launch-1.0 playin rtmp://stream.livespotting.tv/windit-edge/LS_e0cb1_720p.stream so currently we have the choice between segfaulting when zapping to quickly or deadlocking when a stream is broken :))
I have found here is the solution: gstrtmpsrc.c ... static GstFlowReturn gst_rtmp_src_create (GstPushSrc * pushsrc, GstBuffer ** buffer) { ... GST_OBJECT_LOCK(src); while (todo > 0) { int read = RTMP_Read (src->rtmp, (char *) data, todo); ... } else { bsize += todo; todo = 0; } GST_LOG (" got size %d", read); } GST_OBJECT_UNLOCK(src); ... } ... static gboolean gst_rtmp_src_unlock (GstBaseSrc * basesrc) { ... GST_OBJECT_LOCK(rtmpsrc); if (rtmpsrc->rtmp) { RTMP_Close (rtmpsrc->rtmp); } GST_OBJECT_UNLOCK(rtmpsrc); return TRUE; } Still no failures were.
I observed the same issue, reproducible with: gst-discoverer-1.0 rtmp://184.72.239.149/vod/BigBuckBunny_115k.mov This patch does fix it, I'll attach it (with proper attribution)
Created attachment 350802 [details] [review] Fixes the reported issue
Review of attachment 350802 [details] [review]: ::: ext/rtmp/gstrtmpsrc.c @@ +351,2 @@ while (todo > 0) { int read = RTMP_Read (src->rtmp, (char *) data, todo); If this read is blocking, it means that src_unlock() will never be able to make it unblock if the server stops receiving. We end up turn a problem into another.
Need to add more: read_failed: { GST_OBJECT_UNLOCK(src); ... eos: { GST_OBJECT_UNLOCK(src); ... My observations show that RTMP_Read() does not block thread. Thread blocks RTMP_ConnectStream(). Since this function is not called in the plugin, it is apparently called when RTMP_Read() is first called. I have not yet found a solution, how to interrupt execution RTMP_ConnectStream(). Correct me, please, if I'm not right. P.S. Sorry for my English
despites this bug being totally the wrong place for an announcement, but you should try out https://github.com/heftig/gst-plugins-bad/tree/rtmp2
Well, besides trying to write a completely new RTMP implementation, at Make.TV we've added patches from pexip (https://github.com/pexip/gst-plugins-bad/commits/master/ext/rtmp) to avoid this problem. Unfortunately, they also go with patches to librtmp (https://github.com/pexip/librtmp/commits/master).
(In reply to Andreas Frisch from comment #12) > despites this bug being totally the wrong place for an announcement, but you > should try out https://github.com/heftig/gst-plugins-bad/tree/rtmp2 Thanks, feel free to file a bug if/when you'd like to get some feedback about this implementation. We are looking forward a replacement for RTMP library which isn't the best fit, specially due to threading and error handling. (In reply to Jan Alexander Steffens (heftig) from comment #13) > Well, besides trying to write a completely new RTMP implementation, at > Make.TV we've added patches from pexip > (https://github.com/pexip/gst-plugins-bad/commits/master/ext/rtmp) to avoid > this problem. Unfortunately, they also go with patches to librtmp > (https://github.com/pexip/librtmp/commits/master). Thanks for pointing this out. I think we can learn from this. Notice that it initially ad a socket close(), and then removes it (using only shutdown()) to unblock. Could be a shorter term solution until a replacement is ready and merged. I think that meanwhile Pexip has moved and have their own rtmp library and their own element too. I think creating a bug for Andreas replacement would be a good way to discuss this.
(In reply to Nicolas Dufresne (stormer) from comment #14) > Thanks, feel free to file a bug if/when you'd like to get some feedback about this > implementation. We are looking forward a replacement for RTMP library which isn't the > best fit, specially due to threading and error handling. This is now bug 783354.
Regarding timeout, using patch posted on librtmp list https://lists.mplayerhq.hu/pipermail/rtmpdump/2014-November/002398.html and timeout=10 was working fine. Most probably above patch never accepted upstream.
maybe someone could be find useful this implementation based on ffmpeg https://bugzilla.gnome.org/show_bug.cgi?id=788583
-- 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-plugins-bad/issues/143.