> Since a destroy signal inidicates the object is utterly dead, I don't think 
> it's unreasonable for users to have assumed that they don't have to clean up 
> their listener link.  It's *never* going to fire again, so why should 
> anything need to be removed?

This only implies that the signal is never fired.
If *any* listener that's on the signal wants to remove itself form the list, 
it will access the memory of the recently freed listener.

While this is unlikly in a codebase where the full implementation is in one 
hand, after they made the decision to handle things this way, it's possible if 
a library is used, that doesn't follow the convention.

> It seems that this patch makes that assumption invalid, and we would 
> need patches to weston, enlightenment, and mutter to prevent a 
> use-after-free during the signal emit?  Now I'm seeing valgrind errors 
> on E and weston during buffer destroy.
> 
> Personally, I don't think we should change this assumption and declare 
> the existing code that's worked for years suddenly buggy. :/

The code was buggy the whole time. Just because it was never triggered, does 
not imply it's not a bug.
free()ing these struct wl_list without removing them from the signal list 
leaves other struct wl_list that are outside the control of the current code 
in an invalid, prone to use-after-free, state.
Suddenly allowing this is a breaking API change (*some* struct wl_list inside 
a wl_listener) can suddenly become invalid for reasons outside the users 
control.

Related to this entire thing:
In [1] you added tests for this and promote something, that is in essence, a 
breaking change.

It also makes good wrapper implementations into managed languages annoying.
For example (admittedly my own) [2] ensures a wl_listener can never be lost 
and leak memory. It is freed when the Handle is GC'd.
To prevent any use-after-free into this wl_listener, it removes the listener 
from the list beforehand.
I would very much like to keep this code (since it is perfectly valid on the 
current ABI) and is good design in managed languages.

With kind regards,
ongy


[1] 
https://lists.freedesktop.org/archives/wayland-devel/2018-February/037169.html
[2] 
https://github.com/swaywm/hsroots/blob/master/src/Graphics/Wayland/Signal.hsc

P.S. sorry for format issues, or if doesn't get tagged into the thread 
properly, I wasn't subscribed to the list, when this came in and had to build 
references manually.

Attachment: signature.asc
Description: PGP signature

_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to