Hi, I have a v2 RFC _emit_final based on your idea. It passes `make check` of libwayland and weston. It also passes the remove-without-free test I sent in another mail.
--- (This patch isn't quite intended to be merged, but to get feedback on the approach) This adds a wl_priv_signal_emit_final and a wl_signal_emit_final. The `_final` emit functions have the additional asumption that every listener currently in the list will be removed after the _emit. Either gracefully, or simply free()d under the asumption that a destroy signal will not be emitted more than once. These functions take advantage of this and allow to use both tyles in the same signal list. It obsoletes the wl_priv_signal for destroy signals. Non destroy wl_priv_signals currently have a stronger guarantee than normal signals, which forces us to either keep the type around, or use this as destroy signal path, and another safe version for other signals. --- src/wayland-private.h | 5 +++++ src/wayland-server.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/wayland-private.h b/src/wayland-private.h index 12b9032..76adb32 100644 --- a/src/wayland-private.h +++ b/src/wayland-private.h @@ -250,6 +250,11 @@ wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener); struct wl_listener * wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify); +void +wl_signal_emit_final(struct wl_list *signal, void *data); + +void +wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data); void wl_priv_signal_emit(struct wl_priv_signal *signal, void *data); diff --git a/src/wayland-server.c b/src/wayland-server.c index eb1e500..62fc71a 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_emit_final(&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_emit_final(&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_emit_final(&display->destroy_signal, display); wl_list_for_each_safe(s, next, &display->socket_list, link) { wl_socket_destroy(s); @@ -1992,6 +1992,50 @@ wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify) return NULL; } +/** + * This emits a signal under the asumpton that every listener in it will be + * removed from the list in some way. + * + * It supports listeners that remove themselves without issues. + * + * The situation where a listener is only free()d, but not removed from the + * list is also supported, to not break existing library users. + * + * The notify function can remove arbitrary elements from the wl_signal, and + * the wl_signal will be in a valid state at all times (s.t. later elements in + * the list not beeing free()d without removal, but that broke at any time). + * + * The idea is based on wl_priv_signal_emit, we iterate until a list is empty. + * In this case we don't need the emit_list, since we know our actual signal + * list will be empty in the end. + */ +void +wl_signal_emit_final(struct wl_list *signal, void *data) +{ + struct wl_listener *l; + struct wl_list *pos; + + while (!wl_list_empty(signal)) { + pos = signal->next; + + /* Remove and ensure validity of position element */ + wl_list_remove(pos); + wl_list_init(pos); + + l = wl_container_of(pos, l, link); + l->notify(l, data); + } +} + +/** + * see wl_signal_emit_final for more details + */ +void +wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data) +{ + wl_signal_emit_final(&signal->listener_list, data); +} + /** Emit the signal, calling all the installed listeners * * Iterate over all the listeners added to this \a signal and call -- 2.16.2 On 2018/2月/22 04:02, Derek Foreman 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 <der...@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); > + > + l->notify(l, data); > + } > +} > + > /** \endcond INTERNAL */ > > /** \cond */ /* Deprecated functions below. */ > -- > 2.14.3 > > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
signature.asc
Description: PGP signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel