[Mesa-dev] [PATCH] util/queue: fix a race condition in the fence code

[Eric Engestrom]

On Thursday, 2017-09-28 16:10:51 +0000, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> A tempting alternative fix would be adding a lock/unlock pair in
> util_queue_fence_is_signalled. However, that wouldn't actually
> improve anything in the semantics of util_queue_fence_is_signalled,
> while making that test much more heavy-weight. So this lock/unlock
> pair in util_queue_fence_destroy for "flushing out" other threads
> that may still be in util_queue_fence_signal looks like the better
> fix.
> 
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/util/u_queue.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/util/u_queue.c b/src/util/u_queue.c
> index 449da7dc9ab..82a761e8420 100644
> --- a/src/util/u_queue.c
> +++ b/src/util/u_queue.c
> @@ -113,20 +113,33 @@ util_queue_fence_init(struct util_queue_fence *fence)
>     memset(fence, 0, sizeof(*fence));
>     (void) mtx_init(&fence->mutex, mtx_plain);
>     cnd_init(&fence->cond);
>     fence->signalled = true;
>  }
>  
>  void
>  util_queue_fence_destroy(struct util_queue_fence *fence)
>  {
>     assert(fence->signalled);
> +
> +   /* Protect against the following race:
> +    *
> +    * util_queue_fence_signal (begin)
> +    *   fence->signalled = true;
> +    *                                             util_queue_fence_is_signalled
> +    *                                             util_queue_fence_destroy
> +    *   cnd_broadcast(&fence->cond); <-- use-after-free
> +    * util_queue_fence_signal (end)
> +    */
> +   mtx_lock(&fence->mutex);
> +   mtx_unlock(&fence->mutex);

How's this protecting anything?
This feels completely wrong...

> +
>     cnd_destroy(&fence->cond);
>     mtx_destroy(&fence->mutex);
>  }
>  
>  /****************************************************************************
>   * util_queue implementation
>   */
>  
>  struct thread_input {
>     struct util_queue *queue;
> -- 
> 2.11.0
>