GNOME Bugzilla – Bug 765203
Racy driver registration code
Last modified: 2016-04-19 08:58:36 UTC
On startup without a smartcard reader of any kind: ==18858== Invalid read of size 8 ==18858== at 0x77E8D05: g_type_check_instance_is_fundamentally_a (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x77C8F24: g_object_unref (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x21B3B16B: UnknownInlinedFun (gsd-smartcard-manager.c:508) ==18858== by 0x21B3B16B: on_driver_activated (gsd-smartcard-manager.c:529) ==18858== by 0x74B6342: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x74B69E5: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x21B3B09C: on_driver_registered (gsd-smartcard-manager.c:371) ==18858== by 0x74B6342: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x74B69E5: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x21B3B1BC: on_task_thread_to_complete_driver_registration (gsd-smartcard-manager.c:404) ==18858== by 0x7A50802: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A50BAF: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A50ED1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x21B3B8C8: watch_smartcards (gsd-smartcard-manager.c:630) ==18858== by 0x74B6B0F: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x7A778A4: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A76EA7: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7D1D589: start_thread (in /usr/lib64/libpthread-2.23.so) ==18858== by 0x803D5CC: clone (in /usr/lib64/libc-2.23.so) ==18858== Address 0x306075f0 is 0 bytes inside a block of size 208 free'd ==18858== at 0x4C2CD5A: free (vg_replace_malloc.c:530) ==18858== by 0x7A5603D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A6D5AF: g_slice_free1 (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x77E7AC9: g_type_free_instance (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x21B3B163: UnknownInlinedFun (gsd-smartcard-manager.c:504) ==18858== by 0x21B3B163: on_driver_activated (gsd-smartcard-manager.c:529) ==18858== by 0x74B6342: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x74B69E5: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x21B3B09C: on_driver_registered (gsd-smartcard-manager.c:371) ==18858== by 0x74B6342: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x74B69E5: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x21B3B1BC: on_task_thread_to_complete_driver_registration (gsd-smartcard-manager.c:404) ==18858== by 0x7A50802: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A50BAF: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A50ED1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x21B3B8C8: watch_smartcards (gsd-smartcard-manager.c:630) ==18858== by 0x74B6B0F: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x7A778A4: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A76EA7: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7D1D589: start_thread (in /usr/lib64/libpthread-2.23.so) ==18858== by 0x803D5CC: clone (in /usr/lib64/libc-2.23.so) ==18858== Block was alloc'd at ==18858== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==18858== by 0x7A55F28: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A6CEA2: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A6D4CD: g_slice_alloc0 (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x77E7771: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x77C964A: ??? (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x77CB06C: g_object_newv (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x77CB853: g_object_new (in /usr/lib64/libgobject-2.0.so.0.4800.0) ==18858== by 0x74B66A4: g_task_new (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x21B3B695: activate_all_drivers_async (gsd-smartcard-manager.c:543) ==18858== by 0x21B3B695: watch_smartcards (gsd-smartcard-manager.c:624) ==18858== by 0x74B6B0F: ??? (in /usr/lib64/libgio-2.0.so.0.4800.0) ==18858== by 0x7A778A4: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7A76EA7: ??? (in /usr/lib64/libglib-2.0.so.0.4800.0) ==18858== by 0x7D1D589: start_thread (in /usr/lib64/libpthread-2.23.so) ==18858== by 0x803D5CC: clone (in /usr/lib64/libc-2.23.so) I failed to see how the code in activate_all_drivers_async() and on_driver_activated() could not be racy. If activate_driver() returns quickly enough, then the pending operations counter could be 0, when activate_all_drivers_async() is still looping over the drivers to activate. This could result in the task being disposed of when we're still using it.
So what's going on here is we're unrefing the task at activate_all_drivers_async_finish time with the gsd_smartcard_utils_finish_boolean_task() helper. this isn't very idiomatic, so i think we should change it to follow more common GTask patterns. Before I look into that (which is a little bit bigger of a job than I have stomach for on a monday morning), I'll attach a simpler patch that also fixes the problem in a more true-to-the-current-way-it-does-it way. I'll also attach a patch that fixes a problem with the task not completing at all if there are no drivers.
Created attachment 326268 [details] [review] smartcard-manager: complete activation task right away if no drivers Right now if we don't have any smartcard drivers, then the activation task never completes. This commit makes sure we mop up that case up front.
Created attachment 326269 [details] [review] smartcard-manager: don't double-unref task Right now we unref the task associated with activate_all_drivers_async, twice: once when activate_all_drivers_async_finish is called, and once after we call g_task_return . This commit gets rid of the unref after g_task_return. It's not idiomatic to do it this way, but this is the "easy" fix.
Created attachment 326272 [details] [review] smartcard: Fix crash on startup A number of async helpers were using the gsd_smartcard_utils_finish_boolean_task() function, that would unref the GTask when the _finish() function was called. The idiomatic way (and the way the GTask examples show) to unref the GTask is after calling g_task_return_*(). The g_task_return_*() call will take care of holding a reference to the GTask until the _finish() call is made and done.
That last patch would replace the one in comment 3. Would be best if it saw some testing though.
Review of attachment 326272 [details] [review]: this doesn't look wrong to me, and it does get rid of the weird utility function, but there are still a number of cases in the code where we don't g_object_unref after g_task_return. Presumably leaks, but needs investigation.
I'll commit what we have now, to fix the crashes at least.
Attachment 326268 [details] pushed as 4f0c24a - smartcard-manager: complete activation task right away if no drivers Attachment 326272 [details] pushed as 276339c - smartcard: Fix crash on startup