Re: [RFC wayland] server: Add special case destroy signal emitter
On Fri, 23 Feb 2018 07:18:06 -0600
Derek Foreman wrote:
> On 2018-02-23 02:15 AM, Pekka Paalanen wrote:
> > On Thu, 22 Feb 2018 16:02:49 -0600
> > 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
> >> ---
> >>
> >> 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);
> >>
Re: [RFC wayland] server: Add special case destroy signal emitter
On 2018-02-23 02:15 AM, Pekka Paalanen wrote:
On Thu, 22 Feb 2018 16:02:49 -0600
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
---
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...
Or would you want keep e.g
Re: [RFC wayland] server: Add special case destroy signal emitter
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
> des
Re: [RFC wayland] server: Add special case destroy signal emitter
On Thu, 22 Feb 2018 16:02:49 -0600
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
> ---
>
> 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?
Or would you want keep e.g. checks like
wl
