[Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

[Yonit Halperin]

On 05/15/2012 12:06 PM, Alon Levy wrote:
> On Tue, May 15, 2012 at 11:57:05AM +0300, Yonit Halperin wrote:
>> Hi,
>>
>> Instead of this patch series and the previous "self_bitmap" patch, I think
>> we should do the following:
>> Both GLZDrawable and Drawable share references to RedDrawable and
>> self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
>> So we need to have a new type which warps RedDrawable and self_bitmap, and
>> move the ref count from RedDrawable to this new type.
>> Then, Drawable and GlzDrawable will just hold a reference to
>> RedDrawableWrapper, instead of two references to RedDrawable and
>> self_bitmap, and they will just decrease the reference to RedDrawableWrapper
>> when they are released.
>
> Why can't we have GlzDrawable reference Drawable?
>
Currently Drawables are allocated on the stack. Making GLZDrawable and 
Drawable life time dependent, will lead to more calls for 
free_one_drawable, and will limit the glz dictionary as well. In 
addition, the current code assumes GLZDrawable and Drawable are 
independent, while RedDrawable life time is dependent on both of them. 
Making the change you suggest will require more risky refactoring to the 
worker.
Maybe we should move Drawables to the heap, as we already discussed,
and limit the drawables allocation not by number, but by the size their 
corresponding qxl drawables occupy in the device RAM. However, I think 
this should be done gradually, after fixing the "self_bitmap" double 
release bug.

>>
>> Thanks,
>> Yonit.
>> On 05/14/2012 03:32 PM, Alon Levy wrote:
>>> After the previous patch moving self_bitmap freeing inside red_drawable
>>> ref count, we have a possible self_bitmap leak:
>>>
>>> red_process_commands
>>>   red_get_drawable | red_drawable #1, red_drawable->self_bitmap == 1
>>>    red_process_drawable | rd #2, d #1, d->self_bitmap != NULL
>>>     release_drawable | rd #1, d# = 0, try to release self_bitmap, but
>>>                          blocked by rd #1
>>>    put_red_drawable | rd #0
>>>
>>> This patch moves the call to release_drawable after the call to
>>> put_red_drawable, thereby fixing the above situation.
>>> ---
>>>   server/red_worker.c |   12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index e1c86fa..1adf4c9 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -3867,8 +3867,8 @@ static inline void red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
>>>       }
>>>   }
>>>
>>> -static inline void red_process_drawable(RedWorker *worker, RedDrawable *drawable,
>>> -                                        uint32_t group_id)
>>> +static inline Drawable *red_process_drawable(RedWorker *worker, RedDrawable *drawable,
>>> +                                             uint32_t group_id)
>>>   {
>>>       int surface_id;
>>>       Drawable *item = get_drawable(worker, drawable->effect, drawable, group_id);
>>> @@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *drawable
>>>   #endif
>>>       }
>>>   cleanup:
>>> -    release_drawable(worker, item);
>>> +    return item;
>>>   }
>>>
>>>   static inline void red_create_surface(RedWorker *worker, uint32_t surface_id,uint32_t width,
>>> @@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
>>>           switch (ext_cmd.cmd.type) {
>>>           case QXL_CMD_DRAW: {
>>>               RedDrawable *red_drawable = red_drawable_new(); // returns with 1 ref
>>> +            Drawable *drawable;
>>>
>>>               if (red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
>>>                                    red_drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
>>>                   break;
>>>               }
>>> -            red_process_drawable(worker, red_drawable, ext_cmd.group_id);
>>> -            // release the red_drawable
>>> +            drawable = red_process_drawable(worker, red_drawable, ext_cmd.group_id);
>>> +            // release red_drawable first, it will not die because drawable holds a reference on it.
>>>               put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
>>> +            release_drawable(worker, drawable);
>>>               break;
>>>           }
>>>           case QXL_CMD_UPDATE: {
>>