wl_list_remove(&view->link) point to invalid address

[Pekka Paalanen]

On Thu, 29 Mar 2018 11:01:47 +0800
zou lan <nancy.lan.zou at gmail.com> wrote:

> Hi Pekka & all
> 
> When I run some full screen applications under desktop shell,  then kill
> some applications randomly, I find
> crashes maybe happened sometimes.
> 
> Crash call stack:
> weston_view_destroy-->weston_view_unmap-->wl_list_reomve(&view->link).
> The view is black view.

Hi,

that's a good find. If I understand correctly, we do not handle the
case where a weston_view is effectively unmapped by removing it from a
weston_layer. As weston_compositor::view_list is never torn down
properly but simply rebuilt, the unmapped view is left with dangling
pointers, and those will be hit when finally destroying the view.

Would that be an accurate explanation?

> Then I debug code, find that shell.c  lower_fullscreen_layer function only
> delete the layer of black view from layer_list, but not handle the
> &view.link.
> lower_fullscreen_layer()
> {
> .....
>     /* Hide the black view */
> 
> weston_layer_entry_remove(&shsurf->fullscreen.black_view->layer_link);
> 
> wl_list_init(&shsurf->fullscreen.black_view->layer_link.link);

This wl_list_init() seems to be redundant, btw.

> 
> weston_view_damage_below(shsurf->fullscreen.black_view);

Could this path simply call weston_view_unmap() to unmap the black_view?

It effectively is unmapping the view, but forgetting to clear the
'is_mapped' boolean, which later leads to accessing weston_view::link
dangling pointers. weston_view_unmap() does a lot more which might be
necessary as well.

> 
> }
> 
> The code will be called when start second full screen application.
> 
> when weston build view list again, the first surface's black view is not in
> the view list, the list &view.link maybe point some invalid address.
> 
> I think ivi shell will have the same problems too.  I have a temporary
> patch to resolve this problem. I want to discuss it in the community.
> 
> option 1
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -2195,7 +2195,7 @@ view_list_add(struct weston_compositor *compositor,
>         struct weston_subsurface *sub;
> 
>         weston_view_update_transform(view);
> -
> +       wl_list_remove(&view.link);

For better or worse, the weston_view::link is deliberately left with
garbage pointers when the weston_compositor::view_list is discarded for
a rebuild.

Wouldn't this addition cause weston_compositor::view_list to become
corrupted if the link removed here happened to be the first or the last
in the old list order? Or maybe be get lucky most times, and it would
need quite special circumstances to trigger the corruption.

I still think this approach is not good.

> 
> option 2:
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3651,6 +3651,7 @@ lower_fullscreen_layer(struct desktop_shell *shell,
> 
> weston_layer_entry_remove(&shsurf->fullscreen.black_view->layer_link);
> 
> wl_list_init(&shsurf->fullscreen.black_view->layer_link.link);
> 
> weston_view_damage_below(shsurf->fullscreen.black_view);
> +
>  wl_list_remove(&shsurf->fullscreen.black_view->link);

Removing without init might cause issues with a potential double-remove
on e.g. view destruction.

> 
>                 }
> 
> Do you think these options reasonable? Thank you.

I think using weston_view_unmap() would be the best as I mentioned
above, unless there is some specific reason why it cannot be used. None
come to my mind this time.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180403/55757b6f/attachment.sig>