[PATCH wayland 8/9] client: Replace the singleton zombie with bespoke zombies
Mike Blumenkrantz
michael.blumenkrantz at gmail.com
Tue Apr 11 16:05:46 UTC 2017
On Tue, Apr 11, 2017 at 11:39 AM Derek Foreman <derekf at osg.samsung.com>
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)
>
ITYM 1024 but I understood what you meant
> 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)
> > {
> >
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170411/ba6df285/attachment-0001.html>
More information about the wayland-devel
mailing list