On Fri, 13 May 2016 15:01:19 +0200 Marek Chalupa <mchqwe...@gmail.com> wrote:
> We did it only for client entries for some reason, so when > we used wl_client_get_object() for some server object that > has been destroyed, we got dangling pointer. > > NOTE: this is basically an API change, since it changes > the return value of wl_client_get_object() in some corner cases. > However, now we return NULL insted of a pointer to invalid memory, > which could be OK API break. Hi Marek, I'm not sure I see why there is something to fix here. Let's assume we gave a server-side created wl_resource, and wl_resource_destroy() is called on it: wl_resource_destroy(): - destroy_resource(): - - does nothing to the map, really - server-side created object so get to else-branch of (id < WL_SERVER_ID_START) - wl_map_remove(): - - The entry is completely removed from the map, so wl_map_lookup() on it will return NULL. - At this point the id longer exists and can be re-used, so no lookups should be made on it. Then if we look at wl_client_get_object(): - wl_map_lookup() Ok, so this will just return NULL now, but assuming the id has not been re-used. If it was re-used, it returns a valid pointer to a "wrong" object. Did I miss something? To me it seems there are two things that make the premise of this patch invalid: 1. You already get NULL from wl_client_lookup_object(). 2. The lookup would be invalid anyway as the id as you knew it does not exist anymore. The other call site for destroy_resource() is wl_client_destroy() which also means all ids are invalidated. Now, client entries are NULLed, because they *need* to stay in the map, because a) the free_list is only for the map->side of the two object arrays in a wl_map, and b) wl_map_insert_at() will only increase the array size by one at most. The latter is also limiting what ids clients can allocate at a time. Therefore, NAK for this patch because the premise why this patch was written is false. If you wanted to apply this only as a clean-up or clarification, I'm not sure it would be a win. wl_map_remove() would still be called only for server-allocated ids. Thanks, pq > Signed-off-by: Marek Chalupa <mchqwe...@gmail.com> > --- > src/wayland-server.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/wayland-server.c b/src/wayland-server.c > index f745e62..c93a426 100644 > --- a/src/wayland-server.c > +++ b/src/wayland-server.c > @@ -562,16 +562,20 @@ destroy_resource(void *element, void *data) > { > struct wl_resource *resource = element; > struct wl_client *client = resource->client; > + uint32_t id = resource->object.id; > uint32_t flags; > > wl_signal_emit(&resource->destroy_signal, resource); > > - flags = wl_map_lookup_flags(&client->objects, resource->object.id); > + flags = wl_map_lookup_flags(&client->objects, id); > if (resource->destroy) > resource->destroy(resource); > > if (!(flags & WL_MAP_ENTRY_LEGACY)) > free(resource); > + > + /* replace the object with NULL since it is destroyed */ > + wl_map_insert_at(&client->objects, 0, id, NULL); > } > > WL_EXPORT void > @@ -584,11 +588,9 @@ wl_resource_destroy(struct wl_resource *resource) > destroy_resource(resource, NULL); > > if (id < WL_SERVER_ID_START) { > - if (client->display_resource) { > + if (client->display_resource) > wl_resource_queue_event(client->display_resource, > WL_DISPLAY_DELETE_ID, id); > - } > - wl_map_insert_at(&client->objects, 0, id, NULL); > } else { > wl_map_remove(&client->objects, id); > }
pgpPWKYhJeHqD.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel