On Fri, 23 Feb 2018 07:18:06 -0600 Derek Foreman <[email protected]> wrote:
> On 2018-02-23 02:15 AM, Pekka Paalanen wrote: > > On Thu, 22 Feb 2018 16:02:49 -0600 > > Derek Foreman <[email protected]> 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 <[email protected]> > >> --- > >> > >> 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
pgpMoVT_Ip5cV.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
