On Tuesday, September 18, 2018 8:43 PM, Derek Foreman <[email protected]> wrote: > On 2018-08-08 07:00 AM, Simon Ser wrote: > > > This new function allows listeners to remove themselves or any > > other listener when called. This version only works if listeners > > are properly removed before they are free'd. > > wl_signal_emit tries to be safe but it fails: it works fine if a > > handler removes its own listener, but not if it removes another > > one. > > It's not possible to patch wl_signal_emit directly as attempted > > in 1 because some projects using libwayland directly free > > destroy listeners without removing them from the list. Using this > > new strategy fails in this case, causing read-after-free errors. > > Signed-off-by: Simon Ser [email protected] > > Hi Simon, > > Is there intent to use this internally in libwayland (or in weston?) > somewhere in a further patch? > > Since there's no way to know from the callback whether the signal is > being emit "safely" or not, I'm a little concerned that developers might > get confused about what style of callback they're writing. Or people > will see "safe" and just lazily use that everywhere, even for destroy > signals... > > Also, it looks at a glance like all of the struct members and such > touched by this function are in public headers? I think the safe emit > function doesn't strictly need to be in libwayland at all? > > I don't really mean to block this work, just wondering what follows next. > > Some tiny comments inlined further down.
Hi, Thanks for you review. I have no plans to make libwayland or weston use this function in a future patch. It should be used by everybody who's sure users correctly remove destroy listeners (or doesn't care to break its ABI). Yeah, this function doesn't need to be in libwayland per se, in fact we already have it in wlroots and use it everywhere. I just thought users would benefit from having a safe version of wl_signal_emit in libwayland, because it's easy to remove multiple listeners from a destroy handler once you have multiple destroy signals and inter-dependant structs. It doesn't _need_ to be in libwayland, but putting it in libwayland would allow people to make sure they don't run into issues without needing to copy-paste the function from wlroots. People could use it without understanding why it's safe, but I prefer to have users running into issues because they forget to remove the destroy listener than having users running into issues because they use multiple wl_signals. What do you think? In the end it's not that a big deal, I just think it would be nice to have. Simon _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
