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

[zou lan]

Hi Pekka

Thank you for your comment. They are very helpful.

>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.

Your explanation is exactly correct.

I have verified the optional 1 can't work.

optional 2
>> 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.

yes.  I add the initialization for the list.

+ wl_list_init (&shsurf->fullscreen.black_view->link);

I agree with you weston_view_unmap maybe better for optional 2.  Since I
find crashes  caused by invalid pointer, I only concern how to make the
view.link to point  the right address.

No matter what functions use here, this only handle the black views.
Are there other views have the same problems?  Especially in ivi shell, I
find the crash exist in many  scenarios.

How would weston to resolve the potential risks? Thank you.

Best Regards
Nancy

2018-04-03 20:28 GMT+08:00 Pekka Paalanen <ppaalanen at gmail.com>:

> 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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180404/8aafede9/attachment-0001.html>