[PATH] core: implement a safe wl_signal_emit

Thu Feb 22 16:48:51 UTC 2018

On 2018/2月/22 09:34, Derek Foreman wrote:
> On 2018-02-22 08:58 AM, Daniel Stone wrote:
> > Hi,
> > 
> > On 22 February 2018 at 14:14, Markus Ongyerth <wl at ongy.net> wrote:
> > > > 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.
> > 
> > There's a difference between something being 'buggy' and a design with
> > non-obvious details you might not like. If destroy handlers not
> > removing their list elements were buggy, we would be seeing bugs from
> > that. But instead it's part of the API contract: when a destroy signal
> > is invoked, you are guaranteed that this will be the first and only
> > access to your list member. This implies that anyone trying to remove
> > their link from the list (accessing other listeners in the list) is
> > buggy.
> > 
> > > 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.
> > 
> > I don't know if I've quite parsed this right, but as above, not
> > removing elements of a destroy listener list, when the listener is
> > invoked, is our current API.
> > 
> > > Related to this entire thing:
> > > In [1] you added tests for this and promote something, that is in essence, a
> > > breaking change.
> > 
> > It's not a breaking change though: it's the API we've pushed on everyone so far.
> 
> Also, it doesn't prevent external libraries from doing whichever they want
> if they have complete control of the destroy listener list contents.

So you suggest we break a now mandated api and expose ourselves to funny 
implementation detail changes that are now justified, because *we break API*?

> 
> What is prevented is libwayland's destroy notifier list walk accessing an
> element again after it is potentially freed by external code.

Which could be fixed by said node removing itself from a list, instead of 
leaving a list in invalid states for asumed behaviour.

> 
> We can completely replace the internal data structures in libwayland with
> whatever we want, but we must preserve that behaviour.

Why can we change one implementation detail, but have to keep another one?

> 
> It does not mean we can never rework the destroy signal emit path in
> libwayland to allow some items to be removed by the notification handlers
> and others just freed, or to allow a destroy notifier to touch the list.A

There is no destroy signal emit path.
There is a signal emit path, which may be called on destroy signals, and 
suddenly has to follow different semantics because of what exactly?
And how exactly do we expose that to our listeners? By the name of the signal 
in the struct?

> 
> It just makes it easy to verify that attempts to do that don't break
> guarantees we've always made.

Again, kindly point me to the point where an implementation detail made it 
towards a guarantee.
And for which signals exactly. Under which circumstances, oh and what 
guarantee exactly.

It would be nice if I had a proper proposal to take apart for this to be 
explictly added to the API.
Otherwise I can provide a (intentionaly snarky) one :)

Part one ammend [1] (wl_listener) with:
"The wl_list inside a wl_listener can be invalid (pointer towards free'd 
memory) at any time the listener notify is called. For further details see 
wl_signal.

Part two ammend [2] (wl_signal) with:
Signals with the name `*_destroy` have special semantics.
If they are currently emitted, any wl_signal_add/wl_signal_get on the signal 
or wl_list_remove on the link of any listener in it is invalid.
This is also the cause for invalid struct wl_list entries in wl_listener.

[1] https://wayland.freedesktop.org/docs/html/apc.html#Server-structwl__listener
[2] https://wayland.freedesktop.org/docs/html/apc.html#Server-structwl__signal

Sure, the exact way they are specified here is a bit funny.
We could also add that to the various `wl_*_add_destroy_listener` functions.
Then we'd have the (from libwayland side breaking) change that e.g.  
`wl_event_loop_get_destroy_listener` can't be called anymore under certain 
circumstances.

> 
> Thanks,
> Derek
> 
> > > 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.
> > 
> > Sure, that is annoying. In hindsight, it probably wasn't a good API
> > for particularly the new generation of managed languages. In the
> > meantime, probably the easiest way to do this, and come into line with
> > all the other users, would be to define a separate destroy-listener
> > type which intentionally leaks its wl_listener link after being
> > signaled, rather than removing it.
> > 
> > Cheers,
> > Daniel
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180222/14fec203/attachment.sig>