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

2016-05-16 Thread Pekka Paalanen 
On Fri, 13 May 2016 15:01:19 +0200
Marek Chalupa  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 
> ---
>  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(>destroy_signal, resource);
>  
> - flags = wl_map_lookup_flags(>objects, resource->object.id);
> + flags = wl_map_lookup_flags(>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(>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(>objects, 0, id, NULL);
>   } else {
>   wl_map_remove(>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

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

2016-05-13 Thread Marek Chalupa 
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.

Signed-off-by: Marek Chalupa 
---
 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(>destroy_signal, resource);
 
-   flags = wl_map_lookup_flags(>objects, resource->object.id);
+   flags = wl_map_lookup_flags(>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(>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(>objects, 0, id, NULL);
} else {
wl_map_remove(>objects, id);
}
-- 
2.5.5

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel