[PATCH wayland 8/9] client: Replace the singleton zombie with bespoke zombies
Derek Foreman
derekf at osg.samsung.com
Tue Apr 11 15:37:28 UTC 2017
On 07/04/17 03:27 PM, Derek Foreman wrote:
> 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.
>
> Instead of having a singleton zombie object, we add a new wl_map flag
> (valid only on the client side where zombies roam - the existing
> "legacy" flag was only ever used on the server side) to indicate that a
> map entry is now a zombie.
>
> We still have to refcount and potentially free the proxy immediately or
> we're left with situations where it can be leaked forever. If we clean
> up on delete_id, for example, a forced disconnect will result in a leaked
> proxy (breaking much of the test suite).
>
> So, since we must free the zombied proxy immediately, we store just the
> interface for it in its previous map location, so signature information
> can be retained for zombie-destined events.
So even with this in place it's still possible for a malicious
application to consume 1000fds (the number of fds that fit in fds_in)
and go to sleep and hold them - on client disconnect they should be
freed. I don't really see a way to prevent that sort of crap anyway - a
client could use sendmsg directly, send a single byte of regular data
(ie: not enough to trigger demarshalling, the server will assume there's
more to come), and a buffer's worth of file descriptors, then never
speak again.
This appears indistinguishable from reasonable behaviour?
There's also an interesting side effect to this patch that I noticed
yesterday - it places a requirement on the client to keep the
wl_interfaces around in memory long after it destroys the proxy - up
until the delete_id occurs. We have no way to hook delete_id in the
client. This turned into a crash in EFL clients using the text input
protocol, as the input method code in EFL is a module that's unloaded on
shutdown. It was easily fixed in EFL, but I'd like some input - is a
client allowed to unmap the memory that a wl_interface is stored in at
the moment it deletes the relevant proxy?
This isn't terribly difficult to fix, but I won't bother if the
behaviour is universally concluded to be a client bug. :)
Oh, and I just noticed wl_map_set_flags() is in this patch - that's a
remnant of a previous attempt to close this leak, and will be removed
for v2.
Thanks,
Derek
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> src/connection.c | 20 +++++++++++++++++++-
> src/wayland-client.c | 10 ++++++----
> src/wayland-private.h | 12 ++++++++----
> src/wayland-util.c | 22 ++++++++++++++++++++--
> 4 files changed, 53 insertions(+), 11 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index f2ebe06..84f5d79 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -809,6 +809,24 @@ wl_connection_demarshal(struct wl_connection *connection,
> return NULL;
> }
>
> +bool
> +wl_object_is_zombie(struct wl_map *map, uint32_t id)
> +{
> + uint32_t flags;
> +
> + if (map->side == WL_MAP_SERVER_SIDE)
> + return false;
> +
> + if (id >= WL_SERVER_ID_START)
> + return false;
> +
> + flags = wl_map_lookup_flags(map, id);
> + if (flags & WL_MAP_ENTRY_ZOMBIE)
> + return true;
> +
> + return false;
> +}
> +
> int
> wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)
> {
> @@ -830,7 +848,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 7243d3d..2cd713d 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -408,8 +408,10 @@ 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)
> - wl_map_insert_at(&proxy->display->objects, 0,
> - proxy->object.id, WL_ZOMBIE_OBJECT);
> + wl_map_insert_at(&proxy->display->objects,
> + WL_MAP_ENTRY_ZOMBIE,
> + proxy->object.id,
> + (void *)proxy->object.interface);
> else
> wl_map_insert_at(&proxy->display->objects, 0,
> proxy->object.id, NULL);
> @@ -837,7 +839,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);
> @@ -1228,7 +1230,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 d906a6f..2acd2d4 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);
> @@ -69,7 +66,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 {
> @@ -107,6 +105,9 @@ uint32_t
> wl_map_lookup_flags(struct wl_map *map, uint32_t i);
>
> void
> +wl_map_set_flags(struct wl_map *map, uint32_t i, uint32_t flags);
> +
> +void
> wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data);
>
> struct wl_connection *
> @@ -189,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);
>
> diff --git a/src/wayland-util.c b/src/wayland-util.c
> index cab7fc5..71abb2e 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)
> {
> @@ -360,6 +358,26 @@ wl_map_lookup_flags(struct wl_map *map, uint32_t i)
> return 0;
> }
>
> +void
> +wl_map_set_flags(struct wl_map *map, uint32_t i, uint32_t flags)
> +{
> + union map_entry *start;
> + uint32_t count;
> + struct wl_array *entries;
> +
> + if (i < WL_SERVER_ID_START) {
> + entries = &map->client_entries;
> + } else {
> + entries = &map->server_entries;
> + i -= WL_SERVER_ID_START;
> + }
> +
> + start = entries->data;
> + count = entries->size / sizeof *start;
> + if (i < count && !map_entry_is_free(start[i]))
> + start[i].next |= (flags & 0x1) << 1;
> +}
> +
> static enum wl_iterator_result
> for_each_helper(struct wl_array *entries, wl_iterator_func_t func, void *data)
> {
>
More information about the wayland-devel
mailing list