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

[Pekka Paalanen]

On Wed, 4 Apr 2018 10:50:26 +0800
zou lan <nancy.lan.zou at gmail.com> wrote:

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

Hi,

weston_view_unmap() is *the* way to unmap a view, any view. Every bit
of code that wants to unmap a view should call weston_view_unmap() to
make sure it is done correctly. Open-coding bits of weston_view_unmap()
here and there will only lead to bugs and is harder to maintain.

So there needs to be a very good reason why something would not call
weston_view_unmap() and instead open-code something similar. If there
is a such a reason, it should be documented as a comment in the code.

My recommendation is to inspect all places where
weston_layer_entry_remove() is called, see if they are actually
unmapping a view, and if they are, either replace code in that place
with a call to weston_view_unmap() or add a comment on why this unmap
cannot be done by calling weston_view_unmap().

Mentioning weston_view_unmap() in a comment is also good for posterity,
because grepping for it will also reveal places where unmap is done
without calling it.

Unfortunately we don't have the symmetry with mapping a view. Mapping
has been open-coded everywhere, since it used to vary for every place
doing it. It is in my hopes to unify and clean that up some day as well.


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/20180404/575c79e2/attachment.sig>