GNOME Bugzilla – Bug 680111
GIOScheduler assumes GCancellable "cancelled" is emitted only once
Last modified: 2012-07-17 17:46:07 UTC
Unfortunately (I think this is a misfeature) GCancellable can be "cancelled" more than once. If it is g_cancellable_reset() in between g_cancellable_cancel() calls. GIOSchedulerJob thinks it can only be cancelled once in on_job_canceled() and clears the cancellable_id when cancelled, which it would otherwise later use to disconnect from the GCancellable signal later during freeing the job.
You can see this in the libgdata tests: G_SLICE=always-malloc libtool --mode=execute valgrind --log-file=$HOME/Desktop/gioscheduler-access-after-free.log gdata/tests/youtube Produces: ==4371== Memcheck, a memory error detector ==4371== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al. ==4371== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info ==4371== Command: /data/projects/libgdata/gdata/tests/.libs/lt-youtube ==4371== Parent PID: 32143 ==4371== ==4371== Invalid write of size 4 ==4371== at 0x5D35A8A: on_job_canceled (gioscheduler.c:133) ==4371== by 0x627B333: g_cclosure_marshal_VOID__VOID (gmarshal.c:85) ==4371== by 0x62782AF: g_closure_invoke (gclosure.c:777) ==4371== by 0x6295A3E: signal_emit_unlocked_R (gsignal.c:3551) ==4371== by 0x6294CA7: g_signal_emit_valist (gsignal.c:3300) ==4371== by 0x62951EF: g_signal_emit (gsignal.c:3356) ==4371== by 0x5D030E0: g_cancellable_cancel (gcancellable.c:507) ==4371== by 0x412CC8: gdata_async_test_cancellation_cb (common.c:628) ==4371== by 0x6711177: g_timeout_dispatch (gmain.c:4002) ==4371== by 0x670F4DB: g_main_dispatch (gmain.c:2691) ==4371== by 0x671008B: g_main_context_dispatch (gmain.c:3195) ==4371== by 0x671026E: g_main_context_iterate (gmain.c:3266) ==4371== Address 0x7884e68 is 40 bytes inside a block of size 72 free'd ==4371== at 0x4A079AE: free (vg_replace_malloc.c:427) ==4371== by 0x6717582: standard_free (gmem.c:98) ==4371== by 0x6717745: g_free (gmem.c:252) ==4371== by 0x5D35990: g_io_job_free (gioscheduler.c:78) ==4371== by 0x5D35B3F: job_destroy (gioscheduler.c:153) ==4371== by 0x5D35BC9: io_job_thread (gioscheduler.c:175) ==4371== by 0x673BBCD: g_thread_pool_thread_proxy (gthreadpool.c:309) ==4371== by 0x673B607: g_thread_proxy (gthread.c:801) ==4371== by 0x3C85207EF4: start_thread (in /usr/lib64/libpthread-2.15.90.so) ==4371== by 0x3C84EF4EAC: clone (in /usr/lib64/libc-2.15.90.so) ==4371== ==4371== Invalid write of size 8 ==4371== at 0x5D35A95: on_job_canceled (gioscheduler.c:134) ==4371== by 0x627B333: g_cclosure_marshal_VOID__VOID (gmarshal.c:85) ==4371== by 0x62782AF: g_closure_invoke (gclosure.c:777) ==4371== by 0x6295A3E: signal_emit_unlocked_R (gsignal.c:3551) ==4371== by 0x6294CA7: g_signal_emit_valist (gsignal.c:3300) ==4371== by 0x62951EF: g_signal_emit (gsignal.c:3356) ==4371== by 0x5D030E0: g_cancellable_cancel (gcancellable.c:507) ==4371== by 0x412CC8: gdata_async_test_cancellation_cb (common.c:628) ==4371== by 0x6711177: g_timeout_dispatch (gmain.c:4002) ==4371== by 0x670F4DB: g_main_dispatch (gmain.c:2691) ==4371== by 0x671008B: g_main_context_dispatch (gmain.c:3195) ==4371== by 0x671026E: g_main_context_iterate (gmain.c:3266) ==4371== Address 0x7884e78 is 56 bytes inside a block of size 72 free'd ==4371== at 0x4A079AE: free (vg_replace_malloc.c:427) ==4371== by 0x6717582: standard_free (gmem.c:98) ==4371== by 0x6717745: g_free (gmem.c:252) ==4371== by 0x5D35990: g_io_job_free (gioscheduler.c:78) ==4371== by 0x5D35B3F: job_destroy (gioscheduler.c:153) ==4371== by 0x5D35BC9: io_job_thread (gioscheduler.c:175) ==4371== by 0x673BBCD: g_thread_pool_thread_proxy (gthreadpool.c:309) ==4371== by 0x673B607: g_thread_proxy (gthread.c:801) ==4371== by 0x3C85207EF4: start_thread (in /usr/lib64/libpthread-2.15.90.so) ==4371== by 0x3C84EF4EAC: clone (in /usr/lib64/libc-2.15.90.so) ==4371== ...
Created attachment 219036 [details] [review] GIOScheduler: Fix access after free in "cancelled" handler * GCancellable can be "cancelled" more than once if g_cancellable_reset() is called. * Don't assume that because the "cancelled" signal fired it won't fire again.
Review of attachment 219036 [details] [review]: This is a regression from 2839297686a9305b4fa909b93c337ef1d1a5e94b (my commit). Hmm...you have two asterisks in your commit message, but you're only changing one line really. Seems like the commit message should just be one paragraph? Anyways, looks OK to me.
The following fixes have been pushed: a0b7183 GIOScheduler: Fix access after free in "cancelled" handler e97a4c7 2.33.6
Created attachment 219043 [details] [review] GIOScheduler: Fix access after free in "cancelled" handler * GCancellable can be "cancelled" more than once if g_cancellable_reset() is called. * Don't assume that because the "cancelled" signal fired it won't fire again.
Created attachment 219044 [details] [review] 2.33.6