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

[Hans de Goede]

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

New commits:
commit 50efe1e48d2fcf6a2f12c933ce29e73b6985f599
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Thu Jan 10 23:30:34 2013 +0100

    worker_update_monitors_config: Drop bogus real_count accounting
    
    1) This does not buy us much, as red_marshall_monitors_config() also
       removes 0x0 sized monitors and does a much better job at it
       (also removing intermediate ones, not only tailing ones)
    2) The code is wrong, as it allocs space for real_count heads, where
       real_count always <= monitors_config->count and then stores
       monitors_config->count in worker->monitors_config->count, causing
       red_marshall_monitors_config to potentially walk
       worker->monitors_config->heads past its boundaries.
    
    Signed-off-by: Hans de Goede <hdegoede at redhat.com>

diff --git a/server/red_worker.c b/server/red_worker.c
index de070a6..11fa126 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10951,7 +10951,6 @@ static void worker_update_monitors_config(RedWorker *worker,
 {
     int heads_size;
     MonitorsConfig *monitors_config;
-    int real_count = 0;
     int i;
 
     if (worker->monitors_config) {
@@ -10968,19 +10967,7 @@ static void worker_update_monitors_config(RedWorker *worker,
                     dev_monitors_config->heads[i].width,
                     dev_monitors_config->heads[i].height);
     }
-
-    // Ignore any empty sized monitors at the end of the config.
-    // 4: {w1,h1},{w2,h2},{0,0},{0,0} -> 2: {w1,h1},{w2,h2}
-    for (i = dev_monitors_config->count ; i > 0 ; --i) {
-        if (dev_monitors_config->heads[i - 1].width > 0 &&
-            dev_monitors_config->heads[i - 1].height > 0) {
-            real_count = i;
-            break;
-        }
-    }
-    heads_size = real_count * sizeof(QXLHead);
-    spice_debug("new working monitor config (count: %d, real: %d)",
-                dev_monitors_config->count, real_count);
+    heads_size = dev_monitors_config->count * sizeof(QXLHead);
     worker->monitors_config = monitors_config =
         spice_malloc(sizeof(*monitors_config) + heads_size);
     monitors_config->refs = 1;
commit d2e1f939fec9b4d852a009cc55c4bbb3d2a94d3b
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Thu Jan 10 22:55:51 2013 +0100

    server: Fix SpiceWorker-CRITICAL **: red_worker.c:10968:red_push_monitors_config: condition `monitors_config != NULL' failed
    
    During my dynamic monitor support testing today, I hit the following assert
    in red_worker.c:
    "red_push_monitors_config: condition `monitors_config != NULL' failed"
    
    This is caused by the following scenario:
    1) Guest causes handle_dev_monitors_config_async() to be called
    2) handle_dev_monitors_config_async() calls worker_update_monitors_config()
    3) handle_dev_monitors_config_async() pushes worker->monitors_config, this
       takes a ref on the current monitors_config
    4) Guest causes handle_dev_monitors_config_async() to be called *again*
    5) handle_dev_monitors_config_async() calls worker_update_monitors_config()
    6) worker_update_monitors_config() does a decref on worker->monitors_config,
       releasing the workers reference, this monitor_config from step 2 is
       not yet free-ed though as the pipe-item still holds a ref
    7) worker_update_monitors_config() creates a new monitors_config with an
       initial ref-count of 1 and stores that in worker->monitors_config
    8) The pipe-item of the *first* monitors_config is send, upon completion
       a decref is done on the monitors_config, and monitors_config_decref not
       only frees the monitor_config, but *also* sets worker->monitors_config
       to NULL, even though worker->monitors_config no longer refers to the
       monitor_config being freed, it refers to the 2nd monitor_config!
    9) The client which was connected when this all happened disconnects
    10) A new client connects, leading to the assert:
        at red_worker.c:9519
        num_common_caps=1, common_caps=0x5555569b6f60, migrate=0,
        stream=<optimized out>, client=<optimized out>, worker=<optimized out>)
        at red_worker.c:10423
        at red_worker.c:11301
    
    Note that red_worker.c:9519 is:
            red_push_monitors_config(dcc);
    gdb does not point to the actual line of the assert because the function gets
    inlined.
    
    The fix is easy and obvious, don't set worker->monitors_config to NULL in
    monitors_config_decref. I'm a bit baffled as to why that code is there in
    the first place, the whole point of ref-counting is to not have one single
    unique place to store the reference...
    
    This fix should not have any adverse side-effects as the 4 callers of
    monitors_config_decref fall into 2 categories:
    1) Code which immediately after the decref replaces worker->monitors_config
       with a new monitors_config:
       worker_update_monitors_config()
       set_monitors_config_to_primary()
    2) pipe-item freeing code, which should not touch the worker state at all
       to being with
    
    Signed-off-by: Hans de Goede <hdegoede at redhat.com>

diff --git a/server/red_worker.c b/server/red_worker.c
index 085e3e6..de070a6 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1239,8 +1239,7 @@ static void monitors_config_decref(MonitorsConfig *monitors_config)
         return;
     }
 
-    spice_debug("removing worker monitors config");
-    monitors_config->worker->monitors_config = NULL;
+    spice_debug("freeing monitors config");
     free(monitors_config);
 }