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

Submitted by Alon Levy on May 14, 2012, 12:32 p.m.

Details

Message ID 1336998749-11201-3-git-send-email-alevy@redhat.com
State Accepted
Commit 35dbf3ccc4b852f9dbb29eb8a53c94f26d2e3a6e
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy May 14, 2012, 12:32 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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

> 
> 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: {
>
On 05/15/2012 11:57 AM, 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.

Can we just add self_bitmap into RedDrawable ?
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: {
>>
On Tue, May 15, 2012 at 12:27:24PM +0300, Yonit Halperin wrote:
> 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.

OK, I'll do a new patch with self_bitmap moves to RedDrawable.

> 
> >>
> >>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: {
> >>
>
On Tue, May 15, 2012 at 12:18:44PM +0300, Uri Lublin wrote:
> On 05/15/2012 11:57 AM, 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.
> 
> Can we just add self_bitmap into RedDrawable ?

Just sent patches that do that.

> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel