On Wed, Jul 18, 2012 at 02:12:23PM +0000, Fiedler, Mathias wrote: > 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.
It looks like just the right fix, thanks. Kristian > Best Regards, > Mathias > > > > ________________________________________ > Von: Kristian Høgsberg [hoegsb...@gmail.com] > Gesendet: Dienstag, 17. Juli 2012 22:49 > An: Fiedler, Mathias > Cc: wayland-devel@lists.freedesktop.org > 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 <mathias.fied...@xse.de> > 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 <mathias.fied...@xse.de> > 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 <mathias.fied...@xse.de> > 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 wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel