[PATCH v2] server: add wl_signal_emit_safe

[Simon Ser]

On Tuesday, September 18, 2018 8:43 PM, Derek Foreman <derek.foreman.samsung at gmail.com> 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 contact at emersion.fr
>
> 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