Hi Kristian, here are some patches for - sending out an error on invalid new_ids - creating new id entry implicitly in wl_connection_demarshal
I'm not 100% sure if i understood the id management correctly, but i hope it fits. Best Regards, Mathias ________________________________________ Von: Kristian Høgsberg [[email protected]] Gesendet: Dienstag, 17. Juli 2012 22:49 An: Fiedler, Mathias Cc: [email protected] Betreff: Re: Resource id allocation on server side On Tue, Jul 17, 2012 at 11:52:02AM +0000, Fiedler, Mathias wrote: > Hi, > > I'm facing an issue when the client tries to create a new object > with an id bigger than the current maximum id on server-side +1. Is > it correct that those ids will be silently ignored? > E.g. wl_client_add_resource() would call wl_map_insert_at() return > -1 which isn't checked. > > For example, this situation might happen when a client calls > wl_seat.get_pointer() on a seat which has no pointer device > assigned. The result is that all the following object creations > will silently fail. The client won't realize its failure until he > tries to call a method on such object. Such error might be hard to > find. > > Is my analysis correct, or did i miss something? If not what would > be a correct fix for this? Yes, you are right, as long as the client doesn't realize that the pointer object didn't get allocated, and keep using new IDs (as opposed to reusing old freed up IDs), those new objects will get silently ignored. > Restricting the id allocation to the next higher id seems > reasonable, so the compositor get in trouble at corrupt messages > with a new id that are too big. But should this restriction in > wayland-server be relaxed? Or shall the compositor keep track of > current maximum id and allocate new ids even when the corresponding > server-side resource is not created? I want to keep the restriction in the server, so we need to fix these cases where an object may not be created. The display_sync handler is similar in that it destroys the object immeidately. That function used to not even allocate the object, just send the event back without ever inserting the object. Now it creates the objects, inserts it and then destroys it, because otherwise it causes the same problem you hit. First we need to not silently ignore this error, and then we need to insert and destroy and object with the id, even in cases where we don't create the object. A better option perhaps is to handle this centrally in deref_new_objects(), which already pre-processes the incoming request and touches all new IDs. Or even better, handle it in the 'n' case in wl_connection_demarshal(), since it should be handle for both server and client. Thanks for tracking this down, Kristian
From 6f72dd70b42030d422c9b367c6c00e978e898571 Mon Sep 17 00:00:00 2001 From: Mathias Fiedler <[email protected]> Date: Wed, 18 Jul 2012 15:51:45 +0200 Subject: [PATCH 1/3] wayland-server: send error on invalid new object id Creation of new client resources was silently ignored when wl_client_add_resource() was used on server side and new object id was out of range. An error is now send out to the client in such case. Also changed error message in wl_client_add_object, since wl_map_insert_at() returns -1 only at invalid new id. --- src/wayland-server.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/wayland-server.c b/src/wayland-server.c index df9bd07..3dc8416 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -395,9 +395,12 @@ wl_client_add_resource(struct wl_client *client, resource->object.id = wl_map_insert_new(&client->objects, WL_MAP_SERVER_SIDE, resource); - else - wl_map_insert_at(&client->objects, - resource->object.id, resource); + else if (wl_map_insert_at(&client->objects, + resource->object.id, resource) < 0) + wl_resource_post_error(client->display_resource, + WL_DISPLAY_ERROR_INVALID_OBJECT, + "invalid new id %d", + resource->object.id); resource->client = client; wl_signal_init(&resource->destroy_signal); @@ -1277,7 +1280,10 @@ wl_client_add_object(struct wl_client *client, wl_signal_init(&resource->destroy_signal); if (wl_map_insert_at(&client->objects, resource->object.id, resource) < 0) { - wl_resource_post_no_memory(client->display_resource); + wl_resource_post_error(client->display_resource, + WL_DISPLAY_ERROR_INVALID_OBJECT, + "invalid new id %d", + resource->object.id); free(resource); return NULL; } -- 1.7.9.5
From 0934b3eb0b2a95f77653ea8eaab65e25469cf14b Mon Sep 17 00:00:00 2001 From: Mathias Fiedler <[email protected]> Date: Wed, 18 Jul 2012 15:52:51 +0200 Subject: [PATCH 2/3] wayland-util: add method for reserving new object id wl_map_reserve_new() ensures that new id is valid and will point to an empty array entry. --- src/wayland-private.h | 1 + src/wayland-util.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/wayland-private.h b/src/wayland-private.h index ea70258..2113d83 100644 --- a/src/wayland-private.h +++ b/src/wayland-private.h @@ -46,6 +46,7 @@ void wl_map_init(struct wl_map *map); void wl_map_release(struct wl_map *map); uint32_t wl_map_insert_new(struct wl_map *map, uint32_t side, void *data); int wl_map_insert_at(struct wl_map *map, uint32_t i, void *data); +int wl_map_reserve_new(struct wl_map *map, uint32_t i); void wl_map_remove(struct wl_map *map, uint32_t i); void *wl_map_lookup(struct wl_map *map, uint32_t i); void wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data); diff --git a/src/wayland-util.c b/src/wayland-util.c index 107b6db..eacf902 100644 --- a/src/wayland-util.c +++ b/src/wayland-util.c @@ -214,6 +214,39 @@ wl_map_insert_at(struct wl_map *map, uint32_t i, void *data) return 0; } +WL_EXPORT int +wl_map_reserve_new(struct wl_map *map, uint32_t i) +{ + 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; + } + + count = entries->size / sizeof *start; + + if (count < i) + return -1; + + if (count == i) { + wl_array_add(entries, sizeof *start); + start = entries->data; + start[i].data = NULL; + } else { + start = entries->data; + if (start[i].data != NULL) { + return -1; + } + } + + return 0; +} + WL_EXPORT void wl_map_remove(struct wl_map *map, uint32_t i) { -- 1.7.9.5
From f9ccadc5c699e2e97a3f4f12f9c33558aecf3c2e Mon Sep 17 00:00:00 2001 From: Mathias Fiedler <[email protected]> Date: Wed, 18 Jul 2012 15:53:23 +0200 Subject: [PATCH 3/3] connection: reserve id on incoming new object If a new object id arrives ensure that there is an empty array entry created, otherwise we might get out of sync for new ids if object isn't created by interface implementation. --- src/connection.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/connection.c b/src/connection.c index 5946556..f4f881e 100644 --- a/src/connection.c +++ b/src/connection.c @@ -756,14 +756,14 @@ wl_connection_demarshal(struct wl_connection *connection, closure->args[i] = id; *id = p; - object = wl_map_lookup(objects, *p); - if (object != NULL) { - printf("not a new object (%d), " + if (wl_map_reserve_new(objects, *p) < 0) { + printf("not a valid new object id (%d), " "message %s(%s)\n", *p, message->name, message->signature); errno = EINVAL; goto err; } + p++; break; case 'a': -- 1.7.9.5
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
