Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

Hi Mat,

On 03/16/2016 03:01 PM, Mat Martineau wrote:
> The race condition test in test-main exposed a case where the events
> array returned by epoll_wait could have a stale watch_data
> pointer. This triggered a use-after-free error that was reported by
> the address sanitizer (./configure --enable-asan).
>
> When the event loop is running, watch_remove now queues the watch_data
> structure to be freed later. The watch_data callback is modified so
> that a safe function is executed if the event loop attempts a callback
> on a pending removed watch. Any queued watch_data structures are freed
> at the end of each event loop iteration.
> ---
>   ell/main.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/ell/main.c b/ell/main.c
> index f8fa4ac..135a6da 100644
> --- a/ell/main.c
> +++ b/ell/main.c
> @@ -56,6 +56,7 @@ static int idle_id;
>   static int exit_status = EXIT_FAILURE;
>
>   static struct l_queue *idle_list;
> +static struct l_queue *watch_free_list;
>
>   struct watch_data {
>   	int fd;
> @@ -101,6 +102,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
>
>   	idle_id = 0;
>
> +	watch_free_list = l_queue_new();

l_queue_new cannot fail.

> +	if (!watch_free_list)
> +		goto free_idle_list;
> +
>   	watch_entries = DEFAULT_WATCH_ENTRIES;
>
>   	for (i = 0; i < watch_entries; i++)
> @@ -108,6 +113,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
>
>   	return true;
>
> +free_idle_list:
> +	l_queue_destroy(idle_list, NULL);
> +	idle_list = NULL;
> +
>   free_watch_list:
>   	free(watch_list);
>   	watch_list = NULL;
> @@ -190,6 +199,11 @@ int watch_modify(int fd, uint32_t events, bool force)
>   	return 0;
>   }
>
> +static void watch_nop_callback(int fd, uint32_t events, void *user_data)
> +{
> +	return;
> +}
> +
>   int watch_remove(int fd)
>   {
>   	struct watch_data *data;
> @@ -212,7 +226,15 @@ int watch_remove(int fd)
>   	if (data->destroy)
>   		data->destroy(data->user_data);
>
> -	l_free(data);
> +	if (epoll_running) {
> +		/* The epoll events array may point to the same memory as
> +		 * 'data', so let the event loop free it later
> +		 */
> +		data->callback = watch_nop_callback;
> +		l_queue_push_tail(watch_free_list, data);

Why don't we do this exactly how the idle events are handled.  E.g. set 
a DESTROYED flag and prune the event list after the epoll processing is 
complete.

> +	} else {
> +		l_free(data);
> +	}
>
>   	return err;
>   }
> @@ -352,6 +374,7 @@ LIB_EXPORT int l_main_run(void)
>
>   		l_queue_foreach(idle_list, idle_dispatch, NULL);
>   		l_queue_foreach_remove(idle_list, idle_prune, NULL);
> +		l_queue_clear(watch_free_list, l_free);
>   	}
>
>   	for (i = 0; i < watch_entries; i++) {
> @@ -378,6 +401,9 @@ LIB_EXPORT int l_main_run(void)
>   	l_queue_destroy(idle_list, idle_destroy);
>   	idle_list = NULL;
>
> +	l_queue_destroy(watch_free_list, l_free);
> +	watch_free_list = NULL;
> +
>   	epoll_running = false;
>
>   	close(epoll_fd);
>

Regards,
-Denis