[PATCH weston] compositor: fix memory corruption when removing an output

[Pekka Paalanen]

On Sat,  6 Sep 2014 16:18:02 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> The destructor set on the wl_output resources needs the weston_output
> to be allocated, because it removes the resource from its list.
> So unset the destructor on all the resources when destroying an
> output.
> ---
>  src/compositor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 20ff6b2..e9c8a49 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -3248,6 +3248,8 @@ weston_compositor_remove_output(struct weston_compositor *compositor,
>  WL_EXPORT void
>  weston_output_destroy(struct weston_output *output)
>  {
> +	struct wl_resource *resource;
> +
>  	output->destroying = 1;
>  
>  	weston_compositor_remove_output(output->compositor, output);
> @@ -3261,6 +3263,10 @@ weston_output_destroy(struct weston_output *output)
>  	pixman_region32_fini(&output->previous_damage);
>  	output->compositor->output_id_pool &= ~(1 << output->id);
>  
> +	wl_resource_for_each(resource, &output->resource_list) {
> +		wl_resource_set_destructor(resource, NULL);
> +	}
> +
>  	wl_global_destroy(output->global);
>  }
>  

Hi,

yeah, I think this works. This is just one more case of making a
protocol object inert.

It would be more complicated if wl_output defined any requests, then
you'd need to reset the user data of the wl_resource, too. Which
reminds me that wl_output has no destructor protocol...

You're taking a shortcut here by not calling unbind_resource()
before setting the destructor to NULL. The resource is left in the
list, but since a) weston_output i.e. the list head is going away, and
b) there are no request handlers that might use wl_resource::link, it
should not be a problem. Just a lot of stale values that no-one should
ever touch... though poisoning those would be nice. ;-)

But yes, I understand the problem this solves, so pushed. I couldn't
reproduce the crash, though, maybe it needs both hot-unplug and
hot-plug.

A different dirty trick would have been to remove the list *head* from
the list, but I like this one better for the smaller WTF-factor. ;-)


Thanks,
pq