GNOME Bugzilla – Bug 778096
race in gdbusconnection reported by TSan
Last modified: 2017-02-05 16:17:36 UTC
Created attachment 344806 [details] ThreadSanitizer: data race glib-2-50/gio/gdbusconnection.c:3469 in g_dbus_connection_signal_subscribe This case reported by the ThreadSanitizer suggests an atomic access to the _global_subscriber_id counter.
Created attachment 344812 [details] [review] use atomic functions to increment _global_subscriber_id
Review of attachment 344812 [details] [review]: I would also mark the declaration of _global_subscriber_id as volatile to indicate it should be accessed atomically; or add a comment there or something. The same change needs to be made to _global_registration_id and _global_subtree_registration_id, as they’re also both accessed by functions which are only locked under a single GDBusConnection’s lock. ::: gio/gdbusconnection.c @@ +3467,3 @@ subscriber.user_data = user_data; subscriber.user_data_free_func = user_data_free_func; + subscriber.id = g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */ Could use g_atomic_int_inc() instead to make things a little simpler. It would be good to fix the overflow handling in a separate patch.
(In reply to Philip Withnall from comment #2) > Review of attachment 344812 [details] [review] [review]: > > I would also mark the declaration of _global_subscriber_id as volatile to > indicate it should be accessed atomically; or add a comment there or > something. > > The same change needs to be made to _global_registration_id and > _global_subtree_registration_id, as they’re also both accessed by functions > which are only locked under a single GDBusConnection’s lock. And _global_filter_id.
(In reply to Philip Withnall from comment #2) > ::: gio/gdbusconnection.c > @@ +3467,3 @@ > subscriber.user_data = user_data; > subscriber.user_data_free_func = user_data_free_func; > + subscriber.id = g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: > overflow etc. */ > > Could use g_atomic_int_inc() instead to make things a little simpler. Whoops, no, that doesn’t return a value.
Created attachment 344874 [details] [review] gdbus: make gdbusconnection ids thread-safe
Review of attachment 344874 [details] [review]: Looks reasonable to me (though note I don’t have the authority to ack GLib patches to be pushed). Are you going to do a patch to sort out the overflow handling as well?
Allison says this is good. I’ll push it shortly.
Pushed, thanks. Attachment 344874 [details] pushed as b1f1414 - gdbus: make gdbusconnection ids thread-safe
(In reply to Philip Withnall from comment #6) > Are you going to do a patch to sort out the overflow > handling as well? I think I am not. guint are 4 bytes on current architectures, so the risk of an overflow seems very low to me. A program incrementing the counter 10 times per seconds, should run for more than 13 years before reaching to max uint value.