[Spice-commits] 4 commits - server/red_worker.c

[Alon Levy]

 server/red_worker.c |   38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

New commits:
commit 35dbf3ccc4b852f9dbb29eb8a53c94f26d2e3a6e
Author: Alon Levy <alevy at redhat.com>
Date:   Mon May 14 14:39:10 2012 +0300

    server/red_worker: fix possible leak of self_bitmap
    
    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.

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: {
commit bab771c10dfa1cc5751f4fd33548b997f7200dbd
Author: Alon Levy <alevy at redhat.com>
Date:   Mon May 14 14:36:33 2012 +0300

    server/red_worker/red_process_commands: rename drawable to red_drawable (later add a local drawable)

diff --git a/server/red_worker.c b/server/red_worker.c
index 4304f29..e1c86fa 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -4843,15 +4843,15 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
         worker->repoll_cmd_ring = 0;
         switch (ext_cmd.cmd.type) {
         case QXL_CMD_DRAW: {
-            RedDrawable *drawable = red_drawable_new(); // returns with 1 ref
+            RedDrawable *red_drawable = red_drawable_new(); // returns with 1 ref
 
             if (red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
-                                 drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
+                                 red_drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
                 break;
             }
-            red_process_drawable(worker, drawable, ext_cmd.group_id);
+            red_process_drawable(worker, red_drawable, ext_cmd.group_id);
             // release the red_drawable
-            put_red_drawable(worker, drawable, ext_cmd.group_id, NULL);
+            put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
             break;
         }
         case QXL_CMD_UPDATE: {
commit c8cefd4a8f28bde14b70a72272c0ccb80c77af04
Author: Alon Levy <alevy at redhat.com>
Date:   Mon May 14 14:32:23 2012 +0300

    server/red_worker: red_process_drawable: have single point of exit

diff --git a/server/red_worker.c b/server/red_worker.c
index 60f30d3..4304f29 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3905,23 +3905,19 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *drawable
     red_inc_surfaces_drawable_dependencies(worker, item);
 
     if (region_is_empty(&item->tree_item.base.rgn)) {
-        release_drawable(worker, item);
-        return;
+        goto cleanup;
     }
 
     if (!red_handle_self_bitmap(worker, item)) {
-        release_drawable(worker, item);
-        return;
+        goto cleanup;
     }
 
     if (!red_handle_depends_on_target_surface(worker, surface_id)) {
-        release_drawable(worker, item);
-        return;
+        goto cleanup;
     }
 
     if (!red_handle_surfaces_dependencies(worker, item)) {
-        release_drawable(worker, item);
-        return;
+        goto cleanup;
     }
 
     if (red_current_add_qxl(worker, &worker->surfaces[surface_id].current, item,
@@ -3934,6 +3930,7 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *drawable
         red_draw_qxl_drawable(worker, item);
 #endif
     }
+cleanup:
     release_drawable(worker, item);
 }
 
commit 6935bd3d604288a64f491e4226f3d31aafdbf81e
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Sun May 13 14:21:28 2012 +0300

    server/red_worker: don't release self_bitmap unless refcount is 0
    
    RHBZ: 808936

diff --git a/server/red_worker.c b/server/red_worker.c
index 473d0d6..60f30d3 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1695,13 +1695,12 @@ static inline void put_red_drawable(RedWorker *worker, RedDrawable *drawable, ui
 {
     QXLReleaseInfoExt release_info_ext;
 
-    if (self_bitmap) {
-        red_put_image(self_bitmap);
-    }
     if (--drawable->refs) {
         return;
     }
-
+    if (self_bitmap) {
+        red_put_image(self_bitmap);
+    }
     worker->red_drawable_count--;
     release_info_ext.group_id = group_id;
     release_info_ext.info = drawable->release_info;