[PATH] core: implement a safe wl_signal_emit

[Derek Foreman]

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.

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

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

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.

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

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
>