[Spice-commits] 2 commits - server/red-channel-client.c server/reds-stream.c server/reds-stream.h

[Frediano Ziglio]

 server/red-channel-client.c |   14 +++++++++++++-
 server/reds-stream.c        |   18 +++++++++++++-----
 server/reds-stream.h        |    1 +
 3 files changed, 27 insertions(+), 6 deletions(-)

New commits:
commit 5d7ecb01621e7cacb8840abe2851b95916d45700
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Aug 30 10:27:33 2017 +0100

    Fix crash attempting to connect duplicate channels
    
    You could easily trigger this issue using multiple monitors and a
    modified spice-gtk client with this patch:
    
      --- a/src/channel-main.c
      +++ b/src/channel-main.c
      @@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
       {
           g_return_val_if_fail(c != NULL, FALSE);
    
      +    if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
           spice_channel_new(c->session, c->type, c->id);
    
           g_object_unref(c->session);
    
    which cause a crash like
    
    (process:28742): Spice-WARNING **: Failed to create channel client: Client 0x40246f5d0: duplicate channel type 2 id 0
    2017-08-24 09:36:57.451+0000: shutting down, reason=crashed
    
    RedChannelClient is an GInitable type, which means that the object is
    constructed, and then the _init() function is called, which can fail.
    If the _init() fails, the newly-created object will be destroyed. As
    part of _init(), we add a new watch for the stream using the core
    interface that is associated with the channel. After adding the watch,
    our rcc creation fails (due to duplicate ID), and the rcc object is
    unreffed. This results in a call to reds_stream_free() (since the rcc
    now owns the stream). But in reds_stream_free, we were trying to remove
    the watch from the core interface associated with the RedsState. For
    most channels, these two core interfaces are equivalent. But for the
    Display and Cursor channels, it is the core Glib-based interface
    associated with the RedWorker.
    
    The watch in RedsStream by default is bound to the Qemu provided
    SpiceCoreInterface while RedChannelClient bound it to Glib one causing
    the crash when the watch is deleted from RedsStream. Change the bound
    interface.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 655e41c7..35bb8d92 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -339,6 +339,16 @@ red_channel_client_finalize(GObject *object)
 {
     RedChannelClient *self = RED_CHANNEL_CLIENT(object);
 
+    SpiceCoreInterfaceInternal *core = red_channel_get_core_interface(self->priv->channel);
+    if (self->priv->latency_monitor.timer) {
+        core->timer_remove(core, self->priv->latency_monitor.timer);
+        self->priv->latency_monitor.timer = NULL;
+    }
+    if (self->priv->connectivity_monitor.timer) {
+        core->timer_remove(core, self->priv->connectivity_monitor.timer);
+        self->priv->connectivity_monitor.timer = NULL;
+    }
+
     reds_stream_free(self->priv->stream);
     self->priv->stream = NULL;
 
@@ -921,12 +931,14 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
     }
 
     core = red_channel_get_core_interface(self->priv->channel);
-    if (self->priv->stream)
+    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);
+    }
 
     if (self->priv->monitor_latency
         && reds_stream_get_family(self->priv->stream) != AF_UNIX) {
commit 692c133765f3bf52866448fae26665a098f62c97
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Aug 30 10:13:43 2017 +0100

    reds-stream: Allows to change core interface
    
    When a stream is moved from the main thread to a
    secondary one the events are potentially registered
    using a different core interface. This cause memory
    corruption accessing the watch registered in RedsStream.
    This patch allows to always use the right interface.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/server/reds-stream.c b/server/reds-stream.c
index 2ea54756..e5336265 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -98,6 +98,7 @@ struct RedsStreamPrivate {
     ssize_t (*writev)(RedsStream *s, const struct iovec *iov, int iovcnt);
 
     RedsState *reds;
+    SpiceCoreInterfaceInternal *core;
 };
 
 static ssize_t stream_write_cb(RedsStream *s, const void *buf, size_t size)
@@ -170,7 +171,7 @@ static ssize_t stream_ssl_read_cb(RedsStream *s, void *buf, size_t size)
 void reds_stream_remove_watch(RedsStream* s)
 {
     if (s->watch) {
-        reds_core_watch_remove(s->priv->reds, s->watch);
+        s->priv->core->watch_remove(s->priv->core, s->watch);
         s->watch = NULL;
     }
 }
@@ -418,6 +419,7 @@ RedsStream *reds_stream_new(RedsState *reds, int socket)
     stream->priv = (RedsStreamPrivate *)(stream+1);
     stream->priv->info = spice_new0(SpiceChannelEventInfo, 1);
     stream->priv->reds = reds;
+    stream->priv->core = reds_get_core_interface(reds);
     reds_stream_set_socket(stream, socket);
 
     stream->priv->read = stream_read_cb;
@@ -427,6 +429,12 @@ RedsStream *reds_stream_new(RedsState *reds, int socket)
     return stream;
 }
 
+void reds_stream_set_core_interface(RedsStream *stream, SpiceCoreInterfaceInternal *core)
+{
+    reds_stream_remove_watch(stream);
+    stream->priv->core = core;
+}
+
 bool reds_stream_is_ssl(RedsStream *stream)
 {
     return (stream->priv->ssl != NULL);
@@ -511,7 +519,7 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
 {
     AsyncRead *async = (AsyncRead *)data;
     RedsStream *stream = async->stream;
-    RedsState *reds = stream->priv->reds;
+    SpiceCoreInterfaceInternal *core = stream->priv->core;
 
     for (;;) {
         int n = async->end - async->now;
@@ -523,9 +531,9 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
             switch (err) {
             case EAGAIN:
                 if (!stream->watch) {
-                    stream->watch = reds_core_watch_add(reds, stream->socket,
-                                                        SPICE_WATCH_EVENT_READ,
-                                                        async_read_handler, async);
+                    stream->watch = core->watch_add(core, stream->socket,
+                                                    SPICE_WATCH_EVENT_READ,
+                                                    async_read_handler, async);
                 }
                 return;
             case EINTR:
diff --git a/server/reds-stream.h b/server/reds-stream.h
index 2dc39c23..55a82745 100644
--- a/server/reds-stream.h
+++ b/server/reds-stream.h
@@ -67,6 +67,7 @@ void reds_stream_remove_watch(RedsStream* s);
 void reds_stream_set_channel(RedsStream *stream, int connection_id,
                              int channel_type, int channel_id);
 RedsStream *reds_stream_new(RedsState *reds, int socket);
+void reds_stream_set_core_interface(RedsStream *stream, SpiceCoreInterfaceInternal *core);
 bool reds_stream_is_ssl(RedsStream *stream);
 RedsStreamSslStatus reds_stream_ssl_accept(RedsStream *stream);
 int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);