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


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