[PATCH] wayland-server: fix resource destroy

[Tiago Vignatti]

On 10/11/2011 09:23 PM, Kristian Høgsberg wrote:
> On Sun, Oct 9, 2011 at 2:32 PM, Tiago Vignatti
> <tiago.vignatti at linux.intel.com>  wrote:
>> On 10/07/2011 10:49 AM, Fiedler, Mathias wrote:
>>>
>>> object id was read from freed memory.
>>> In rare cases i got errors where compositor was complaining about wrong or
>>> still used object ids.
>>>
>>> diff --git a/wayland/src/wayland-server.c b/wayland/src/wayland-server.c
>>> index a6cd88d..5eb7906 100644
>>> --- a/wayland/src/wayland-server.c
>>> +++ b/wayland/src/wayland-server.c
>>> @@ -313,9 +313,9 @@ WL_EXPORT void
>>>   wl_resource_destroy(struct wl_resource *resource, uint32_t time)
>>>   {
>>>          struct wl_client *client = resource->client;
>>> -
>>> +       uint32_t id = resource->object.id;
>>>          destroy_resource(resource,&time);
>>> -       wl_map_insert_at(&client->objects, resource->object.id, NULL);
>>> +       wl_map_insert_at(&client->objects, id, NULL);
>>>   }
>>
>> object id has to be removed from the structure, using wl_map_remove. Also,
>> the way you cooked the patch is not that legible (one could easily go and
>> revert the changes for cleaning up) and I guess we can make better a bit
>
> No it has to be removed by inserting a NULL pointer.  Mathias' patch
> is right, but I edited the fix slightly, to just insert the NULL
> pointer before calling destroy_resource().
>
> The map data structure can be used in one of two modes: either you use
> it to allocate IDs in constant time by using wl_map_insert_new() and
> wl_map_remove().  wl_map_insert_new() will allocate and return a new
> ID, wl_map_remove() will put the removed entry on a free list so
> insert can find it in constant time.  This is how the clients allocate
> IDs and how the server allocates global object names.
>
> The other way is just wl_map_insert_at(), which will insert the given
> pointer and the specified index.  This is how you use it when somebody
> else allocates the IDs and you're just keeping track of the mapping.

right. A bit tricky the API, but thanks for explanation! Should we amend 
it on the code?

     Tiago