Discussion:
[PATCH wayland v4 00/11] Stop leaking file descriptors
(too old to reply)
Daniel Stone
2017-12-28 19:53:46 UTC
Permalink
Hi,
This is an update of Derek's patch series, after some discussions
between us. I've pushed patches 1-6 from his v3 series, and retained
8-10 almost completely untouched in this submission; the main change is
turning patch 7 into 5 patches. There are some more cosmetic changes
which Derek mostly acked on IRC, so I've still retained his S-o-b for
the patch.

In a minor change, zombie objects are only created opportunistically,
rather than always when creating the proxy; some of the logic has also
been slightly simplified.

Patches 5-7 are new, fixing issues with proxy refcounting I discovered
whilst reviewing and splitting these patches.

Cheers,
Daniel
Daniel Stone
2017-12-28 19:53:49 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

This makes it easier for future patches in the series, which can
possibly return NULL for extant map entries.

[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]

Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/wayland-client.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 8fc56340..83f76ec2 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -836,13 +836,12 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)

proxy = wl_map_lookup(&display->objects, id);

- if (!proxy)
- wl_log("error: received delete_id for unknown id (%u)\n", id);
-
- if (proxy && !wl_object_is_zombie(&display->objects, id))
+ if (wl_object_is_zombie(&display->objects, id))
+ wl_map_remove(&display->objects, id);
+ else if (proxy)
proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
else
- wl_map_remove(&display->objects, id);
+ wl_log("error: received delete_id for unknown id (%u)\n", id);

pthread_mutex_unlock(&display->mutex);
}
--
2.14.3
Daniel Stone
2017-12-28 19:53:50 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Since we now have the WL_MAP_ENTRY_ZOMBIE flag to determine whether or
not a client-side object is a zombie, we can remove the faux object.

[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/wayland-client.c | 2 +-
src/wayland-private.h | 3 ---
src/wayland-util.c | 2 --
3 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 83f76ec2..21ef5b04 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -411,7 +411,7 @@ proxy_destroy(struct wl_proxy *proxy)
wl_map_insert_at(&proxy->display->objects,
WL_MAP_ENTRY_ZOMBIE,
proxy->object.id,
- WL_ZOMBIE_OBJECT);
+ NULL);
} else {
wl_map_insert_at(&proxy->display->objects, 0,
proxy->object.id, NULL);
diff --git a/src/wayland-private.h b/src/wayland-private.h
index b0d129f7..8e94864a 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -57,9 +57,6 @@ struct wl_object {
uint32_t id;
};

-extern struct wl_object global_zombie_object;
-#define WL_ZOMBIE_OBJECT ((void*)&global_zombie_object)
-
int
wl_interface_equal(const struct wl_interface *iface1,
const struct wl_interface *iface2);
diff --git a/src/wayland-util.c b/src/wayland-util.c
index ce387f4b..3a471a88 100644
--- a/src/wayland-util.c
+++ b/src/wayland-util.c
@@ -153,8 +153,6 @@ wl_array_copy(struct wl_array *array, struct wl_array *source)

/** \cond */

-struct wl_object global_zombie_object;
-
int
wl_interface_equal(const struct wl_interface *a, const struct wl_interface *b)
{
--
2.14.3
Daniel Stone
2017-12-28 19:53:47 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Add a helper function which determines whether or not an object is a
zombie.

[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/connection.c | 10 +++++++++-
src/wayland-client.c | 4 ++--
src/wayland-private.h | 3 +++
3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index e92de794..2e234ea5 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -831,6 +831,14 @@ wl_connection_demarshal(struct wl_connection *connection,
return NULL;
}

+bool
+wl_object_is_zombie(struct wl_map *map, uint32_t id)
+{
+ struct wl_object *object = wl_map_lookup(map, id);
+
+ return (object == WL_ZOMBIE_OBJECT);
+}
+
int
wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)
{
@@ -852,7 +860,7 @@ wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)
closure->args[i].o = NULL;

object = wl_map_lookup(objects, id);
- if (object == WL_ZOMBIE_OBJECT) {
+ if (wl_object_is_zombie(objects, id)) {
/* references object we've already
* destroyed client side */
object = NULL;
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 795b4e9e..126dd908 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -837,7 +837,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
if (!proxy)
wl_log("error: received delete_id for unknown id (%u)\n", id);

- if (proxy && proxy != WL_ZOMBIE_OBJECT)
+ if (proxy && !wl_object_is_zombie(&display->objects, id))
proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
else
wl_map_remove(&display->objects, id);
@@ -1253,7 +1253,7 @@ queue_event(struct wl_display *display, int len)
return 0;

proxy = wl_map_lookup(&display->objects, id);
- if (!proxy || proxy == WL_ZOMBIE_OBJECT) {
+ if (!proxy || wl_object_is_zombie(&display->objects, id)) {
wl_connection_consume(display->connection, size);
return size;
}
diff --git a/src/wayland-private.h b/src/wayland-private.h
index 93cec6be..c82b1f3e 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -190,6 +190,9 @@ wl_connection_demarshal(struct wl_connection *connection,
struct wl_map *objects,
const struct wl_message *message);

+bool
+wl_object_is_zombie(struct wl_map *map, uint32_t id);
+
int
wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects);
--
2.14.3
Daniel Stone
2017-12-28 19:53:48 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Add a new map entry flag to indicate that the object received is valid,
but a zombie. Previously this relied on a fixed object pointer, but
future patches in this series will have map entries returning either
NULL, or a different structure type entirely, for zombie objects.

wl_object_is_zombie() now solely uses the new flag to determine whether
or not the object is a zombie.

[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/connection.c | 13 +++++++++++--
src/wayland-client.c | 14 ++++++++------
src/wayland-private.h | 3 ++-
3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 2e234ea5..426be574 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -834,9 +834,18 @@ wl_connection_demarshal(struct wl_connection *connection,
bool
wl_object_is_zombie(struct wl_map *map, uint32_t id)
{
- struct wl_object *object = wl_map_lookup(map, id);
+ uint32_t flags;

- return (object == WL_ZOMBIE_OBJECT);
+ /* Zombie objects only exist on the client side. */
+ if (map->side == WL_MAP_SERVER_SIDE)
+ return false;
+
+ /* Zombie objects can only have been created by the client. */
+ if (id >= WL_SERVER_ID_START)
+ return false;
+
+ flags = wl_map_lookup_flags(map, id);
+ return !!(flags & WL_MAP_ENTRY_ZOMBIE);
}

int
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 126dd908..8fc56340 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -405,15 +405,17 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
static void
proxy_destroy(struct wl_proxy *proxy)
{
- if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)
+ if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
wl_map_remove(&proxy->display->objects, proxy->object.id);
- else if (proxy->object.id < WL_SERVER_ID_START)
- wl_map_insert_at(&proxy->display->objects, 0,
- proxy->object.id, WL_ZOMBIE_OBJECT);
- else
+ } else if (proxy->object.id < WL_SERVER_ID_START) {
+ wl_map_insert_at(&proxy->display->objects,
+ WL_MAP_ENTRY_ZOMBIE,
+ proxy->object.id,
+ WL_ZOMBIE_OBJECT);
+ } else {
wl_map_insert_at(&proxy->display->objects, 0,
proxy->object.id, NULL);
-
+ }

proxy->flags |= WL_PROXY_FLAG_DESTROYED;

diff --git a/src/wayland-private.h b/src/wayland-private.h
index c82b1f3e..b0d129f7 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -69,7 +69,8 @@ wl_interface_equal(const struct wl_interface *iface1,
* flags. If more flags are ever added, the implementation of wl_map will have
* to change to allow for new flags */
enum wl_map_entry_flags {
- WL_MAP_ENTRY_LEGACY = (1 << 0)
+ WL_MAP_ENTRY_LEGACY = (1 << 0), /* Server side only */
+ WL_MAP_ENTRY_ZOMBIE = (1 << 0) /* Client side only */
};

struct wl_map {
--
2.14.3
Daniel Stone
2017-12-28 19:53:53 UTC
Permalink
Closures created to hold events which will be dispatched on the client,
take a reference to the proxy for the object the event was sent to, as
well as the proxies for all objects referenced in that event.

These references are dropped immediately before dispatch, with the
display lock also being released. This leaves the potential for a
vanishingly small race, where another thread drops the last reference
on one of the proxies used in an event as it is being dispatched.

Fix this by splitting decrease_closure_args_refcount into two functions:
one which validates the objects (to ensure that clients are not returned
objects which they have destroyed), and another which unrefs all proxies
on the closure (object event was sent to, all referenced objects) as
well as the closure itself. For symmetry, increase_closure_args_refcount
is now the place where the refcount for the proxy for the object the
event was sent to, is increased.

This also happens to fix a bug: previously, if an event was sent to a
client-destroyed object, and the event had object arguments, a reference
would be leaked on the proxy for each of the object arguments.

Found by inspection whilst reviewing the zombie-FD-leak series.

Signed-off-by: Daniel Stone <***@collabora.com>
Cc: Jonas Ådahl <***@gmail.com>
Cc: Derek Foreman <***@osg.samsung.com>
Cc: Pekka Paalanen <***@collabora.co.uk>
---
src/wayland-client.c | 57 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index f3d71b0e..987418a1 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -236,7 +236,7 @@ wl_proxy_unref(struct wl_proxy *proxy)
}

static void
-decrease_closure_args_refcount(struct wl_closure *closure)
+validate_closure_objects(struct wl_closure *closure)
{
const char *signature;
struct argument_details arg;
@@ -251,16 +251,44 @@ decrease_closure_args_refcount(struct wl_closure *closure)
case 'n':
case 'o':
proxy = (struct wl_proxy *) closure->args[i].o;
- if (proxy) {
- if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
- closure->args[i].o = NULL;
+ if (proxy && proxy->flags & WL_PROXY_FLAG_DESTROYED)
+ closure->args[i].o = NULL;
+ break;
+ default:
+ break;
+ }
+ }
+}
+
+/* Destroys a closure which was demarshaled for dispatch; unrefs all the
+ * proxies in its arguments, as well as its own proxy, and destroys the
+ * closure itself. */
+static void
+destroy_queued_closure(struct wl_closure *closure)
+{
+ const char *signature;
+ struct argument_details arg;
+ struct wl_proxy *proxy;
+ int i, count;
+
+ signature = closure->message->signature;
+ count = arg_count_for_signature(signature);
+ for (i = 0; i < count; i++) {
+ signature = get_next_argument(signature, &arg);
+ switch (arg.type) {
+ case 'n':
+ case 'o':
+ proxy = (struct wl_proxy *) closure->args[i].o;
+ if (proxy)
wl_proxy_unref(proxy);
- }
break;
default:
break;
}
}
+
+ wl_proxy_unref(closure->proxy);
+ wl_closure_destroy(closure);
}

static void
@@ -272,9 +300,7 @@ wl_event_queue_release(struct wl_event_queue *queue)
closure = container_of(queue->event_list.next,
struct wl_closure, link);
wl_list_remove(&closure->link);
- decrease_closure_args_refcount(closure);
- wl_proxy_unref(closure->proxy);
- wl_closure_destroy(closure);
+ destroy_queued_closure(closure);
}
}

@@ -1232,6 +1258,8 @@ increase_closure_args_refcount(struct wl_closure *closure)
break;
}
}
+
+ closure->proxy->refcount++;
}

