GNOME Bugzilla – Bug 767684
The tracker plugin crashes Rhythmbox when a phone is connected
Last modified: 2016-08-30 20:28:54 UTC
I reproduced this with Rhythmbox 3.3.1, Grilo 0.3.0 and Grilo-Plugins 0.3.1 Rhythmbox crashes if you connect a phone using a USB cable if the "Grilo media browser" plugin is enabled. I did this with an Android phone, as soon as Nautilus shows the phone icon then Rhythmbox crashes. The problem seems to be easier to reproduce if the plugin is already enabled when you launch Rhythmbox. Here's the backtrace, see the value of source->ref_count below. Program received signal SIGSEGV, Segmentation fault. 0x00007fffbb390571 in notify_change (self=0x17ac8a0 [GrlTrackerSourceNotify], id=104136, change_type=GRL_CONTENT_CHANGED) at grl-tracker-source-notif.c:120 120 if (!source || !GRL_IS_TRACKER_SOURCE (source) || (gdb) bt
+ Trace 236354
(gdb) print source $1 = 0x1482df0 (gdb) print *source $2 = {parent = {parent = {g_type_instance = {g_class = 0xffffffff00000000}, ref_count = 4294967295, qdata = 0x1}, priv = 0x0, _grl_reserved = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x17212a0, 0x0, 0x996200, 0x2, 0x0, 0x1482d90}}, priv = 0x1482d80}
*** Bug 770089 has been marked as a duplicate of this bug. ***
OK, After disabling "Grilo media browser" plugin in Rhythmbox I can't trigger this issue anymore
Trying to debug this but kinda tricky why we have the GrlTrackerSource* but GRL_IS_TRACKER_SOURCE fails. Just as a side note, ignoring grl_tracker_source_find ("") avoids the crash here but I'm sure it's not the right awnser. diff --git a/src/tracker/grl-tracker-source-notif.c b/src/tracker/grl-tracker-source-notif.c index 027174e..005d201 100644 --- a/src/tracker/grl-tracker-source-notif.c +++ b/src/tracker/grl-tracker-source-notif.c @@ -111,8 +111,10 @@ notify_change (GrlTrackerSourceNotify *self, if (info == NULL) goto out; +#if 0 if (!grl_tracker_per_device_source) source = grl_tracker_source_find (""); +#endif if (!source && info->datasource) source = grl_tracker_source_find (info->datasource); -- See: https://bugzilla.gnome.org/show_bug.cgi?id=746974#c33
My guess: grl_tracker_source_find ("") returns a pointer to a freed object. Refcounting issue somewhere? Valgrind could give more information.
Created attachment 334133 [details] [review] tracker: fix use after free of GrlTrackerSource Code was calling grl_tracker_add_source() with a new GrlTrackerSource and unref'ing it afterwards; grl_tracker_add_source() calls grl_registry_register_source() which takes ownership of given GrlSource. Let's have grl_tracker_add_source() to take ownership of @source. Backtrace: #0 0x00007fffbb390571 in notify_change (self=0x17ac8a0 [GrlTrackerSourceNotify], id=104136, change_type=GRL_CONTENT_CHANGED) at grl-tracker-source-notif.c:120 #1 0x00007fffbb3907a1 in update_query_done (self=0x17ac8a0 [GrlTrackerSourceNotify]) at grl-tracker-source-notif.c:159 #2 0x00007fffbb39088e in update_cursor_next_cb (source_object=0x7fffc00109a0 [TrackerDBCursor], result=0x7fffcf7e75c0, user_data=0x17ac8a0) at grl-tracker-source-notif.c:183 #3 0x00007ffff47aeb43 in g_task_return_now (task=0x7fffcf7e75c0 [GTask]) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gtask.c:1107 #4 0x00007ffff47aeb79 in complete_in_idle_cb (task=0x7fffcf7e75c0) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gtask.c:1121 #5 0x00007ffff35b905a in g_main_context_dispatch (context=0x623730) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3154 #6 0x00007ffff35b905a in g_main_context_dispatch (context=context@entry=0x623730) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3769 #7 0x00007ffff35b9400 in g_main_context_iterate (context=context@entry=0x623730, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3840 #8 0x00007ffff35b94ac in g_main_context_iteration (context=context@entry=0x623730, may_block=may_block@entry=1) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3901 #9 0x00007ffff47d4cdd in g_application_run (application=0x62c180 [RBApplication], argc=1, argv=0x7fffffffde18) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gapplication.c:2381 #10 0x00007ffff7acf515 in rb_application_run () at /usr/lib/librhythmbox-core.so.9 #11 0x0000000000400eba in main () Reported-by: Alberto Garcia <berto@igalia.com>
Created attachment 334134 [details] [review] tracker: remove unused GHashTable It seems that this grl_tracker_source_sources_modified GHashTable was being used to keep track of removed GrlTrackerSources while they had pending events/notifications. Current code does never insert on it, we can safely removed.
Created attachment 334135 [details] [review] tracker: remove unused notification_ref Likely a leftover after merging parts of the code.
First patch should fix the issue while the other two are cleanups that I've found while reading the code... It would be great to have some people to test this first patch to be sure that it actually fixes the bug. My reproducer was opening Rhythmbox and generating tracker events on my $HOME folder, creating/removing files/directories. It was always crashing in the second event.
grl_tracker_add_source() won't take ownership in the case priv->notification_ref == 0. It's missing a else{g_object_unref(source);} I personally don't like APIs that takes ownership of references, unless their name clearly tells it, like grl_tracker_take_source() for example. I would rather add an explicit g_object_ref() when calling grl_registry_register_source(). But that's just a personal taste, your patch works too.
ohh, btw that priv->notification_ref is useless, it's only ever decremented!
(In reply to Xavier Claessens from comment #10) > ohh, btw that priv->notification_ref is useless, it's only ever decremented! Yes, fixed in a later patch :D (In reply to Xavier Claessens from comment #9) > grl_tracker_add_source() won't take ownership in the case > priv->notification_ref == 0. It's missing a else{g_object_unref(source);} > > I personally don't like APIs that takes ownership of references, unless > their name clearly tells it, like grl_tracker_take_source() for example. I > would rather add an explicit g_object_ref() when calling > grl_registry_register_source(). But that's just a personal taste, your patch > works too. Agreed. I'll change it. Thanks for the review!
Created attachment 334148 [details] [review] tracker: fix use after free of GrlTrackerSource grl_registry_register_source() takes ownership of GrlSource and we were not considering it. Increasing reference for Grilo API. Backtrace: #0 0x00007fffbb390571 in notify_change (self=0x17ac8a0 [GrlTrackerSourceNotify], id=104136, change_type=GRL_CONTENT_CHANGED) at grl-tracker-source-notif.c:120 #1 0x00007fffbb3907a1 in update_query_done (self=0x17ac8a0 [GrlTrackerSourceNotify]) at grl-tracker-source-notif.c:159 #2 0x00007fffbb39088e in update_cursor_next_cb (source_object=0x7fffc00109a0 [TrackerDBCursor], result=0x7fffcf7e75c0, user_data=0x17ac8a0) at grl-tracker-source-notif.c:183 #3 0x00007ffff47aeb43 in g_task_return_now (task=0x7fffcf7e75c0 [GTask]) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gtask.c:1107 #4 0x00007ffff47aeb79 in complete_in_idle_cb (task=0x7fffcf7e75c0) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gtask.c:1121 #5 0x00007ffff35b905a in g_main_context_dispatch (context=0x623730) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3154 #6 0x00007ffff35b905a in g_main_context_dispatch (context=context@entry=0x623730) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3769 #7 0x00007ffff35b9400 in g_main_context_iterate (context=context@entry=0x623730, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3840 #8 0x00007ffff35b94ac in g_main_context_iteration (context=context@entry=0x623730, may_block=may_block@entry=1) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./glib/gmain.c:3901 #9 0x00007ffff47d4cdd in g_application_run (application=0x62c180 [RBApplication], argc=1, argv=0x7fffffffde18) at /build/glib2.0-wnDt2X/glib2.0-2.48.1/./gio/gapplication.c:2381 #10 0x00007ffff7acf515 in rb_application_run () at /usr/lib/librhythmbox-core.so.9 #11 0x0000000000400eba in main () Reported-by: Alberto Garcia <berto@igalia.com>
Created attachment 334149 [details] [review] tracker: remove unused GHashTable It seems that this grl_tracker_source_sources_modified GHashTable was being used to keep track of removed GrlTrackerSources while they had pending events/notifications. Current code does never insert on it, we can safely removed.
Created attachment 334150 [details] [review] tracker: remove unused notification_ref Likely a leftover after merging parts of the code.
*** Bug 769970 has been marked as a duplicate of this bug. ***
(In reply to Victor Toso from comment #8) > First patch should fix the issue while the other two are cleanups that I've > found while reading the code... It would be great to have some people to > test this first patch to be sure that it actually fixes the bug. I am not seeing this issue with the patch. But, I guess that is not good enough. I have enabled the grilo plugin in Rhythmbox. I'll atleast watch for 3-4 days. If there is no crash ( which I will update after 4 days ), then I can vote for a closure. Thanks !
Review of attachment 334148 [details] [review]: Yep.
Review of attachment 334149 [details] [review]: Sure.
Review of attachment 334150 [details] [review]: Yeah.
(In reply to vrishab from comment #16) > (In reply to Victor Toso from comment #8) > > First patch should fix the issue while the other two are cleanups that I've > > found while reading the code... It would be great to have some people to > > test this first patch to be sure that it actually fixes the bug. > > I am not seeing this issue with the patch. But, I guess that is not good > enough. I have enabled the grilo plugin in Rhythmbox. I'll atleast watch for > 3-4 days. If there is no crash ( which I will update after 4 days ), then I > can vote for a closure. Thanks ! Not seeing this issue any more, with the patch.
Attachment 334148 [details] pushed as 7c6d388 - tracker: fix use after free of GrlTrackerSource Attachment 334149 [details] pushed as 36061c5 - tracker: remove unused GHashTable Attachment 334150 [details] pushed as fbb8165 - tracker: remove unused notification_ref
(In reply to Victor Toso from comment #21) > Attachment 334148 [details] pushed as 7c6d388 - tracker: fix use after free > of GrlTrackerSource > Attachment 334149 [details] pushed as 36061c5 - tracker: remove unused > GHashTable > Attachment 334150 [details] pushed as fbb8165 - tracker: remove unused > notification_ref The commit links are not working. The page displays "Bad object id: 7c6d388"
Just replace "grilo" by "grilo-plugins" is the URL. Bugzilla isn't smart enough to know in which repository those commits are.
Thanks !