[RFC wayland] server: Add special case destroy signal emitter

[Pekka Paalanen]

On Fri, 23 Feb 2018 07:18:06 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 2018-02-23 02:15 AM, Pekka Paalanen wrote:
> > On Thu, 22 Feb 2018 16:02:49 -0600
> > Derek Foreman <derekf at osg.samsung.com> wrote:
> >   
> >> In the past much code (weston, efl/enlightenment, mutter) has
> >> freed structures containing wl_listeners from destroy handlers
> >> without first removing the listener from the signal.  As the
> >> destroy notifier only fires once, this has largely gone
> >> unnoticed until recently.
> >>
> >> Other code does not (Qt, wlroots) - and removes itself from
> >> the signal before free.
> >>
> >> If somehow a destroy signal is listened to by code from both
> >> kinds of callers, those that free will corrupt the lists for
> >> those that don't, and Bad Things will happen.
> >>
> >> To avoid these bad things, remove every item from the signal list
> >> during destroy emit, and put it in a list all its own.  This way
> >> whether the listener is removed or not has no impact on the
> >> following emits.
> >>
> >> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >> ---
> >>
> >>   src/wayland-private.h |  3 +++
> >>   src/wayland-server.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> >>   2 files changed, 49 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/wayland-private.h b/src/wayland-private.h
> >> index 12b9032..29516ec 100644
> >> --- a/src/wayland-private.h
> >> +++ b/src/wayland-private.h
> >> @@ -253,6 +253,9 @@ wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
> >>   void
> >>   wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
> >>   
> >> +void
> >> +wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data);
> >> +
> >>   void
> >>   wl_connection_close_fds_in(struct wl_connection *connection, int max);
> >>   
> >> diff --git a/src/wayland-server.c b/src/wayland-server.c
> >> index eb1e500..df5c16a 100644
> >> --- a/src/wayland-server.c
> >> +++ b/src/wayland-server.c
> >> @@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
> >>   	/* Don't emit the new signal for deprecated resources, as that would
> >>   	 * access memory outside the bounds of the deprecated struct */
> >>   	if (!resource_is_deprecated(resource))
> >> -		wl_priv_signal_emit(&resource->destroy_signal, resource);
> >> +		wl_priv_signal_final_emit(&resource->destroy_signal, resource);
> >>   
> >>   	if (resource->destroy)
> >>   		resource->destroy(resource);
> >> @@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
> >>   {
> >>   	uint32_t serial = 0;
> >>   
> >> -	wl_priv_signal_emit(&client->destroy_signal, client);
> >> +	wl_priv_signal_final_emit(&client->destroy_signal, client);
> >>   
> >>   	wl_client_flush(client);
> >>   	wl_map_for_each(&client->objects, destroy_resource, &serial);
> >> @@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
> >>   	struct wl_socket *s, *next;
> >>   	struct wl_global *global, *gnext;
> >>   
> >> -	wl_priv_signal_emit(&display->destroy_signal, display);
> >> +	wl_priv_signal_final_emit(&display->destroy_signal, display);
> >>   
> >>   	wl_list_for_each_safe(s, next, &display->socket_list, link) {
> >>   		wl_socket_destroy(s);
> >> @@ -2025,6 +2025,49 @@ wl_priv_signal_emit(struct wl_priv_signal *signal, void *data)
> >>   	}
> >>   }
> >>   
> >> +/** Emit the signal for the last time, calling all the installed listeners
> >> + *
> >> + * Iterate over all the listeners added to this \a signal and call
> >> + * their \a notify function pointer, passing on the given \a data.
> >> + * Removing or adding a listener from within wl_priv_signal_emit()
> >> + * is safe, as is freeing the structure containing the listener.
> >> + *
> >> + * A large body of external code assumes it's ok to free a destruction
> >> + * listener without removing that listener from the list.  Mixing code
> >> + * that acts like this and code that doesn't will result in list
> >> + * corruption.
> >> + *
> >> + * We resolve this by removing each item from the list and isolating it
> >> + * in another list.  We discard it completely after firing the notifier.
> >> + * This should allow interoperability between code that unlinks its
> >> + * destruction listeners and code that just frees structures they're in.
> >> + *
> >> + */
> >> +void
> >> +wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data)
> >> +{
> >> +	struct wl_listener *l;
> >> +	struct wl_list *pos;
> >> +
> >> +	wl_list_insert_list(&signal->emit_list, &signal->listener_list);
> >> +
> >> +	/* During a destructor notifier isolate every list item before
> >> +	 * notifying.  This renders harmless the long standing misuse
> >> +	 * of freeing listeners without removing them, but allows
> >> +	 * callers that do choose to remove them to interoperate with
> >> +	 * ones that don't. */
> >> +	while (!wl_list_empty(&signal->emit_list)) {
> >> +		wl_list_init(&signal->listener_list);
> >> +		pos = signal->emit_list.next;
> >> +		l = wl_container_of(pos, l, link);
> >> +
> >> +		wl_list_remove(pos);
> >> +		wl_list_insert(&signal->listener_list, pos);  
> > 
> > Just a quick fly-by comment; is there a reason to actually have a head
> > in the personal list for 'pos'? Wouldn't wl_list_init(pos) be enough?  
> 
> I was trying to keep wl_priv_signal_get() "working" - though I'm fairly 
> solidly convinced it's totally useless for that case...

Ooh, the signal_get... the existing wl_priv_signal_emit() seems to
already break signal_get for everything on the list, right?

I don't recall if we missed it on review or deemed it was not relied on.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180226/a71a0809/attachment.sig>