static int
@@ -1273,9 +1301,8 @@ queue_event(struct wl_display *display, int len)
return -1;
}

- increase_closure_args_refcount(closure);
- proxy->refcount++;
closure->proxy = proxy;
+ increase_closure_args_refcount(closure);

if (proxy == &display->proxy)
queue = &display->display_queue;
@@ -1302,13 +1329,11 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)

/* Verify that the receiving object is still valid by checking if has
* been destroyed by the application. */
-
- decrease_closure_args_refcount(closure);
+ validate_closure_objects(closure);
proxy = closure->proxy;
proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
- wl_proxy_unref(proxy);
if (proxy_destroyed) {
- wl_closure_destroy(closure);
+ destroy_queued_closure(closure);
return;
}

@@ -1328,9 +1353,9 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
&proxy->object, opcode, proxy->user_data);
}

- wl_closure_destroy(closure);
-
pthread_mutex_lock(&display->mutex);
+
+ destroy_queued_closure(closure);
}

static int
--
2.14.3
Derek Foreman
2018-01-03 22:00:19 UTC
Permalink
Post by Daniel Stone
Closures created to hold events which will be dispatched on the client,
take a reference to the proxy for the object the event was sent to, as
well as the proxies for all objects referenced in that event.
These references are dropped immediately before dispatch, with the
display lock also being released. This leaves the potential for a
vanishingly small race, where another thread drops the last reference
on one of the proxies used in an event as it is being dispatched.
one which validates the objects (to ensure that clients are not returned
objects which they have destroyed), and another which unrefs all proxies
on the closure (object event was sent to, all referenced objects) as
well as the closure itself. For symmetry, increase_closure_args_refcount
is now the place where the refcount for the proxy for the object the
event was sent to, is increased.
This also happens to fix a bug: previously, if an event was sent to a
client-destroyed object, and the event had object arguments, a reference
would be leaked on the proxy for each of the object arguments.
Found by inspection whilst reviewing the zombie-FD-leak series.
---
src/wayland-client.c | 57 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 16 deletions(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index f3d71b0e..987418a1 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -236,7 +236,7 @@ wl_proxy_unref(struct wl_proxy *proxy)
}
static void
-decrease_closure_args_refcount(struct wl_closure *closure)
+validate_closure_objects(struct wl_closure *closure)
{
const char *signature;
struct argument_details arg;
@@ -251,16 +251,44 @@ decrease_closure_args_refcount(struct wl_closure *closure)
proxy = (struct wl_proxy *) closure->args[i].o;
- if (proxy) {
- if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
- closure->args[i].o = NULL;
+ if (proxy && proxy->flags & WL_PROXY_FLAG_DESTROYED)
+ closure->args[i].o = NULL;
+ break;
+ break;
+ }
+ }
+}
+
+/* Destroys a closure which was demarshaled for dispatch; unrefs all the
+ * proxies in its arguments, as well as its own proxy, and destroys the
+ * closure itself. */
+static void
+destroy_queued_closure(struct wl_closure *closure)
+{
+ const char *signature;
+ struct argument_details arg;
+ struct wl_proxy *proxy;
+ int i, count;
+
+ signature = closure->message->signature;
+ count = arg_count_for_signature(signature);
+ for (i = 0; i < count; i++) {
+ signature = get_next_argument(signature, &arg);
+ switch (arg.type) {
+ proxy = (struct wl_proxy *) closure->args[i].o;
+ if (proxy)
wl_proxy_unref(proxy);
- }
break;
break;
}
}
+
+ wl_proxy_unref(closure->proxy);
+ wl_closure_destroy(closure);
}
static void
@@ -272,9 +300,7 @@ wl_event_queue_release(struct wl_event_queue *queue)
closure = container_of(queue->event_list.next,
struct wl_closure, link);
wl_list_remove(&closure->link);
- decrease_closure_args_refcount(closure);
- wl_proxy_unref(closure->proxy);
- wl_closure_destroy(closure);
+ destroy_queued_closure(closure);
}
}
@@ -1232,6 +1258,8 @@ increase_closure_args_refcount(struct wl_closure *closure)
break;
}
}
+
+ closure->proxy->refcount++;
}
static int
@@ -1273,9 +1301,8 @@ queue_event(struct wl_display *display, int len)
return -1;
}
- increase_closure_args_refcount(closure);
- proxy->refcount++;
closure->proxy = proxy;
+ increase_closure_args_refcount(closure);
Ok, queue_event() is always called with display lock held so there's no
similar increase_closure_args race.

