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

[Alon Levy]

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