[vlc-devel] [PATCH 1/2] background_worker: Fix potential use after free

[Romain Vimont]

On 2/6/20 4:11 PM, Hugo Beauzée-Luyssen wrote:
> Since we free the task and only then lock the worker mutex to reset the
> task pointer, there's a window during which another thread could probe
> the dangling task pointer.
> 
> Such a case can be seen here: https://code.videolan.org/videolan/medialibrary/-/jobs/351960
> ---
>   src/misc/background_worker.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/misc/background_worker.c b/src/misc/background_worker.c
> index acb93cd0df..f649b402e0 100644
> --- a/src/misc/background_worker.c
> +++ b/src/misc/background_worker.c
> @@ -81,7 +81,6 @@ static struct task *task_Create(struct background_worker *worker, void *id,
>   static void task_Destroy(struct background_worker *worker, struct task *task)
>   {
>       worker->conf.pf_release(task->entity);
> -    free(task);
>   }

It seems weird to me that _Destroy() do not free the task (and thus is 
not symmetric with _Create()).

I suggest to call task_Destroy() after "thread->task = NULL;" (either 
within the mutex locked section, or after) in TerminateTask() instead.

Cheers

>   
>   static struct task *QueueTake(struct background_worker *worker, int timeout_ms)
> @@ -122,6 +121,7 @@ static void QueueRemoveAll(struct background_worker *worker, void *id)
>           {
>               vlc_list_remove(&task->node);
>               task_Destroy(worker, task);
> +            free(task);
>           }
>       }
>   }
> @@ -181,6 +181,7 @@ static void TerminateTask(struct background_thread *thread, struct task *task)
>       task_Destroy(worker, task);
>   
>       vlc_mutex_lock(&worker->lock);
> +    free(task);
>       thread->task = NULL;
>       worker->uncompleted--;
>       assert(worker->uncompleted >= 0);
>