I guess I'd have done the proxy->refcount++ move into
increase_closure_args_refcount() in a separate patch.
Post by Daniel Stone
if (proxy == &display->proxy)
queue = &display->display_queue;
@@ -1302,13 +1329,11 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
/* Verify that the receiving object is still valid by checking if has
* been destroyed by the application. */
-
- decrease_closure_args_refcount(closure);
+ validate_closure_objects(closure);
proxy = closure->proxy;
proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
- wl_proxy_unref(proxy);
if (proxy_destroyed) {
- wl_closure_destroy(closure);
+ destroy_queued_closure(closure);
return;
}
@@ -1328,9 +1353,9 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
&proxy->object, opcode, proxy->user_data);
}
- wl_closure_destroy(closure);
-
This (wl_closure_destroy occurring with lock held) was bothering me when
I noticed it while reviewing your other patches - though I'd probably
have moved it in a separate patch.

Nice catch.
Post by Daniel Stone
pthread_mutex_lock(&display->mutex);
+
+ destroy_queued_closure(closure);
}
static int
Daniel Stone
2017-12-28 19:53:55 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

We need to close file descriptors sent to zombie proxies to avoid leaking
them, and perhaps more importantly, to prevent them from being dispatched
in events on other objects (since they would previously be left in the
buffer and potentially fed to following events destined for live proxies)

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/connection.c | 6 ++++++
src/wayland-client.c | 8 ++++++++
src/wayland-private.h | 3 +++
3 files changed, 17 insertions(+)

diff --git a/src/connection.c b/src/connection.c
index 426be574..6f83bab2 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -192,6 +192,12 @@ close_fds(struct wl_buffer *buffer, int max)
buffer->tail += size;
}

+void
+wl_connection_close_fds_in(struct wl_connection *connection, int max)
+{
+ close_fds(&connection->fds_in, max);
+}
+
int
wl_connection_destroy(struct wl_connection *connection)
{
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 62d4f222..c1369b88 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -1353,8 +1353,16 @@ queue_event(struct wl_display *display, int len)
if (len < size)
return 0;

+ /* If our proxy is gone or a zombie, just eat the event (and any FDs,
+ * if applicable). */
proxy = wl_map_lookup(&display->objects, id);
if (!proxy || wl_object_is_zombie(&display->objects, id)) {
+ struct wl_zombie *zombie = wl_map_lookup(&display->objects, id);
+
+ if (zombie)
+ wl_connection_close_fds_in(display->connection,
+ zombie->fd_count[opcode]);
+
wl_connection_consume(display->connection, size);
return size;
}
diff --git a/src/wayland-private.h b/src/wayland-private.h
index 8e94864a..12b9032c 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -253,4 +253,7 @@ 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_connection_close_fds_in(struct wl_connection *connection, int max);
+
#endif
--
2.14.3
Daniel Stone
2017-12-28 19:53:54 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Using the singleton zombie object doesn't allow us to posthumously retain
object interface information, which makes it difficult to properly inter
future events destined for the recently deceased proxy.

Notably, this makes it impossible for zombie proxy destined file
descriptors to be properly consumed.

When we create a proxy, we now create a zombie-state object to hold
information about the file descriptors in events it can receive. This
will allow us, in a future patch, to close those FDs.

[daniels: Split Derek's patch into a few smaller ones.]

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
src/wayland-client.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 78 insertions(+), 4 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 987418a1..62d4f222 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -55,6 +55,11 @@ enum wl_proxy_flag {
WL_PROXY_FLAG_WRAPPER = (1 << 2),
};

+struct wl_zombie {
+ int event_count;
+ int *fd_count;
+};
+
struct wl_proxy {
struct wl_object object;
struct wl_display *display;
@@ -350,6 +355,66 @@ wl_display_create_queue(struct wl_display *display)
return queue;
}

+static int
+message_count_fds(const char *signature)
+{
+ unsigned int count, i, fds = 0;
+ struct argument_details arg;
+
+ count = arg_count_for_signature(signature);
+ for (i = 0; i < count; i++) {
+ signature = get_next_argument(signature, &arg);
+ if (arg.type == 'h')
+ fds++;
+ }
+
+ return fds;
+}
+
+static struct wl_zombie *
+prepare_zombie(struct wl_proxy *proxy)
+{
+ const struct wl_interface *interface = proxy->object.interface;
+ const struct wl_message *message;
+ int i, count;
+ struct wl_zombie *zombie = NULL;
+
+ /* If we hit an event with an FD, ensure we have a zombie object and
+ * fill the fd_count slot for that event with the number of FDs for
+ * that event. Interfaces with no events containing FDs will not have
+ * zombie objects created. */
+ for (i = 0; i < interface->event_count; i++) {
+ message = &interface->events[i];
+ count = message_count_fds(message->signature);
+
+ if (!count)
+ continue;
+
+ if (!zombie) {
+ zombie = zalloc(sizeof(*zombie) +
+ (interface->event_count * sizeof(int)));
+ if (!zombie)
+ return NULL;
+
+ zombie->event_count = interface->event_count;
+ zombie->fd_count = (int *) &zombie[1];
+ }
+
+ zombie->fd_count[i] = count;
+ }
+
+ return zombie;
+}
+
+static enum wl_iterator_result
+free_zombies(void *element, void *data, uint32_t flags)
+{
+ if (flags & WL_MAP_ENTRY_ZOMBIE)
+ free(element);
+
+ return WL_ITERATOR_CONTINUE;
+}
+
static struct wl_proxy *
proxy_create(struct wl_proxy *factory, const struct wl_interface *interface,
uint32_t version)
@@ -434,10 +499,14 @@ proxy_destroy(struct wl_proxy *proxy)
if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
wl_map_remove(&proxy->display->objects, proxy->object.id);
} else if (proxy->object.id < WL_SERVER_ID_START) {
+ struct wl_zombie *zombie = prepare_zombie(proxy);
+
+ /* The map now contains the zombie entry, until the delete_id
+ * event arrives. */
wl_map_insert_at(&proxy->display->objects,
WL_MAP_ENTRY_ZOMBIE,
proxy->object.id,
- NULL);
+ zombie);
} else {
wl_map_insert_at(&proxy->display->objects, 0,
proxy->object.id, NULL);
@@ -860,12 +929,16 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)

