[Spice-devel] [PATCH spice 1/2] server: Fix SpiceWorker-CRITICAL **: red_worker.c:10968:red_push_monitors_config: condition `monitors_config != NULL' failed

[Alon Levy]

On Thu, Jan 10, 2013 at 11:52:38PM +0100, Hans de Goede wrote:
> 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...

The reason is me. Thanks for the fix.

> 
> 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>
> ---
>  server/red_worker.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 1a9c375..6f101ca 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -1237,8 +1237,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);
>  }
>  
> -- 
> 1.8.0.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel