[Spice-devel] [PATCH spice-server 06/10] red-channel-client: Early check for valid stream

[Christophe Fergeau]

On Mon, Sep 11, 2017 at 11:15:43AM +0100, Frediano Ziglio wrote:
> Code tested for the presence of a stream while initializing
> RedChannelClient.

"The code tests for the presence of RedChannelClient::stream while
initializing RedChannelClient"

> However the check was done too late possibly using the invalid
> pointer in red_channel_client_config_socket.

"However, the check was done too late, and a
RedChannelClient::config_socket implementation (for example
snd_channel_client_config_socket) could have tried to use
it before the check that it's not NULL."


Interestingly, this was introduced in 654dfa4c
"red-channel-client: Change initialization order"
The commit log does not say much, but this apparently was only changed
to be closer to older code, not to fix an actual issue (
https://lists.freedesktop.org/archives/spice-devel/2016-October/033161.html
)

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

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> Can the stream be NULL? By the way, I think this version
> does not introduce possible regressions.
> ---
>  server/red-channel-client.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 9a47b7e6f..34202c492 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -922,6 +922,14 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
>      SpiceCoreInterfaceInternal *core;
>      RedChannelClient *self = RED_CHANNEL_CLIENT(initable);
>  
> +    if (!self->priv->stream) {
> +        g_set_error_literal(&local_error,
> +                            SPICE_SERVER_ERROR,
> +                            SPICE_SERVER_ERROR_FAILED,
> +                            "Socket not available");
> +        goto cleanup;
> +    }
> +
>      if (!red_channel_client_config_socket(self)) {
>          g_set_error_literal(&local_error,
>                              SPICE_SERVER_ERROR,
> @@ -931,14 +939,12 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
>      }
>  
>      core = red_channel_get_core_interface(self->priv->channel);
> -    if (self->priv->stream) {
> -        reds_stream_set_core_interface(self->priv->stream, core);
> -        self->priv->stream->watch =
> -            core->watch_add(core, self->priv->stream->socket,
> -                            SPICE_WATCH_EVENT_READ,
> -                            red_channel_client_event,
> -                            self);
> -    }
> +    reds_stream_set_core_interface(self->priv->stream, core);
> +    self->priv->stream->watch =
> +        core->watch_add(core, self->priv->stream->socket,
> +                        SPICE_WATCH_EVENT_READ,
> +                        red_channel_client_event,
> +                        self);
>  
>      if (self->priv->monitor_latency
>          && reds_stream_get_family(self->priv->stream) != AF_UNIX) {
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel