GNOME Bugzilla – Bug 748263
Use-after-free in g_dbus_connection_call_internal()
Last modified: 2017-06-23 12:13:08 UTC
Created attachment 302096 [details] [review] proposed glib patch When running evolution-calendar-factory under valgrind I spotted this in the log: Thread 5 pool: Invalid read of size 4 at 0x650DBAA: g_dbus_connection_call_internal (gdbusconnection.c:5482) by 0x650E453: g_dbus_connection_call_with_unix_fd_list (gdbusconnection.c:5903) by 0x651EAEB: g_dbus_proxy_call_internal (gdbusproxy.c:2719) by 0x651F14D: g_dbus_proxy_call (gdbusproxy.c:2959) by 0x5102942: e_dbus_source_proxy_set_property (e-dbus-source.c:1473) by 0x67DF049: object_set_property (gobject.c:1415) by 0x67E0EC7: g_object_set_valist (gobject.c:2158) by 0x67E15CB: g_object_set (gobject.c:2268) by 0x5101DE1: e_dbus_source_set_connection_status (e-dbus-source.c:897) by 0x4E8DB58: e_source_set_connection_status (e-source.c:3716) by 0x1C67FCD0: e_cal_backend_file_open (e-cal-backend-file.c:1390) by 0x5366BE8: e_cal_backend_sync_open (e-cal-backend-sync.c:69) Address 0x1c92f3e0 is 16 bytes inside a block of size 32 free'd at 0x4A07CE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x6A77D6F: g_free (gmem.c:192) by 0x6A90C62: g_slice_free1 (gslice.c:1112) by 0x650D629: call_state_free (gdbusconnection.c:5354) by 0x64A2F8E: g_task_finalize (gtask.c:635) by 0x67E3415: g_object_unref (gobject.c:3174) by 0x650D7C3: g_dbus_connection_call_done (gdbusconnection.c:5404) by 0x64A3560: g_task_return_now (gtask.c:1106) by 0x64A35BC: complete_in_idle_cb (gtask.c:1120) by 0x6A71FF8: g_idle_dispatch (gmain.c:5397) by 0x6A6F655: g_main_dispatch (gmain.c:3122) by 0x6A70490: g_main_context_dispatch (gmain.c:3737) The gdbusconnection.c:5482 looks like: 5479 cancellable, 5480 g_dbus_connection_call_done, 5481 task); 5482 serial = state->serial; 5483 } 5484 else That means that the 'state' was freed before the code got to this point. A simple solution for git master is attached. A similar approach might be used for the 2.44 branch as well.
It would be nice to get a review on this and finally have it fixed in the code.
Indeed, g_dbus_connection_call_internal() can return immediately when the cancellable has already been cancelled. That leads to the callback being called immediately, which frees the task. The patch feels like a workaround to me, though. I think we could instead remove the 'serial' field from CallState, use the 'serial' local variable as a return location directly, and use g_dbus_message_get_reply_serial() in the callback. Thanks for reporting this!
(In reply to Lars Uebernickel from comment #2) > The patch feels like a workaround to me, though. I think we could instead > remove the 'serial' field from CallState, use the 'serial' local variable as > a return location directly, and use g_dbus_message_get_reply_serial() in the > callback. Maybe. I didn't want to change the internals too much, I do not know all the consequences, neither the gio code, thus the proposed patch seemed like a possible solution to me.
Lars, will you push a fix along those lines?
Created attachment 319943 [details] [review] gdbus: fix use-after-free g_dbus_connection_call_internal() accesses the user data it passes to g_dbus_connection_send_message_with_reply() after the call. That data might be freed already in the case that the callback is called immediately. Fix this by removing the 'serial' field from the user data altogether and fetch the serial from the message in the callback.
I am not completely satisfied by this patch: something doesn't add up here. The GTask should be freed only from g_dbus_connection_call_done, which should _always_ run from a new mainloop invocation (regardless of cancelled, or anything else). That means that the task should survive until after the function returns. We should figure out why this isn't the case before we commit this fix.
fwiw, I agree that this patch is correct... and probably even a nice simplification... but if it fixes anything then, well, I think something else is wrong somewhere else.
Thread interleaving might be related here. The valgrind log from comment #0 shows the g_dbus_connection_call_internal() being called in a dedicated thread, not in the main thread (or the thread where the thread default main loop is running). Thus in some cases you can call the g_dbus_connection_call_done() in the "main" thread while the dedicated thread is still waiting for processing by the CPU.
(In reply to Milan Crha from comment #8) > Thread interleaving might be related here. The valgrind log from comment #0 > shows the g_dbus_connection_call_internal() being called in a dedicated > thread, not in the main thread (or the thread where the thread default main > loop is running). Thus in some cases you can call the > g_dbus_connection_call_done() in the "main" thread while the dedicated > thread is still waiting for processing by the CPU. This doesn't make sense to me -- g_dbus_connection_call_done() is called when the call is done... the function in the dedicated thread surely must have finished running by that point.
(In reply to Allison Ryan Lortie (desrt) from comment #9) > This doesn't make sense to me -- g_dbus_connection_call_done() is called > when the call is done... the function in the dedicated thread surely must > have finished running by that point. I see something else in the valgrind log. But I can be wrong too, no doubt.
A report from an address sanitizer: ==31480==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000407360 at pc 0x7f515ca3fcc4 bp 0x7f5142786dd0 sp 0x7f5142786dc0 READ of size 4 at 0x603000407360 thread T7 (pool) #0 0x7f515ca3fcc3 in g_dbus_connection_call_internal ..../glib-2.50.2/gio/gdbusconnection.c:5790 #1 0x7f515ca40b31 in g_dbus_connection_call_with_unix_fd_list ..../glib-2.50.2/gio/gdbusconnection.c:6211 #2 0x7f515ca6a084 in g_dbus_proxy_call_internal ..../glib-2.50.2/gio/gdbusproxy.c:2724 #3 0x7f515ca6af2c in g_dbus_proxy_call ..../glib-2.50.2/gio/gdbusproxy.c:2964 #4 0x7f515abe6ce1 in e_dbus_source_proxy_set_property ..../evolution-data-server/_build/src/private/e-dbus-source.c:1630 #5 0x7f515c1409cd in object_set_property ..../glib-2.50.2/gobject/gobject.c:1423 #6 0x7f515c145f2c in g_object_set_valist ..../glib-2.50.2/gobject/gobject.c:2167 #7 0x7f515c147415 in g_object_set ..../glib-2.50.2/gobject/gobject.c:2277 #8 0x7f515abe5988 in e_dbus_source_set_connection_status ..../evolution-data-server/_build/src/private/e-dbus-source.c:936 #9 0x7f515b07b7cc in e_source_set_connection_status ..../evolution-data-server/src/libedataserver/e-source.c:3520 #10 0x7f5144273598 in caldav_server_open_calendar ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:1352 #11 0x7f514427ec55 in open_calendar_wrapper ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:3012 #12 0x7f514427f7a8 in caldav_do_open ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:3102 #13 0x7f515d38d1a9 in e_cal_backend_sync_open ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend-sync.c:67 #14 0x7f515d38f2fb in cal_backend_open ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend-sync.c:618 #15 0x7f515d36f829 in cal_backend_open_thread ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:1560 #16 0x7f515d368bef in cal_backend_dispatch_thread ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:241 #17 0x7f515bb0211d in g_thread_pool_thread_proxy ..../glib-2.50.2/glib/gthreadpool.c:307 #18 0x7f515bb01049 in g_thread_proxy ..../glib-2.50.2/glib/gthread.c:784 #19 0x7f515d8566c9 in start_thread (/lib64/libpthread.so.0+0x76c9) #20 0x7f515a8a7f6e in clone (/lib64/libc.so.6+0x107f6e) 0x603000407360 is located 16 bytes inside of 32-byte region [0x603000407350,0x603000407370) freed by thread T0 here: #0 0x7f515e84baf0 in free (/usr/lib64/libasan.so.3+0xc6af0) #1 0x7f515ba9647c in g_free ..../glib-2.50.2/glib/gmem.c:189 #2 0x7f515bae1eff in g_slice_free1 ..../glib-2.50.2/glib/gslice.c:1136 #3 0x7f515ca3f079 in call_state_free ..../glib-2.50.2/gio/gdbusconnection.c:5661 #4 0x7f515c993659 in g_task_finalize ..../glib-2.50.2/gio/gtask.c:638 #5 0x7f515c14b4ee in g_object_unref ..../glib-2.50.2/gobject/gobject.c:3185 #6 0x7f515ca3f4e3 in g_dbus_connection_call_done ..../glib-2.50.2/gio/gdbusconnection.c:5711 #7 0x7f515c99493b in g_task_return_now ..../glib-2.50.2/gio/gtask.c:1121 #8 0x7f515c994a1f in complete_in_idle_cb ..../glib-2.50.2/gio/gtask.c:1135 #9 0x7f515ba85048 in g_idle_dispatch ..../glib-2.50.2/glib/gmain.c:5545 #10 0x7f515ba7babc in g_main_dispatch ..../glib-2.50.2/glib/gmain.c:3203 #11 0x7f515ba7ff4c in g_main_context_dispatch ..../glib-2.50.2/glib/gmain.c:3856 #12 0x7f515ba80522 in g_main_context_iterate ..../glib-2.50.2/glib/gmain.c:3929 #13 0x7f515ba81074 in g_main_loop_run ..../glib-2.50.2/glib/gmain.c:4125 #14 0x4042f6 in main ..../evolution-data-server/src/calendar/libedata-cal/evolution-calendar-factory-subprocess.c:217 #15 0x7f515a7c0400 in __libc_start_main (/lib64/libc.so.6+0x20400) previously allocated by thread T7 (pool) here: #0 0x7f515e84be50 in malloc (/usr/lib64/libasan.so.3+0xc6e50) #1 0x7f515ba96313 in g_malloc ..../glib-2.50.2/glib/gmem.c:94 #2 0x7f515bae1c0c in g_slice_alloc ..../glib-2.50.2/glib/gslice.c:1025 #3 0x7f515bae1c4c in g_slice_alloc0 ..../glib-2.50.2/glib/gslice.c:1051 #4 0x7f515ca3fade in g_dbus_connection_call_internal ..../glib-2.50.2/gio/gdbusconnection.c:5770 #5 0x7f515ca40b31 in g_dbus_connection_call_with_unix_fd_list ..../glib-2.50.2/gio/gdbusconnection.c:6211 #6 0x7f515ca6a084 in g_dbus_proxy_call_internal ..../glib-2.50.2/gio/gdbusproxy.c:2724 #7 0x7f515ca6af2c in g_dbus_proxy_call ..../glib-2.50.2/gio/gdbusproxy.c:2964 #8 0x7f515abe6ce1 in e_dbus_source_proxy_set_property ..../evolution-data-server/_build/src/private/e-dbus-source.c:1630 #9 0x7f515c1409cd in object_set_property ..../glib-2.50.2/gobject/gobject.c:1423 #10 0x7f515c145f2c in g_object_set_valist ..../glib-2.50.2/gobject/gobject.c:2167 #11 0x7f515c147415 in g_object_set ..../glib-2.50.2/gobject/gobject.c:2277 #12 0x7f515abe5988 in e_dbus_source_set_connection_status ..../evolution-data-server/_build/src/private/e-dbus-source.c:936 #13 0x7f515b07b7cc in e_source_set_connection_status ..../evolution-data-server/src/libedataserver/e-source.c:3520 #14 0x7f5144273598 in caldav_server_open_calendar ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:1352 #15 0x7f514427ec55 in open_calendar_wrapper ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:3012 #16 0x7f514427f7a8 in caldav_do_open ..../evolution-data-server/src/calendar/backends/caldav/e-cal-backend-caldav.c:3102 #17 0x7f515d38d1a9 in e_cal_backend_sync_open ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend-sync.c:67 #18 0x7f515d38f2fb in cal_backend_open ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend-sync.c:618 #19 0x7f515d36f829 in cal_backend_open_thread ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:1560 #20 0x7f515d368bef in cal_backend_dispatch_thread ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:241 #21 0x7f515bb0211d in g_thread_pool_thread_proxy ..../glib-2.50.2/glib/gthreadpool.c:307 #22 0x7f515bb01049 in g_thread_proxy ..../glib-2.50.2/glib/gthread.c:784 #23 0x7f515d8566c9 in start_thread (/lib64/libpthread.so.0+0x76c9) Thread T7 (pool) created by T0 here: #0 0x7f515e7b6488 in pthread_create (/usr/lib64/libasan.so.3+0x31488) #1 0x7f515bb6e62b in g_system_thread_new ..../glib-2.50.2/glib/gthread-posix.c:1170 #2 0x7f515bb0131f in g_thread_new_internal ..../glib-2.50.2/glib/gthread.c:874 #3 0x7f515bb012ae in g_thread_try_new ..../glib-2.50.2/glib/gthread.c:858 #4 0x7f515bb026eb in g_thread_pool_start_thread ..../glib-2.50.2/glib/gthreadpool.c:407 #5 0x7f515bb0323a in g_thread_pool_push ..../glib-2.50.2/glib/gthreadpool.c:563 #6 0x7f515d3690a0 in cal_backend_dispatch_next_operation ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:278 #7 0x7f515d36f9da in e_cal_backend_open ..../evolution-data-server/src/calendar/libedata-cal/e-cal-backend.c:1603 #8 0x7f515d3a03b9 in data_cal_handle_open_cb ..../evolution-data-server/src/calendar/libedata-cal/e-data-cal.c:641 #9 0x7f5159467c57 in ffi_call_unix64 (/lib64/libffi.so.6+0x5c57) #10 0x7fff73970e7f (<unknown module>)
I just noticed that this might be a duplicate of bug #703045.
*** Bug 703045 has been marked as a duplicate of this bug. ***
Review of attachment 319943 [details] [review]: This version seems nice and clean to me!
After reading all 3 proposed patches (including mine before I read this bug), I vote for Lars' patch.
Attachment 319943 [details] pushed as 0751ccd - gdbus: fix use-after-free