GNOME Bugzilla – Bug 554887
boxed type registration is not thread safe
Last modified: 2010-01-25 14:34:45 UTC
this report has been filed here: https://bugs.edge.launchpad.net/ubuntu/+source/rhythmbox/+bug/277299 "After starting Rhythmbox I tried to start playing an MP3-stream (internet radio). Rhythmbox froze for a few seconds before closing and apport reported an error." rhythmbox version is: 0.11.6svn20080916 ".
+ Trace 207716
Thread 3 (process 9832)
Thread 1 (process 9818)
Thanks!,
*** Bug 561980 has been marked as a duplicate of this bug. ***
What's happening here is that a new boxed type is being registered (in totem_pl_parser_class_init) while a boxed value is being used elsewhere (the g_object_get in check_entry_type is retrieving a boxed value). Registered boxed types are stored in a binary search array. With nothing protecting against concurrent reads and writes to the array, it's going to crash sometimes.
(In reply to comment #2) > What's happening here is that a new boxed type is being registered (in > totem_pl_parser_class_init) while a boxed value is being used elsewhere (the > g_object_get in check_entry_type is retrieving a boxed value). Right, thanks for figuring. What's needed is locking a mutex around accesses to gboxed.c:boxed_bsa.
*** Bug 576857 has been marked as a duplicate of this bug. ***
*** Bug 579035 has been marked as a duplicate of this bug. ***
*** Bug 583962 has been marked as a duplicate of this bug. ***
*** Bug 593398 has been marked as a duplicate of this bug. ***
*** Bug 594847 has been marked as a duplicate of this bug. ***
Created attachment 143778 [details] Test case Here's a test case. Compile and run with $ while true; do ./test ; done and you'll see core dumps galore.
Created attachment 143779 [details] [review] Patch And here's a patch to fix the problem
Created attachment 143800 [details] [review] Updated patch There's a problem with the previous patch, and it's that if node->copy() or node->free() make calls to GBoxed functions it'll block the program since the mutex would be locked. This patch fixes that problem.
can please someone review this patch and commit? thanks !
ping
*** Bug 555103 has been marked as a duplicate of this bug. ***
I think this might be a great case for GAtomicArray.
Created attachment 151870 [details] [review] Put calls to registered copy/free functions into separate functions This eases cleaning up these functions. One optimization in value_set_internal() was lost in the process. It shouldn't cause too many issues when all is said and done.
Created attachment 151871 [details] [review] Move the boxed private type data to TypeNode This way we don't need to keep a custom array that we bsearch on (and that isn't threadsafe) but can use the gtype.c machinery that is threadsafe. And fast, too!
Here's an implementation that uses the TypeNode data available in gtype.c and therefor does not require its own lock for every copy/free. It passes the attached test (it's running in the background as I type this and hasn't complained yet). It should be faster, too because it avoids the bsearch. The only thing I don't like is the interaction with gtype.c and the necessity to introduce gtype-private.h. But I think it's the best solution. Comments?
Looks reasonable to me.
looks ok to me too