[RESEND PATCH weston v3] pixman: Destroy pixman images when underlying buffer is destroyed

[Kristian Høgsberg]

> On Dec 2, 2013, at 11:59 PM, Lubomir Rintel <lkundrak at v3.sk> wrote:
> 
>> On Mon, 2013-12-02 at 15:53 -0800, Kristian Høgsberg wrote:
>>> On Sat, Nov 30, 2013 at 03:41:00PM +0100, Lubomir Rintel wrote:
>>> While the pixman image might be attached, the underlying buffer might be
>>> already gone under certain circumstances. This is easily reproduced by
>>> attempting to resize gnome-terminal on a fbdev backend.
>>> 
>>> $ WAYLAND_DEBUG=1 strace -emunmap weston --backend=fbdev-backend.so
>>> ...
>>> [1524826.942] wl_shm at 7.create_pool(new id wl_shm_pool at 23, fd 40, 1563540)
>>> [1524827.315] wl_shm_pool at 23.create_buffer(new id wl_buffer at 24, 0, 759, 515, 3036, 0)
>>> ...
>>> [1524829.488] wl_surface at 14.attach(wl_buffer at 24, 0, 0)
>>> [1524829.766] wl_surface at 14.set_buffer_scale(1)
>>> [1524829.904] wl_surface at 14.damage(0, 0, 759, 515)
>>> [1524830.248] wl_surface at 14.frame(new id wl_callback at 25)
>>> [1524830.450] wl_surface at 14.commit()
>>> ...
>>> [1524846.706] wl_shm at 7.create_pool(new id wl_shm_pool at 26, fd 40, 1545000)
>>> [1524847.215] wl_shm_pool at 26.create_buffer(new id wl_buffer at 27, 0, 750, 515, 3000, 0)
>>> [1524847.735] wl_buffer at 24.destroy()
>>> [1524847.953]  -> wl_display at 1.delete_id(24)
>>> [1524848.144] wl_shm_pool at 23.destroy()
>>> munmap(0xb5b2e000, 1563540)             = 0
>>> [1524849.021]  -> wl_display at 1.delete_id(23)
>>> [1524849.425] wl_surface at 14.attach(wl_buffer at 27, 0, 0)
>>> [1524849.730] wl_surface at 14.set_buffer_scale(1)
>>> [1524849.821] wl_surface at 14.damage(0, 0, 750, 515)
>>> <No commit yet, so drawing is attempted from older buffer that used to be
>>> attached to the surface, which happens to come from a destroyed pool,
>>> resulting it an invalid read from address 0xb5b2e000>
>>> 
>>> Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>
>>> ---
>>> Hi,
>>> 
>>> This has been previously discussed in this thread:
>>> http://lists.freedesktop.org/archives/wayland-devel/2013-November/012122.html
>>> 
>>> Pekka doesn't mind, Arnaud's suggestion does not seem doable.
>>> Please let me know if there's anything else that should be done before this is 
>>> merged.
>>> 
>>> Thank you!
>>> Lubo
>>> 
>>> src/pixman-renderer.c | 27 ++++++++++++++++++++++++++-
>>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
>>> index 5961965..1a1e6ae 100644
>>> --- a/src/pixman-renderer.c
>>> +++ b/src/pixman-renderer.c
>>> @@ -42,6 +42,7 @@ struct pixman_surface_state {
>>>    pixman_image_t *image;
>>>    struct weston_buffer_reference buffer_ref;
>>> 
>>> +    struct wl_listener buffer_destroy_listener;
>>>    struct wl_listener surface_destroy_listener;
>>>    struct wl_listener renderer_destroy_listener;
>>> };
>>> @@ -450,6 +451,22 @@ pixman_renderer_flush_damage(struct weston_surface *surface)
>>> }
>>> 
>>> static void
>>> +buffer_state_handle_buffer_destroy(struct wl_listener *listener, void *data)
>>> +{
>>> +    struct pixman_surface_state *ps;
>>> +
>>> +    ps = container_of(listener, struct pixman_surface_state,
>>> +              buffer_destroy_listener);
>>> +
>>> +    if (ps->image) {
>>> +        pixman_image_unref(ps->image);
>>> +        ps->image = NULL;
>>> +    }
>>> +
>>> +    ps->buffer_destroy_listener.notify = NULL;
>>> +}
>>> +
>>> +static void
>>> pixman_renderer_attach(struct weston_surface *es, struct weston_buffer *buffer)
>>> {
>>>    struct pixman_surface_state *ps = get_surface_state(es);
>>> @@ -499,6 +516,13 @@ pixman_renderer_attach(struct weston_surface *es, struct weston_buffer *buffer)
>>>        buffer->width, buffer->height,
>>>        wl_shm_buffer_get_data(shm_buffer),
>>>        wl_shm_buffer_get_stride(shm_buffer));
>>> +
>>> +    if (ps->buffer_destroy_listener.notify)
>>> +        wl_list_remove(&ps->buffer_destroy_listener.link);
>> 
>> We need to remove the old listener early in the function, before
>> 
>>        if (!buffer)
>>                return;
>> 
>> so that we also remove the listener if a NULL buffer is attached.
> 
> Why? There's no harm invoking the listener if a null buffer has been
> reattached to a surface. It checks for actual presence for pixmap data
> and still gets unhooked upon surface destroy. (Apart from that the code
> might look a bit more elegant and the ps->image NULL check in the
> listener might be unnecessary.)

If you keep the buffer destroy listener when we attach a NULL buffer, attach another buffer and then destroy the first buffer, the first buffer destroy listener kicks in and sets the image to NULL.

Kristian

>> 
>> Kristian
>> 
>>> +    ps->buffer_destroy_listener.notify =
>>> +        buffer_state_handle_buffer_destroy;
>>> +    wl_signal_add(&buffer->destroy_signal,
>>> +              &ps->buffer_destroy_listener);
>>> }
>>> 
>>> static void
>>> @@ -506,7 +530,8 @@ pixman_renderer_surface_state_destroy(struct pixman_surface_state *ps)
>>> {
>>>    wl_list_remove(&ps->surface_destroy_listener.link);
>>>    wl_list_remove(&ps->renderer_destroy_listener.link);
>>> -
>>> +    if (ps->buffer_destroy_listener.notify)
>>> +        wl_list_remove(&ps->buffer_destroy_listener.link);
>>> 
>>>    ps->surface->renderer_state = NULL;
>>> 
>>> -- 
>>> 1.8.4.2
>>> 
>>> _______________________________________________
>>> wayland-devel mailing list
>>> wayland-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
>