aio: Fix use-after-free in cancellation path
authorFam Zheng <famz@redhat.com>
Wed, 21 May 2014 02:42:13 +0000 (10:42 +0800)
committerStefan Hajnoczi <stefanha@redhat.com>
Wed, 28 May 2014 12:28:46 +0000 (14:28 +0200)
commit271c0f68b4eae72691721243a1c37f46a3232d61
tree70ea4e56b2f6705022b0c5979bb1564829d9dda6
parentbd60436936ecebf2c683eef9ea561f2b949700f3
aio: Fix use-after-free in cancellation path

The current flow of canceling a thread from THREAD_ACTIVE state is:

  1) Caller wants to cancel a request, so it calls thread_pool_cancel.

  2) thread_pool_cancel waits on the conditional variable
     elem->check_cancel.

  3) The worker thread changes state to THREAD_DONE once the task is
     done, and notifies elem->check_cancel to allow thread_pool_cancel
     to continue execution, and signals the notifier (pool->notifier) to
     allow callback function to be called later. But because of the
     global mutex, the notifier won't get processed until step 4) and 5)
     are done.

  4) thread_pool_cancel continues, leaving the notifier signaled, it
     just returns to caller.

  5) Caller thinks the request is already canceled successfully, so it
     releases any related data, such as freeing elem->common.opaque.

  6) In the next main loop iteration, the notifier handler,
     event_notifier_ready, is called. It finds the canceled thread in
     THREAD_DONE state, so calls elem->common.cb, with an (likely)
     dangling opaque pointer. This is a use-after-free.

Fix it by calling event_notifier_ready before leaving
thread_pool_cancel.

Test case update: This change will let cancel complete earlier than
test-thread-pool.c expects, so update the code to check this case: if
it's already done, done_cb sets .aiocb to NULL, skip calling
bdrv_aio_cancel on them.

Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
tests/test-thread-pool.c
thread-pool.c