[PATCH weston] xwm: fix use after free

[Bryce Harrington]

On Fri, Dec 26, 2014 at 06:10:35PM +0200, Giulio Camuffo wrote:
> Calling wl_event_source_remove() will free the event source later, so
> reset the pointer to avoid calling it two times on the same pointer.
> Fix a compositor crash when copying some text from weston terminal,
> pasting it in the same terminal and hitting ctrl-u, while a X client
> is running.
> ---
>  xwayland/selection.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Looking at wl_event_source_remove(), I definitely see what you mean
about it delaying the free's.

Verifying the pointer does eventually get freed is a bit tricky but I
think I understand it: wl_event_source_remove() adds source->link to the
loop->destroy_list, then later on this list is processed by a C macro:

        wl_list_for_each_safe(source, next, &loop->destroy_list, link)
                free(source);

This uses wl_container_of() on the link stored in destroy_list to find
its parent container, source, and then frees it.

So, presuming my understanding of the logic is correct:

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>

> diff --git a/xwayland/selection.c b/xwayland/selection.c
> index b694477..24bd56d 100644
> --- a/xwayland/selection.c
> +++ b/xwayland/selection.c
> @@ -46,6 +46,7 @@ writable_callback(int fd, uint32_t mask, void *data)
>  		wm->property_reply = NULL;
>  		if (wm->property_source)
>  			wl_event_source_remove(wm->property_source);
> +		wm->property_source = NULL;
>  		close(fd);
>  		weston_log("write error to target fd: %m\n");
>  		return 1;
> @@ -61,6 +62,7 @@ writable_callback(int fd, uint32_t mask, void *data)
>  		wm->property_reply = NULL;
>  		if (wm->property_source)
>  			wl_event_source_remove(wm->property_source);

If we need to check wm->property_source before calling
wl_event_source_remove(), shouldn't this also be done for the subsequent
calls as well?

> +		wm->property_source = NULL;
>  
>  		if (wm->incr) {
>  			xcb_delete_property(wm->conn,
> @@ -352,6 +354,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>  		weston_log("read error from data source: %m\n");
>  		weston_wm_send_selection_notify(wm, XCB_ATOM_NONE);
>  		wl_event_source_remove(wm->property_source);
> +		wm->property_source = NULL;
>  		close(fd);
>  		wl_array_release(&wm->source_data);
>  	}
> @@ -375,6 +378,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>  			wm->selection_property_set = 1;
>  			wm->flush_property_on_delete = 1;
>  			wl_event_source_remove(wm->property_source);
> +			wm->property_source = NULL;
>  			weston_wm_send_selection_notify(wm, wm->selection_request.property);
>  		} else if (wm->selection_property_set) {
>  			weston_log("got %zu bytes, waiting for "
> @@ -382,6 +386,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>  
>  			wm->flush_property_on_delete = 1;
>  			wl_event_source_remove(wm->property_source);
> +			wm->property_source = NULL;
>  		} else {
>  			weston_log("got %zu bytes, "
>  				"property deleted, seting new property\n",
> @@ -395,6 +400,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>  		weston_wm_send_selection_notify(wm, wm->selection_request.property);
>  		xcb_flush(wm->conn);
>  		wl_event_source_remove(wm->property_source);
> +		wm->property_source = NULL;
>  		close(fd);
>  		wl_array_release(&wm->source_data);
>  		wm->selection_request.requestor = XCB_NONE;
> @@ -413,6 +419,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>  		}
>  		xcb_flush(wm->conn);
>  		wl_event_source_remove(wm->property_source);
> +		wm->property_source = NULL;
>  		close(wm->data_source_fd);
>  		wm->data_source_fd = -1;
>  		close(fd);
> -- 
> 2.2.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel