Patches for resource id allocation
Kristian Høgsberg
hoegsberg at gmail.com
Sun Jul 22 11:10:41 PDT 2012
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 [hoegsberg at gmail.com]
> Gesendet: Dienstag, 17. Juli 2012 22:49
> An: Fiedler, Mathias
> Cc: wayland-devel at 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.fiedler at 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.fiedler at 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.fiedler at 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
>
More information about the wayland-devel
mailing list