[PATCH wayland 2/5] server: set map entry to NULL after resource is destroyed

[Pekka Paalanen]

On Fri, 13 May 2016 15:01:19 +0200
Marek Chalupa <mchqwerty at 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 <mchqwerty at 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);
>  	}

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160516/4d8aebb1/attachment-0001.sig>