<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Tue, Apr 11, 2017 at 11:39 AM Derek Foreman <<a href="mailto:derekf@osg.samsung.com">derekf@osg.samsung.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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) </blockquote><div> </div><div>ITYM 1024 but I understood what you meant</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <<a href="mailto:derekf@osg.samsung.com" class="gmail_msg" target="_blank">derekf@osg.samsung.com</a>> > --- > 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-><a href="http://object.id" rel="noreferrer" class="gmail_msg" target="_blank">object.id</a>); > else if (proxy-><a href="http://object.id" rel="noreferrer" class="gmail_msg" target="_blank">object.id</a> < WL_SERVER_ID_START) > - wl_map_insert_at(&proxy->display->objects, 0, > - proxy-><a href="http://object.id" rel="noreferrer" class="gmail_msg" target="_blank">object.id</a>, WL_ZOMBIE_OBJECT); > + wl_map_insert_at(&proxy->display->objects, > + WL_MAP_ENTRY_ZOMBIE, > + proxy-><a href="http://object.id" rel="noreferrer" class="gmail_msg" target="_blank">object.id</a>, > + (void *)proxy->object.interface); > else > wl_map_insert_at(&proxy->display->objects, 0, > proxy-><a href="http://object.id" rel="noreferrer" class="gmail_msg" target="_blank">object.id</a>, 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 <a href="mailto:wayland-devel@lists.freedesktop.org" class="gmail_msg" target="_blank">wayland-devel@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" class="gmail_msg" target="_blank">https://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div></div>