Bug 735569 – rtspconnection: Crash due to no protection of watchs readsrc

After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.

Bug 735569 - rtspconnection: Crash due to no protection of watchs readsrc


Summary:	rtspconnection: Crash due to no protection of watchs readsrc


Status:	RESOLVED FIXED

Product:	GStreamer
Classification:	Platform
Component:	gst-plugins-base
Version:	unspecified
Hardware:	Other Linux

Importance:	Normal normal
Target Milestone:	1.4.2
Assigned To:	GStreamer Maintainers
QA Contact:	GStreamer Maintainers

URL:
Whiteboard:

Depends on:
Blocks:

Reported:	2014-08-28 07:20 UTC by Göran Jönsson
Modified:	2014-09-04 09:57 UTC

See Also:
GNOME target:	---
GNOME version:	---

Attachments
patch for gst-plugins-base (3.08 KB, patch) 2014-08-28 07:20 UTC, Göran Jönsson	needs-work	Details \| Review
new gst-plugins-base patch (2.61 KB, patch) 2014-08-29 04:52 UTC, Göran Jönsson	committed	Details \| Review

Description Göran Jönsson 2014-08-28 07:20:41 UTC

Created attachment 284662 [details] [review]
patch for gst-plugins-base

This happends due to it is possible to manipulate watchs readsrc from different threads.

This is happening when running with multiple client threads in rtsp server.
More specific from handle_tunnel when calling 
gst_rtsp_watch_reset (opriv->watch);
( Observe that opriv is from another client saved in hash_table )
( Observe same watch Thread4 #4 watch=0x58ced8  Thread12 #11 watch=0x58ced8)

+ Trace 234014

Thread 12 (Thread 21856)

#0 g_mutex_get_impl
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gthread-posix.c line 123
#1 g_mutex_lock
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gthread-posix.c line 210
#2 g_source_destroy_internal
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1185
#3 closure_invoke_notifiers
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/gobject/gclosure.c line 264
#4 g_closure_invalidate
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/gobject/gclosure.c line 562
#5 g_closure_unref
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/gobject/gclosure.c line 593
#6 g_source_destroy_internal
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1204
#7 g_child_source_remove_internal
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1446
#8 g_source_destroy_internal
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1222
#9 g_child_source_remove_internal
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1446
#10 g_source_remove_child_source
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 1479
#11 gst_rtsp_source_dispatch_read
at /home/goranjn/FLASHDIR/560aftersemester/libs/gst-plugins-base/gst-plugins-base/gst-libs/gst/rtsp/gstrtspconnection.c line 3056
#12 g_main_dispatch
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 3063
#13 g_main_context_dispatch
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 3639
#14 g_main_context_iterate
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 3710
#15 g_main_loop_run
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gmain.c line 3904
#16 do_loop
at /home/goranjn/FLASHDIR/560aftersemester/libs/gst-rtsp-server/gst-rtsp-server/gst/rtsp-server/rtsp-thread-pool.c line 329
#17 g_thread_pool_thread_proxy
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gthreadpool.c line 309
#18 g_thread_proxy
at /home/goranjn/FLASHDIR/560aftersemester/libs/glib/glib/glib/gthread.c line 798
#19 start_thread
at pthread_create.c line 310
#20 __thread_start
from target/mipsisa32r2el-axis-linux-gnu/lib/libc.so.6

Comment 1 Göran Jönsson 2014-08-28 07:25:31 UTC

The trace contain two threads. If I just check the "+" sign i only see one.
But if I choose to open link in new tab i can see both.

Comment 2 Sebastian Dröge (slomo) 2014-08-28 07:54:32 UTC

Review of attachment 284662 [details] [review]:

The same seems to be true for the writesrc.

::: gst-libs/gst/rtsp/gstrtspconnection.c
@@ +2935,3 @@
   guint id;
   GMutex mutex;
+  GMutex readsrc_mutex;

Why not just reuse the existing mutex?

@@ +3413,2 @@
   if (watch->conn->input_stream) {
+    g_mutex_lock (&watch->readsrc_mutex);

Just put the lock/unlock outside the if/else block

Comment 3 Göran Jönsson 2014-08-29 04:52:25 UTC

Created attachment 284788 [details] [review]
new gst-plugins-base patch

New patch with corrections.

Comment 4 Sebastian Dröge (slomo) 2014-08-29 08:29:41 UTC

commit acdb7feacf5253b661b0c0e5ce5283cb85483849
Author: Göran Jönsson <goranjn@axis.com>
Date:   Wed Aug 27 13:45:57 2014 +0200

    rtspconnection: Protect readsrc, writesrc and controllsrc with a mutex
    
    Fixes a crash when controlsrc, readsrc or writesrc are modified from
    gst_rtsp_source_dispatch_read/write and gst_rtsp_watch_reset at the
    same time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=735569