proxy = wl_map_lookup(&display->objects, id);

- if (wl_object_is_zombie(&display->objects, id))
+ if (wl_object_is_zombie(&display->objects, id)) {
+ /* For zombie objects, the 'proxy' is actually the zombie
+ * event-information structure, which we can free. */
+ free(proxy);
wl_map_remove(&display->objects, id);
- else if (proxy)
+ } else if (proxy) {
proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
- else
+ } else {
wl_log("error: received delete_id for unknown id (%u)\n", id);
+ }

pthread_mutex_unlock(&display->mutex);
}
@@ -1087,6 +1160,7 @@ WL_EXPORT void
wl_display_disconnect(struct wl_display *display)
{
wl_connection_destroy(display->connection);
+ wl_map_for_each(&display->objects, free_zombies, NULL);
wl_map_release(&display->objects);
wl_event_queue_release(&display->default_queue);
wl_event_queue_release(&display->display_queue);
--
2.14.3
Derek Foreman
2018-01-03 22:25:18 UTC
Permalink
Post by Daniel Stone
Using the singleton zombie object doesn't allow us to posthumously retain
object interface information, which makes it difficult to properly inter
future events destined for the recently deceased proxy.
Notably, this makes it impossible for zombie proxy destined file
descriptors to be properly consumed.
When we create a proxy, we now create a zombie-state object to hold
information about the file descriptors in events it can receive. This
will allow us, in a future patch, to close those FDs.
[daniels: Split Derek's patch into a few smaller ones.]
---
src/wayland-client.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 78 insertions(+), 4 deletions(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 987418a1..62d4f222 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -55,6 +55,11 @@ enum wl_proxy_flag {
WL_PROXY_FLAG_WRAPPER = (1 << 2),
};
+struct wl_zombie {
+ int event_count;
+ int *fd_count;
+};
+
struct wl_proxy {
struct wl_object object;
struct wl_display *display;
@@ -350,6 +355,66 @@ wl_display_create_queue(struct wl_display *display)
return queue;
}
+static int
+message_count_fds(const char *signature)
+{
+ unsigned int count, i, fds = 0;
+ struct argument_details arg;
+
+ count = arg_count_for_signature(signature);
+ for (i = 0; i < count; i++) {
+ signature = get_next_argument(signature, &arg);
+ if (arg.type == 'h')
+ fds++;
+ }
+
+ return fds;
+}
+
+static struct wl_zombie *
+prepare_zombie(struct wl_proxy *proxy)
+{
+ const struct wl_interface *interface = proxy->object.interface;
+ const struct wl_message *message;
+ int i, count;
+ struct wl_zombie *zombie = NULL;
+
+ /* If we hit an event with an FD, ensure we have a zombie object and
+ * fill the fd_count slot for that event with the number of FDs for
+ * that event. Interfaces with no events containing FDs will not have
+ * zombie objects created. */
+ for (i = 0; i < interface->event_count; i++) {
+ message = &interface->events[i];
+ count = message_count_fds(message->signature);
+
+ if (!count)
+ continue;
+
+ if (!zombie) {
+ zombie = zalloc(sizeof(*zombie) +
+ (interface->event_count * sizeof(int)));
+ if (!zombie)
+ return NULL;
+
+ zombie->event_count = interface->event_count;
+ zombie->fd_count = (int *) &zombie[1];
+ }
+
+ zombie->fd_count[i] = count;
+ }
+
+ return zombie;
+}
+
+static enum wl_iterator_result
+free_zombies(void *element, void *data, uint32_t flags)
+{
+ if (flags & WL_MAP_ENTRY_ZOMBIE)
+ free(element);
+
+ return WL_ITERATOR_CONTINUE;
+}
+
static struct wl_proxy *
proxy_create(struct wl_proxy *factory, const struct wl_interface *interface,
uint32_t version)
@@ -434,10 +499,14 @@ proxy_destroy(struct wl_proxy *proxy)
if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
wl_map_remove(&proxy->display->objects, proxy->object.id);
} else if (proxy->object.id < WL_SERVER_ID_START) {
+ struct wl_zombie *zombie = prepare_zombie(proxy);
I think we discussed this on irc and I said this patch was ok, but I
missed this change...

prepare_zombie(proxy) should probably only be called from
wl_proxy_create and create_for_id - calling it in the destroy path
results in a malloc() call during deleting.

Should the malloc() fail and we can't actually allocate the zombie at
during deletion we're going to have a problem.

Otherwise, I like the way you've split up the series, and I've reviewed
your new patches.

Thanks,
Derek
Post by Daniel Stone
+
+ /* The map now contains the zombie entry, until the delete_id
+ * event arrives. */
wl_map_insert_at(&proxy->display->objects,
WL_MAP_ENTRY_ZOMBIE,
proxy->object.id,
- NULL);
+ zombie);
} else {
wl_map_insert_at(&proxy->display->objects, 0,
proxy->object.id, NULL);
@@ -860,12 +929,16 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
proxy = wl_map_lookup(&display->objects, id);
- if (wl_object_is_zombie(&display->objects, id))
+ if (wl_object_is_zombie(&display->objects, id)) {
+ /* For zombie objects, the 'proxy' is actually the zombie
+ * event-information structure, which we can free. */
+ free(proxy);
wl_map_remove(&display->objects, id);
- else if (proxy)
+ } else if (proxy) {
proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
- else
+ } else {
wl_log("error: received delete_id for unknown id (%u)\n", id);
+ }
pthread_mutex_unlock(&display->mutex);
}
@@ -1087,6 +1160,7 @@ WL_EXPORT void
wl_display_disconnect(struct wl_display *display)
{
wl_connection_destroy(display->connection);
+ wl_map_for_each(&display->objects, free_zombies, NULL);
wl_map_release(&display->objects);
wl_event_queue_release(&display->default_queue);
wl_event_queue_release(&display->display_queue);
Daniel Stone
2018-01-09 15:24:41 UTC
Permalink
Hi,
Post by Daniel Stone
@@ -434,10 +499,14 @@ proxy_destroy(struct wl_proxy *proxy)
if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
wl_map_remove(&proxy->display->objects, proxy->object.id);
} else if (proxy->object.id < WL_SERVER_ID_START) {
+ struct wl_zombie *zombie = prepare_zombie(proxy);
I think we discussed this on irc and I said this patch was ok, but I missed
this change...
prepare_zombie(proxy) should probably only be called from wl_proxy_create
and create_for_id - calling it in the destroy path results in a malloc()
call during deleting.
Should the malloc() fail and we can't actually allocate the zombie at during
deletion we're going to have a problem.
Hm, yes. On the other hand, if we can't allocate the zombie object,
then we probably won't be able to allocate the closure when
demarshaling the events either?

The other thing I started doing before this move, was just allocating
zombie on the end of the wl_proxy so we only have one allocation and
free. It does mean that proxies will consume more memory when undead,
I suppose. I don't have strong feelings either way, so am happy to
with any of those three ...
Otherwise, I like the way you've split up the series, and I've reviewed your
new patches.
Thanks a lot! I'll leave them sit for a couple more days to see if
anyone spots anything and then just push them if not.

Cheers,
Daniel
Derek Foreman
2018-01-10 17:03:03 UTC
Permalink
Post by Daniel Stone
Hi,
Post by Daniel Stone
@@ -434,10 +499,14 @@ proxy_destroy(struct wl_proxy *proxy)
if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
wl_map_remove(&proxy->display->objects, proxy->object.id);
} else if (proxy->object.id < WL_SERVER_ID_START) {
+ struct wl_zombie *zombie = prepare_zombie(proxy);
I think we discussed this on irc and I said this patch was ok, but I missed
this change...
prepare_zombie(proxy) should probably only be called from wl_proxy_create
and create_for_id - calling it in the destroy path results in a malloc()
call during deleting.
Should the malloc() fail and we can't actually allocate the zombie at during
deletion we're going to have a problem.
Hm, yes. On the other hand, if we can't allocate the zombie object,
then we probably won't be able to allocate the closure when
demarshaling the events either?
I suppose since OOM situations can be ephemeral this isn't universally
true, but I guess in situations like that we're going to fall apart
rather soon after anyway.
Post by Daniel Stone
The other thing I started doing before this move, was just allocating
zombie on the end of the wl_proxy so we only have one allocation and
free. It does mean that proxies will consume more memory when undead,
I suppose. I don't have strong feelings either way, so am happy to
with any of those three ...
I'm not really tied to any specific implementation method. That way
would be fine with me too if you prefer it. I don't think the memory
increase will be significant.

For reference,
https://lists.freedesktop.org/archives/wayland-devel/2017-April/033877.html

Was where Pekka suggested the up-front allocation to avoid problems in
low memory conditions.

I suspect 100% of the software I work on on a daily basis will explode
in completely unpredictable and undiagnosable ways in response to a
malloc() failure anyway, so you can have my

Reviewed-by: Derek Foreman <***@osg.samsung.com>

for the current approach - but I would like to give Pekka an opportunity
to veto, since I think strictly speaking he's correct. :)

Thanks,
Derek
Post by Daniel Stone
Otherwise, I like the way you've split up the series, and I've reviewed your
new patches.
Thanks a lot! I'll leave them sit for a couple more days to see if
anyone spots anything and then just push them if not.
Cheers,
Daniel
_______________________________________________
wayland-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Simon McVittie
2018-01-10 17:47:17 UTC
Permalink
I suspect 100% of the software I work on on a daily basis will explode in
completely unpredictable and undiagnosable ways in response to a malloc()
failure anyway
Does anyone test these code paths in Wayland? If so, how? (Genuine
questions, I'm interested in the answers.)

I ask because the original authors of libdbus wrote it thinking that
they had handled OOM conditions, at significant complexity cost,
then later added infrastructure to simulate malloc() failures during
automated testing and discovered that a significant fraction of them
were mishandled (Havoc estimates "at least 5%" in [1]). Next month that
test infrastructure will be 15 years old, and I'm *still* semi-regularly
finding bugs in pre-existing code where malloc() failures are mishandled.

If those code paths are never tested (and therefore probably don't work),
you might find that you get more reliable software by declining to handle
malloc() failures (usually done with an "xmalloc()" wrapper that either
returns non-NULL or aborts), and as a consequence not exposing users to
bugs in code that only exists to be able to cope with malloc() failures.
Handling OOM is great if you can consistently write completely bug-free
code, but there is considerable anecdotal evidence that we can't.

libdbus still claims to cope with malloc() failures, because it has
become policy (and because system components that can't crash, like
Upstart, systemd and dbus-daemon, were meant to be able to rely on it);
but even for libdbus, and even with automated testing to catch some of
the failures, I'm far from convinced that it was a net positive to do so.

[1] https://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/

Regards,
smcv
Daniel Stone
2018-01-11 15:44:19 UTC
Permalink
Hi Simon,
Post by Simon McVittie
I suspect 100% of the software I work on on a daily basis will explode in
completely unpredictable and undiagnosable ways in response to a malloc()
failure anyway
Does anyone test these code paths in Wayland? If so, how? (Genuine
questions, I'm interested in the answers.)
It might as well have been rhetorical though, since the answer is: no,
not at all. Especially not with overcommit.
Post by Simon McVittie
I ask because the original authors of libdbus wrote it thinking that
they had handled OOM conditions, at significant complexity cost,
then later added infrastructure to simulate malloc() failures during
automated testing and discovered that a significant fraction of them
were mishandled (Havoc estimates "at least 5%" in [1]). Next month that
test infrastructure will be 15 years old, and I'm *still* semi-regularly
finding bugs in pre-existing code where malloc() failures are mishandled.
I would agree with this. I've got a lot of trouble imagining the exact
scenario where malloc fails for our new zombie object during
destruction and then succeeds for the wl_closure allocation when we
next demarshal a message. I'd be inclined to keep this patch as-is.

Cheers,
Daniel
Daniel Stone
2018-01-18 11:29:20 UTC
Permalink
Hi all,
Post by Daniel Stone
Post by Simon McVittie
I ask because the original authors of libdbus wrote it thinking that
they had handled OOM conditions, at significant complexity cost,
then later added infrastructure to simulate malloc() failures during
automated testing and discovered that a significant fraction of them
were mishandled (Havoc estimates "at least 5%" in [1]). Next month that
test infrastructure will be 15 years old, and I'm *still* semi-regularly
finding bugs in pre-existing code where malloc() failures are mishandled.
I would agree with this. I've got a lot of trouble imagining the exact
scenario where malloc fails for our new zombie object during
destruction and then succeeds for the wl_closure allocation when we
next demarshal a message. I'd be inclined to keep this patch as-is.
Pekka somewhat agreed on IRC, and ultimately I went ahead and just
landed things as-is last night. Thanks all!

Cheers,
Daniel

Daniel Stone
2017-12-28 19:53:51 UTC
Permalink
Commit e273c7cde added a refcount to wl_proxy. The refcount is set to 1
on creation, decreased when the client explicitly destroys the proxy,
and is increased and decreased every time an event referencing that
proxy is queued.

Assuming no bugs, this means the refcount cannot reach 0 without the
proxy being explicitly destroyed. However, some (not all) of the
proxy-unref paths were only destroying the proxy if it had already been
deleted. This should already be enforced by refcounting, so remove the
check and rely solely on the refcount as the arbiter of when to free a
proxy.

Signed-off-by: Daniel Stone <***@collabora.com>
Cc: Jonas Ådahl <***@gmail.com>
Cc: Derek Foreman <***@osg.samsung.com>
---
src/wayland-client.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 21ef5b04..55838cd9 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -258,7 +258,6 @@ wl_event_queue_release(struct wl_event_queue *queue)
{
struct wl_closure *closure;
struct wl_proxy *proxy;
- bool proxy_destroyed;

while (!wl_list_empty(&queue->event_list)) {
closure = container_of(queue->event_list.next,
@@ -268,10 +267,8 @@ wl_event_queue_release(struct wl_event_queue *queue)
decrease_closure_args_refcount(closure);

proxy = closure->proxy;
- proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
-
proxy->refcount--;
- if (proxy_destroyed && !proxy->refcount)
+ if (!proxy->refcount)
free(proxy);

wl_closure_destroy(closure);
@@ -1310,10 +1307,10 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);

proxy->refcount--;
- if (proxy_destroyed) {
- if (!proxy->refcount)
- free(proxy);
+ if (!proxy->refcount)
+ free(proxy);

+ if (proxy_destroyed) {
wl_closure_destroy(closure);
return;
}
--
2.14.3
Derek Foreman
2018-01-03 21:44:57 UTC
Permalink
Post by Daniel Stone
Commit e273c7cde added a refcount to wl_proxy. The refcount is set to 1
on creation, decreased when the client explicitly destroys the proxy,
and is increased and decreased every time an event referencing that
proxy is queued.
Assuming no bugs, this means the refcount cannot reach 0 without the
proxy being explicitly destroyed. However, some (not all) of the
proxy-unref paths were only destroying the proxy if it had already been
deleted. This should already be enforced by refcounting, so remove the
check and rely solely on the refcount as the arbiter of when to free a
proxy.
---
src/wayland-client.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 21ef5b04..55838cd9 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -258,7 +258,6 @@ wl_event_queue_release(struct wl_event_queue *queue)
{
struct wl_closure *closure;
struct wl_proxy *proxy;
- bool proxy_destroyed;
while (!wl_list_empty(&queue->event_list)) {
closure = container_of(queue->event_list.next,
@@ -268,10 +267,8 @@ wl_event_queue_release(struct wl_event_queue *queue)
decrease_closure_args_refcount(closure);
proxy = closure->proxy;
- proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
-
proxy->refcount--;
- if (proxy_destroyed && !proxy->refcount)
+ if (!proxy->refcount)
free(proxy);
wl_closure_destroy(closure);
@@ -1310,10 +1307,10 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
proxy->refcount--;
- if (proxy_destroyed) {
- if (!proxy->refcount)
- free(proxy);
+ if (!proxy->refcount)
+ free(proxy);
This descends into use after free in the case the patch is concerned with?

I see you've turned that into an assert in the follow up patch though,
which looks like a solid idea to me.

This and the follow up are
Post by Daniel Stone
+ if (proxy_destroyed) {
wl_closure_destroy(closure);
return;
}
Daniel Stone
2017-12-28 19:53:52 UTC
Permalink
Rather than open-coded decrement-and-maybe-free, introduce a
wl_proxy_unref helper to do this for us. This will come in useful for
future patches, where we may also have to free a zombie object.

Signed-off-by: Daniel Stone <***@collabora.com>
Cc: Jonas Ådahl <***@gmail.com>
Cc: Derek Foreman <***@osg.samsung.com>
---
src/wayland-client.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 55838cd9..f3d71b0e 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -222,6 +222,19 @@ wl_event_queue_init(struct wl_event_queue *queue, struct wl_display *display)
queue->display = display;
}

+static void
+wl_proxy_unref(struct wl_proxy *proxy)
+{
+ assert(proxy->refcount > 0);
+ if (--proxy->refcount > 0)
+ return;
+
+ /* If we get here, the client must have explicitly requested
+ * deletion. */
+ assert(proxy->flags & WL_PROXY_FLAG_DESTROYED);
+ free(proxy);
+}
+
static void
decrease_closure_args_refcount(struct wl_closure *closure)
{
@@ -241,10 +254,7 @@ decrease_closure_args_refcount(struct wl_closure *closure)
if (proxy) {
if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
closure->args[i].o = NULL;
-
- proxy->refcount--;
- if (!proxy->refcount)
- free(proxy);
+ wl_proxy_unref(proxy);
}
break;
default:
@@ -257,20 +267,13 @@ static void
wl_event_queue_release(struct wl_event_queue *queue)
{
struct wl_closure *closure;
- struct wl_proxy *proxy;

while (!wl_list_empty(&queue->event_list)) {
closure = container_of(queue->event_list.next,
struct wl_closure, link);
wl_list_remove(&closure->link);
-
decrease_closure_args_refcount(closure);
-
- proxy = closure->proxy;
- proxy->refcount--;
- if (!proxy->refcount)
- free(proxy);
-
+ wl_proxy_unref(closure->proxy);
wl_closure_destroy(closure);
}
}
@@ -416,9 +419,7 @@ proxy_destroy(struct wl_proxy *proxy)

proxy->flags |= WL_PROXY_FLAG_DESTROYED;

- proxy->refcount--;
- if (!proxy->refcount)
- free(proxy);
+ wl_proxy_unref(proxy);
}

/** Destroy a proxy object
@@ -1305,11 +1306,7 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
decrease_closure_args_refcount(closure);
proxy = closure->proxy;
proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
-
- proxy->refcount--;
- if (!proxy->refcount)
- free(proxy);
-
+ wl_proxy_unref(proxy);
if (proxy_destroyed) {
wl_closure_destroy(closure);
return;
--
2.14.3
Daniel Stone
2017-12-28 19:53:56 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Until recently, if a client destroying a resource raced with the
server generating an event on that resource that delivered a file
descriptor, we would leak the fd.

This tests for a leaked fd from that race condition.

Reviewed-by: Daniel Stone <***@collabora.com>
Signed-off-by: Derek Foreman <***@osg.samsung.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
Makefile.am | 7 +++-
protocol/tests.xml | 43 ++++++++++++++++++++++
tests/display-test.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 151 insertions(+), 1 deletion(-)
create mode 100644 protocol/tests.xml

diff --git a/Makefile.am b/Makefile.am
index 0eedb108..bceca2a8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -114,7 +114,8 @@ protocol/%-client-protocol-core.h : $(top_srcdir)/protocol/%.xml
BUILT_SOURCES = \
$(nodist_libwayland_server_la_SOURCES) \
$(nodist_libwayland_client_la_SOURCES) \
- $(nodist_headers_test_SOURCES)
+ $(nodist_headers_test_SOURCES) \
+ $(nodist_display_test_SOURCES)

CLEANFILES = $(BUILT_SOURCES) doc/doxygen/doxygen_sqlite3.db
DISTCLEANFILES = src/wayland-version.h
@@ -206,6 +207,10 @@ client_test_SOURCES = tests/client-test.c
client_test_LDADD = libtest-runner.la
display_test_SOURCES = tests/display-test.c
display_test_LDADD = libtest-runner.la
+nodist_display_test_SOURCES = \
+ protocol/tests-server-protocol.h \
+ protocol/tests-client-protocol.h \
+ protocol/tests-protocol.c
connection_test_SOURCES = tests/connection-test.c
connection_test_LDADD = libtest-runner.la
event_loop_test_SOURCES = tests/event-loop-test.c
diff --git a/protocol/tests.xml b/protocol/tests.xml
new file mode 100644
index 00000000..77f6e243
--- /dev/null
+++ b/protocol/tests.xml
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<protocol name="build_time_wayland_tests">
+
+ <copyright>
+ Copyright © 2017 Samsung Electronics Co., Ltd
+
+ Permission is hereby granted, free of charge, to any person
+ obtaining a copy of this software and associated documentation files
+ (the "Software"), to deal in the Software without restriction,
+ including without limitation the rights to use, copy, modify, merge,
+ publish, distribute, sublicense, and/or sell copies of the Software,
+ and to permit persons to whom the Software is furnished to do so,
+ subject to the following conditions:
+
+ The above copyright notice and this permission notice (including the
+ next paragraph) shall be included in all copies or substantial
+ portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ SOFTWARE.
+ </copyright>
+
+ <interface name="fd_passer" version="1">
+ <description summary="Sends an event with an fd">
+ A trivial interface for fd passing tests.
+ </description>
+
+ <request name="destroy" type="destructor"/>
+
+ <event name="pre_fd"/>
+
+ <event name="fd">
+ <description summary="passes a file descriptor"/>
+ <arg name="fd" type="fd" summary="file descriptor"/>
+ </event>
+ </interface>
+</protocol>
diff --git a/tests/display-test.c b/tests/display-test.c
index e6f03693..06231585 100644
--- a/tests/display-test.c
+++ b/tests/display-test.c
@@ -47,6 +47,9 @@
#include "test-runner.h"
#include "test-compositor.h"

+#include "tests-server-protocol.h"
+#include "tests-client-protocol.h"
+
struct display_destroy_listener {
struct wl_listener listener;
int done;
@@ -1066,3 +1069,102 @@ TEST(bind_fails_on_filtered_global)

display_destroy(d);
}
+
+static void
+pre_fd(void *data, struct fd_passer *fdp)
+{
+ fd_passer_destroy(fdp);
+}
+
+static void
+fd(void *data, struct fd_passer *fdp, int32_t fd)
+{
+ /* We destroyed the resource before this event */
+ assert(false);
+}
+
+struct fd_passer_listener fd_passer_listener = {
+ pre_fd,
+ fd,
+};
+
+static void
+zombie_fd_handle_globals(void *data, struct wl_registry *registry,
+ uint32_t id, const char *intf, uint32_t ver)
+{
+ struct fd_passer *fdp;
+
+ if (!strcmp(intf, "fd_passer")) {
+ fdp = wl_registry_bind(registry, id, &fd_passer_interface, 1);
+ fd_passer_add_listener(fdp, &fd_passer_listener, NULL);
+ }
+}
+
+static const struct wl_registry_listener zombie_fd_registry_listener = {
+ zombie_fd_handle_globals,
+ NULL
+};
+
+static void
+zombie_client(void *data)
+{
+ struct client *c = client_connect();
+ struct wl_registry *registry;
+
+ registry = wl_display_get_registry(c->wl_display);
+ wl_registry_add_listener(registry, &zombie_fd_registry_listener, NULL);
+
+ /* Gets the registry */
+ wl_display_roundtrip(c->wl_display);
+
+ /* push out the fd_passer bind */
+ wl_display_roundtrip(c->wl_display);
+
+ /* push out our fd_passer.destroy */
+ wl_display_roundtrip(c->wl_display);
+
+ wl_registry_destroy(registry);
+
+ client_disconnect_nocheck(c);
+}
+
+static void
+fd_passer_clobber(struct wl_client *client, struct wl_resource *res)
+{
+ wl_resource_destroy(res);
+}
+
+static const struct fd_passer_interface fdp_interface = {
+ fd_passer_clobber,
+};
+
+static void
+bind_fd_passer(struct wl_client *client, void *data,
+ uint32_t vers, uint32_t id)
+{
+ struct wl_resource *res;
+
+ res = wl_resource_create(client, &fd_passer_interface, vers, id);
+ wl_resource_set_implementation(res, &fdp_interface, NULL, NULL);
+ assert(res);
+ fd_passer_send_pre_fd(res);
+ fd_passer_send_fd(res, fileno(stdin));
+}
+
+TEST(zombie_fd)
+{
+ struct display *d;
+ struct wl_global *g;
+
+ d = display_create();
+
+ g = wl_global_create(d->wl_display, &fd_passer_interface,
+ 1, d, bind_fd_passer);
+
+ client_create_noarg(d, zombie_client);
+ display_run(d);
+
+ wl_global_destroy(g);
+
+ display_destroy(d);
+}
--
2.14.3
Daniel Stone
2017-12-28 19:53:57 UTC
Permalink
From: Derek Foreman <***@osg.samsung.com>

Until recently, if an event attempting to deliver an fd to a zombie
object was demarshalled after the object was made into a zombie, we
leaked the fd and left it in the buffer.

If another event attempting to deliver an fd to a live object was in that
same buffer, the zombie's fd would be delivered instead.

This test recreates that situation.

While this is a ridiculously contrived way to force this race - delivering
an event from a destruction handler - I do have reports of this race
being hit in real world code.

Signed-off-by: Derek Foreman <***@osg.samsung.com>
Acked-by: Daniel Stone <***@collabora.com>
Signed-off-by: Daniel Stone <***@collabora.com>
---
protocol/tests.xml | 11 +++-
tests/display-test.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 160 insertions(+), 4 deletions(-)

diff --git a/protocol/tests.xml b/protocol/tests.xml
index 77f6e243..ea56ae49 100644
--- a/protocol/tests.xml
+++ b/protocol/tests.xml
@@ -26,7 +26,7 @@
SOFTWARE.
</copyright>

- <interface name="fd_passer" version="1">
+ <interface name="fd_passer" version="2">
<description summary="Sends an event with an fd">
A trivial interface for fd passing tests.
</description>
@@ -39,5 +39,14 @@
<description summary="passes a file descriptor"/>
<arg name="fd" type="fd" summary="file descriptor"/>
</event>
+
+ <!-- Version 2 additions -->
+ <request name="conjoin" since="2">
+ <description summary="register another fd passer with this one">
+ Tells this fd passer object about another one to send events
+ to for more complicated fd leak tests.
+ </description>
+ <arg name="passer" type="object" interface="fd_passer"/>
+ </request>
</interface>
</protocol>
diff --git a/tests/display-test.c b/tests/display-test.c
index 06231585..9b49a0ec 100644
--- a/tests/display-test.c
+++ b/tests/display-test.c
@@ -1128,27 +1128,83 @@ zombie_client(void *data)
client_disconnect_nocheck(c);
}

+struct passer_data {
+ struct wl_resource *conjoined_passer;
+};
+
+static void
+feed_pipe(int fd, char tosend)
+{
+ int count;
+
+ do {
+ count = write(fd, &tosend, 1);
+ } while (count != 1 && errno == EAGAIN);
+ assert(count == 1);
+ close(fd);
+}
+
static void
fd_passer_clobber(struct wl_client *client, struct wl_resource *res)
{
+ struct passer_data *pdata = wl_resource_get_user_data(res);
+ int pipes1[2], pipes2[2], ret;
+
+ if (pdata->conjoined_passer) {
+ ret = pipe(pipes1);
+ assert(ret == 0);
+ ret = pipe(pipes2);
+ assert(ret == 0);
+
+ wl_resource_queue_event(res, FD_PASSER_FD, pipes1[0]);
+ fd_passer_send_fd(pdata->conjoined_passer, pipes2[0]);
+ feed_pipe(pipes1[1], '1');
+ feed_pipe(pipes2[1], '2');
+ close(pipes1[0]);
+ close(pipes2[0]);
+ }
wl_resource_destroy(res);
}

+static void
+fd_passer_twin(struct wl_client *client, struct wl_resource *res, struct wl_resource *passer)
+{
+ struct passer_data *pdata = wl_resource_get_user_data(res);
+
+ pdata->conjoined_passer = passer;
+}
+
static const struct fd_passer_interface fdp_interface = {
fd_passer_clobber,
+ fd_passer_twin
};

+static void
+pdata_destroy(struct wl_resource *res)
+{
+ struct passer_data *pdata = wl_resource_get_user_data(res);
+
+ free(pdata);
+}
+
static void
bind_fd_passer(struct wl_client *client, void *data,
uint32_t vers, uint32_t id)
{
struct wl_resource *res;
+ struct passer_data *pdata;
+
+ pdata = malloc(sizeof(*pdata));
+ assert(pdata);
+ pdata->conjoined_passer = NULL;

res = wl_resource_create(client, &fd_passer_interface, vers, id);
- wl_resource_set_implementation(res, &fdp_interface, NULL, NULL);
+ wl_resource_set_implementation(res, &fdp_interface, pdata, pdata_destroy);
assert(res);
- fd_passer_send_pre_fd(res);
- fd_passer_send_fd(res, fileno(stdin));
+ if (vers == 1) {
+ fd_passer_send_pre_fd(res);
+ fd_passer_send_fd(res, fileno(stdin));
+ }
}

TEST(zombie_fd)
@@ -1168,3 +1224,94 @@ TEST(zombie_fd)

display_destroy(d);
}
+
+
+static void
+double_pre_fd(void *data, struct fd_passer *fdp)
+{
+ assert(false);
+}
+
+static void
+double_fd(void *data, struct fd_passer *fdp, int32_t fd)
+{
+ char buf;
+ int count;
+
+ do {
+ count = read(fd, &buf, 1);
+ } while (count != 1 && errno == EAGAIN);
+ assert(count == 1);
+
+ close(fd);
+ fd_passer_destroy(fdp);
+ assert(buf == '2');
+}
+
+struct fd_passer_listener double_fd_passer_listener = {
+ double_pre_fd,
+ double_fd,
+};
+
+
+static void
+double_zombie_fd_handle_globals(void *data, struct wl_registry *registry,
+ uint32_t id, const char *intf, uint32_t ver)
+{
+ struct fd_passer *fdp1, *fdp2;
+
+ if (!strcmp(intf, "fd_passer")) {
+ fdp1 = wl_registry_bind(registry, id, &fd_passer_interface, 2);
+ fd_passer_add_listener(fdp1, &double_fd_passer_listener, NULL);
+ fdp2 = wl_registry_bind(registry, id, &fd_passer_interface, 2);
+ fd_passer_add_listener(fdp2, &double_fd_passer_listener, NULL);
+ fd_passer_conjoin(fdp1, fdp2);
+ fd_passer_destroy(fdp1);
+ }
+}
+
+static const struct wl_registry_listener double_zombie_fd_registry_listener = {
+ double_zombie_fd_handle_globals,
+ NULL
+};
+
+static void
+double_zombie_client(void *data)
+{
+ struct client *c = client_connect();
+ struct wl_registry *registry;
+
+ registry = wl_display_get_registry(c->wl_display);
+ wl_registry_add_listener(registry, &double_zombie_fd_registry_listener, NULL);
+
+ /* Gets the registry */
+ wl_display_roundtrip(c->wl_display);
+
+ /* One more so server can respond to conjoin+destroy */
+ wl_display_roundtrip(c->wl_display);
+
+ /* And finally push out our last fd_passer.destroy */
+ wl_display_roundtrip(c->wl_display);
+
+ wl_registry_destroy(registry);
+
+ client_disconnect_nocheck(c);
+}
+
+TEST(zombie_fd_errant_consumption)
+{
+ struct display *d;
+ struct wl_global *g;
+
+ d = display_create();
+
+ g = wl_global_create(d->wl_display, &fd_passer_interface,
+ 2, d, bind_fd_passer);
+
+ client_create_noarg(d, double_zombie_client);
+ display_run(d);
+
+ wl_global_destroy(g);
+
+ display_destroy(d);
+}
--
2.14.3
Loading...