[Spice-devel] [PATCH spice-server 3/5] red-qxl: Avoid using dandling pointers to RedClient

[Christophe Fergeau]

On Thu, Aug 24, 2017 at 03:37:18PM +0100, Frediano Ziglio wrote:
> A RedClient can be freed from the main thread following
> a main channel disconnection.

I'd mention reds_client_disconnect or some concrete function name so
that someone wanting to follow that code has something to start from.

> This can happen while
> another thread is allocating a new channel client for
> that client.
> To prevent the usage of a pointer which can be invalid
> take ownership of the pointer.
> Note that we don't need this disconnecting as disconnection

"when disconnecting"

> is done synchronously (the dispatch messages are registered
> with DISPATCH_ACK).


> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-qxl.c    | 6 ++++++
>  server/red-worker.c | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 53f3338b..168b9f0d 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -83,6 +83,9 @@ static void red_qxl_set_display_peer(RedChannel *channel, RedClient *client,
>  
>      spice_debug("%s", "");
>      dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
> +    // get a reference potentially the main channel can be destroyed in
> +    // the main thread causing RedClient to be destroyed before using it
> +    g_object_ref(client);
>      payload.client = client;

payload.client = g_object_ref(client); works too.


Acked-by: Christophe Fergeau <cfergeau at redhat.com>

>      payload.stream = stream;
>      payload.migration = migration;
> @@ -141,6 +144,9 @@ static void red_qxl_set_cursor_peer(RedChannel *channel, RedClient *client, Reds
>      RedWorkerMessageCursorConnect payload = {0,};
>      Dispatcher *dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
>      spice_printerr("");
> +    // get a reference potentially the main channel can be destroyed in
> +    // the main thread causing RedClient to be destroyed before using it
> +    g_object_ref(client);
>      payload.client = client;
>      payload.stream = stream;
>      payload.migration = migration;
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 8fd964ea..fa69da2f 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -724,6 +724,7 @@ static void handle_dev_display_connect(void *opaque, void *payload)
>  
>      dcc = dcc_new(display, msg->client, msg->stream, msg->migration, &msg->caps,
>                    worker->image_compression, worker->jpeg_state, worker->zlib_glz_state);
> +    g_object_unref(msg->client);
>      red_channel_capabilities_reset(&msg->caps);
>      if (!dcc) {
>          return;
> @@ -816,6 +817,7 @@ static void handle_dev_cursor_connect(void *opaque, void *payload)
>      cursor_channel_connect(worker->cursor_channel,
>                             msg->client, msg->stream, msg->migration,
>                             &msg->caps);
> +    g_object_unref(msg->client);
>      red_channel_capabilities_reset(&msg->caps);
>  }
>  
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel