[PATCH wayland 8/9] client: Replace the singleton zombie with bespoke zombies
Jonas Ådahl
jadahl at gmail.com
Wed Apr 12 03:53:02 UTC 2017
On Tue, Apr 11, 2017 at 10:37:28AM -0500, Derek Foreman wrote:
> 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. :)
We could instead introduce a struct wl_zombie_object that constains the
data needed for doing a proper cleanup. This data could be for example
be number of fds per event opcode, or strdup:ed event signatures or
something. All of this would be known at zombification, and we'd avoid
any ABI change as this patch, as you say, seems to introduce.
If we'd care about minimizing allocations, we could also insert the old
WL_ZOMBIE_OBJECT when there are no fds to clean up, and handle that as a
another special case on clean up.
Jonas
>
> 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)
> > {
> >
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list