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 <der...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
