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

[Yonit Halperin]

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.

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: {