[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