[Spice-commits] 8 commits - server/main_channel.c server/main_dispatcher.c server/main_dispatcher.h server/red_channel.c server/red_channel.h server/reds.c server/reds.h server/snd_worker.c

Yonit Halperin yhalperi at kemper.freedesktop.org
Mon Jul 29 08:34:30 PDT 2013


 server/main_channel.c    |    9 +++---
 server/main_dispatcher.c |   43 +++++++++++++++++++++++++++++++-
 server/main_dispatcher.h |    7 +++++
 server/red_channel.c     |   62 +++++++++++++++++++++++++++++++++++++----------
 server/red_channel.h     |   17 +++++++++++-
 server/reds.c            |    8 +++---
 server/reds.h            |    2 +
 server/snd_worker.c      |   36 +++++++++++++--------------
 8 files changed, 143 insertions(+), 41 deletions(-)

New commits:
commit c2e46b926e0ee4226f0f93942e7fa2c5b409f73d
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Fri Jul 26 12:15:00 2013 -0400

    log: improve debug information related to client disconnection

diff --git a/server/red_channel.c b/server/red_channel.c
index 9168b8a..d565634 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1112,6 +1112,7 @@ static void red_channel_client_ref(RedChannelClient *rcc)
 static void red_channel_client_unref(RedChannelClient *rcc)
 {
     if (!--rcc->refs) {
+        spice_debug("destroy rcc=%p", rcc);
         if (rcc->send_data.main.marshaller) {
             spice_marshaller_destroy(rcc->send_data.main.marshaller);
         }
@@ -1708,6 +1709,8 @@ static void red_channel_client_disconnect_dummy(RedChannelClient *rcc)
 {
     spice_assert(rcc->dummy);
     if (ring_item_is_linked(&rcc->channel_link)) {
+        spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel,
+                       rcc->channel->type, rcc->channel->id);
         red_channel_remove_client(rcc);
     }
     rcc->dummy_connected = FALSE;
@@ -1715,8 +1718,6 @@ static void red_channel_client_disconnect_dummy(RedChannelClient *rcc)
 
 void red_channel_client_disconnect(RedChannelClient *rcc)
 {
-    spice_printerr("%p (channel %p type %d id %d)", rcc, rcc->channel,
-                                                rcc->channel->type, rcc->channel->id);
     if (rcc->dummy) {
         red_channel_client_disconnect_dummy(rcc);
         return;
@@ -1724,6 +1725,8 @@ void red_channel_client_disconnect(RedChannelClient *rcc)
     if (!red_channel_client_is_connected(rcc)) {
         return;
     }
+    spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel,
+                   rcc->channel->type, rcc->channel->id);
     red_channel_client_pipe_clear(rcc);
     if (rcc->stream->watch) {
         rcc->channel->core->watch_remove(rcc->stream->watch);
@@ -2007,7 +2010,7 @@ void red_client_destroy(RedClient *client)
     RingItem *link, *next;
     RedChannelClient *rcc;
 
-    spice_printerr("destroy client with #channels %d", client->channels_num);
+    spice_printerr("destroy client %p with #channels=%d", client, client->channels_num);
     if (!pthread_equal(pthread_self(), client->thread_id)) {
         spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
                       "If one of the threads is != io-thread && != vcpu-thread,"
diff --git a/server/snd_worker.c b/server/snd_worker.c
index 3827416..ebddfcd 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -201,8 +201,8 @@ static SndChannel *snd_channel_get(SndChannel *channel)
 static SndChannel *snd_channel_put(SndChannel *channel)
 {
     if (!--channel->refs) {
+        spice_printerr("SndChannel=%p freed", channel);
         free(channel);
-        spice_printerr("sound channel freed");
         return NULL;
     }
     return channel;
@@ -216,7 +216,8 @@ static void snd_disconnect_channel(SndChannel *channel)
         spice_debug("not connected");
         return;
     }
-    spice_debug("%p", channel);
+    spice_debug("SndChannel=%p rcc=%p type=%d",
+                 channel, channel->channel_client, channel->channel_client->channel->type);
     worker = channel->worker;
     channel->cleanup(channel);
     red_channel_client_disconnect(worker->connection->channel_client);
@@ -976,11 +977,11 @@ static void snd_disconnect_channel_client(RedChannelClient *rcc)
 {
     SndWorker *worker;
 
-    spice_debug(NULL);
     spice_assert(rcc->channel);
     spice_assert(rcc->channel->data);
     worker = (SndWorker *)rcc->channel->data;
 
+    spice_debug("channel-type=%d", rcc->channel->type);
     if (worker->connection) {
         spice_assert(worker->connection->channel_client == rcc);
         snd_disconnect_channel(worker->connection);
commit 02f44c137df99ed2e89699e49b64c13673b0cd06
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Thu Jul 25 15:07:43 2013 -0400

    snd_worker/snd_disconnect_channel: don't call snd_channel_put if the channel has already been disconnected
    
    The snd channels has one reference as long as their socket is active.
    The playback channel has an additional reference for each frame that is
    currently filled by the sound device.
    Once the channel is disconnected (the socket has been freed and the
    first reference is released) snd_disconnect_channel shouldn't release
    a reference again.

diff --git a/server/snd_worker.c b/server/snd_worker.c
index 849f002..3827416 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -212,21 +212,20 @@ static void snd_disconnect_channel(SndChannel *channel)
 {
     SndWorker *worker;
 
-    if (!channel) {
+    if (!channel || !channel->stream) {
         spice_debug("not connected");
         return;
     }
     spice_debug("%p", channel);
     worker = channel->worker;
-    if (channel->stream) {
-        channel->cleanup(channel);
-        red_channel_client_disconnect(worker->connection->channel_client);
-        core->watch_remove(channel->stream->watch);
-        channel->stream->watch = NULL;
-        reds_stream_free(channel->stream);
-        channel->stream = NULL;
-        spice_marshaller_destroy(channel->send_data.marshaller);
-    }
+    channel->cleanup(channel);
+    red_channel_client_disconnect(worker->connection->channel_client);
+    worker->connection->channel_client = NULL;
+    core->watch_remove(channel->stream->watch);
+    channel->stream->watch = NULL;
+    reds_stream_free(channel->stream);
+    channel->stream = NULL;
+    spice_marshaller_destroy(channel->send_data.marshaller);
     snd_channel_put(channel);
     worker->connection = NULL;
 }
@@ -897,6 +896,7 @@ static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t channel_i
     int tos;
     MainChannelClient *mcc = red_client_get_main(client);
 
+    spice_assert(stream);
     if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
         spice_printerr("accept failed, %s", strerror(errno));
         goto error1;
commit 134b7f310de5120b233670d18641d32204f31318
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Thu Jul 25 14:49:33 2013 -0400

    snd_worker: fix memory leak of PlaybackChannel
    
    When the sequence of calls bellow occurs, the PlaybackChannel
    is not released (snd_channel_put is not called for the
    samples that refer to the channel).
    
        spice_server_playback_get_buffer
        snd_channel_disconnect
        spice_server_playback_put_samples

diff --git a/server/snd_worker.c b/server/snd_worker.c
index d6ec47a..849f002 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -1097,14 +1097,13 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance
     PlaybackChannel *playback_channel;
     AudioFrame *frame;
 
-    if (!sin->st->worker.connection) {
-        return;
-    }
-
     frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
     playback_channel = frame->channel;
-    if (!snd_channel_put(&playback_channel->base) || !playback_channel->base.worker->connection) {
+    spice_assert(playback_channel);
+    if (!snd_channel_put(&playback_channel->base) ||
+        sin->st->worker.connection != &playback_channel->base) {
         /* lost last reference, channel has been destroyed previously */
+        spice_info("audio samples belong to a disconnected channel");
         return;
     }
     spice_assert(playback_channel->base.active);
commit 46c2ce8f1af0170a2c6a335cc743a3ddff2d96d0
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Thu Jul 25 14:25:24 2013 -0400

    reds: s/red_client_disconnect/red_channel_client_shutdown inside callbacks
    
    When we want to disconnect the main channel from a callback, it is
    safer to use red_channel_client_shutdown, instead of directly
    destroying the client. It is also more consistent with how other
    channels treat errors.
    red_channel_client_shutdown will trigger socket error in the main channel.
    Then, main_channel_client_on_disconnect will be called,
    and eventually, main_dispatcher_client_disconnect.
    
    I didn't replace calls to reds_disconnect/reds_client_disconnect in
    places where those calls were safe && that might need immediate client
    disconnection.

diff --git a/server/reds.c b/server/reds.c
index c66ddc4..ae87c90 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -879,7 +879,8 @@ static void vdi_port_on_free_self_token(void *opaque)
 
 static void vdi_port_remove_client(RedClient *client, void *opaque)
 {
-    reds_client_disconnect(client);
+    red_channel_client_shutdown(main_channel_client_get_base(
+                                    red_client_get_main(client)));
 }
 
 /****************************************************************************/
@@ -1009,7 +1010,7 @@ void reds_on_main_agent_start(MainChannelClient *mcc, uint32_t num_tokens)
 
         if (!client_added) {
             spice_warning("failed to add client to agent");
-            reds_client_disconnect(rcc->client);
+            red_channel_client_shutdown(rcc);
             return;
         }
     } else {
@@ -1126,7 +1127,7 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
         reds_on_main_agent_monitors_config(mcc, message, size);
         return;
     case AGENT_MSG_FILTER_PROTO_ERROR:
-        reds_disconnect();
+        red_channel_client_shutdown(main_channel_client_get_base(mcc));
         return;
     }
 
commit 8490f83e1f51ac8dbcac3c68c97827e0acb9ec4e
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Thu Jul 25 14:19:21 2013 -0400

    decouple disconnection of the main channel from client destruction
    
    Fixes rhbz#918169
    
    Some channels make direct calls to reds/main_channel routines. If
    these routines try to read/write to the socket, and they get socket
    error, main_channel_client_on_disconnect is called, and triggers
    red_client_destroy. In order to prevent accessing expired references
    to RedClient, RedChannelClient, or other objects (inside the original call, after
    red_client_destroy has been called) I made the call to
    red_client_destroy asynchronous with respect to main_channel_client_on_disconnect.
    I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher.
    main_channel_client_on_disconnect pushes this msg to the dispatcher,
    instead of calling directly to reds_client_disconnect.
    
    The patch uses RedClient ref-count in order to handle a case where
    reds_client_disconnect is called directly (e.g., when a new client connects while
    another one is connected), while there is already CLIENT_DISCONNECT msg
    pending in the main_dispatcher.
    
    Examples:
    (1) snd_worker.c
    
        snd_disconnect_channel()
            channel->cleanup() //snd_playback_cleanup
                reds_enable_mm_timer()
                    .
                    .
                    main_channel_push_multi_media_time()...socket_error
                        .
                        .
                        red_client_destory()
                            .
                            .
                            snd_disconnect_channel()
                                channel->cleanup()
                                    celt051_encoder_destroy()
                celt051_encoder_destory() // double release
    
    Note that this bug could have been solved by changing the order of
    calls: e.g., channel->stream = NULL before calling cleanup, and
    some other changes + reference counting. However, I found other
    places in the code with similar problems, and I looked for a general
    solution, at least till we redesign red_channel to handle reference
    counting more consistently.
    
    (2) inputs_channel.c
    
        inputs_connect()
            main_channel_client_push_notify()...socket_error
                    .
                    .
                red_client_destory()
                    .
                    .
            red_channel_client_create() // refers to client which is already destroyed
    
    (3) reds.c
    
        reds_handle_main_link()
           main_channel_push_init() ...socket error
                    .
                    .
                red_client_destory()
                    .
                    .
           main_channel_client_start_net_test(mcc) // refers to mcc which is already destroyed
    
        This can explain the assert in rhbz#964136, comment #1 (but not the hang that occurred before).

diff --git a/server/main_channel.c b/server/main_channel.c
index 233e633..fe032a6 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -46,6 +46,7 @@
 #include "red_common.h"
 #include "reds.h"
 #include "migration_protocol.h"
+#include "main_dispatcher.h"
 
 #define ZERO_BUF_SIZE 4096
 
@@ -175,13 +176,13 @@ int main_channel_is_connected(MainChannel *main_chan)
     return red_channel_is_connected(&main_chan->base);
 }
 
-// when disconnection occurs, let reds shutdown all channels. This will trigger the
-// real disconnection of main channel
+/*
+ * When the main channel is disconnected, disconnect the entire client.
+ */
 static void main_channel_client_on_disconnect(RedChannelClient *rcc)
 {
     spice_printerr("rcc=%p", rcc);
-    reds_client_disconnect(rcc->client);
-//    red_channel_client_disconnect(rcc);
+    main_dispatcher_client_disconnect(rcc->client);
 }
 
 RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t connection_id)
diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index bf160dd..dbe1037 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -41,6 +41,7 @@ enum {
     MAIN_DISPATCHER_CHANNEL_EVENT = 0,
     MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
     MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
+    MAIN_DISPATCHER_CLIENT_DISCONNECT,
 
     MAIN_DISPATCHER_NUM_MESSAGES
 };
@@ -59,6 +60,10 @@ typedef struct MainDispatcherMmTimeLatencyMessage {
     uint32_t latency;
 } MainDispatcherMmTimeLatencyMessage;
 
+typedef struct MainDispatcherClientDisconnectMessage {
+    RedClient *client;
+} MainDispatcherClientDisconnectMessage;
+
 /* channel_event - calls core->channel_event, must be done in main thread */
 static void main_dispatcher_self_handle_channel_event(
                                                 int event,
@@ -108,6 +113,16 @@ static void main_dispatcher_handle_mm_time_latency(void *opaque,
     red_client_unref(msg->client);
 }
 
+static void main_dispatcher_handle_client_disconnect(void *opaque,
+                                                     void *payload)
+{
+    MainDispatcherClientDisconnectMessage *msg = payload;
+
+    spice_debug("client=%p", msg->client);
+    reds_client_disconnect(msg->client);
+    red_client_unref(msg->client);
+}
+
 void main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
 {
     MainDispatcherMigrateSeamlessDstCompleteMessage msg;
@@ -137,6 +152,20 @@ void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency)
                             &msg);
 }
 
+void main_dispatcher_client_disconnect(RedClient *client)
+{
+    MainDispatcherClientDisconnectMessage msg;
+
+    if (!client->disconnecting) {
+        spice_debug("client %p", client);
+        msg.client = red_client_ref(client);
+        dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT,
+                                &msg);
+    } else {
+        spice_debug("client %p already during disconnection", client);
+    }
+}
+
 static void dispatcher_handle_read(int fd, int event, void *opaque)
 {
     Dispatcher *dispatcher = opaque;
@@ -144,6 +173,11 @@ static void dispatcher_handle_read(int fd, int event, void *opaque)
     dispatcher_handle_recv_read(dispatcher);
 }
 
+/*
+ * FIXME:
+ * Reds routines shouldn't be exposed. Instead reds.c should register the callbacks,
+ * and the corresponding operations should be made only via main_dispatcher.
+ */
 void main_dispatcher_init(SpiceCoreInterface *core)
 {
     memset(&main_dispatcher, 0, sizeof(main_dispatcher));
@@ -160,4 +194,7 @@ void main_dispatcher_init(SpiceCoreInterface *core)
     dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
                                 main_dispatcher_handle_mm_time_latency,
                                 sizeof(MainDispatcherMmTimeLatencyMessage), 0 /* no ack */);
+    dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT,
+                                main_dispatcher_handle_client_disconnect,
+                                sizeof(MainDispatcherClientDisconnectMessage), 0 /* no ack */);
 }
diff --git a/server/main_dispatcher.h b/server/main_dispatcher.h
index 0c79ca8..522c7f9 100644
--- a/server/main_dispatcher.h
+++ b/server/main_dispatcher.h
@@ -7,6 +7,13 @@
 void main_dispatcher_channel_event(int event, SpiceChannelEventInfo *info);
 void main_dispatcher_seamless_migrate_dst_complete(RedClient *client);
 void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency);
+/*
+ * Disconnecting the client is always executed asynchronously,
+ * in order to protect from expired references in the routines
+ * that triggered the client destruction.
+ */
+void main_dispatcher_client_disconnect(RedClient *client);
+
 void main_dispatcher_init(SpiceCoreInterface *core);
 
 #endif //MAIN_DISPATCHER_H
diff --git a/server/reds.c b/server/reds.c
index 30d0652..c66ddc4 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -537,6 +537,7 @@ void reds_client_disconnect(RedClient *client)
     }
 
     if (!client || client->disconnecting) {
+        spice_debug("client %p already during disconnection", client);
         return;
     }
 
diff --git a/server/reds.h b/server/reds.h
index c5c557d..1c5ae84 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -136,6 +136,8 @@ void reds_handle_agent_mouse_event(const VDAgentMouseState *mouse_state); // use
 extern struct SpiceCoreInterface *core;
 
 // Temporary measures to make splitting reds.c to inputs_channel.c easier
+
+/* should be called only from main_dispatcher */
 void reds_client_disconnect(RedClient *client);
 
 // Temporary (?) for splitting main channel
commit 06ba03b7b32a2f0c7f78c82d8f399242526a0b45
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Fri Jul 26 13:49:24 2013 -0400

    main_dispatcher: add ref count protection to RedClient instances

diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index e7a451a..bf160dd 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -97,6 +97,7 @@ static void main_dispatcher_handle_migrate_complete(void *opaque,
     MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete = payload;
 
     reds_on_client_seamless_migrate_complete(mig_complete->client);
+    red_client_unref(mig_complete->client);
 }
 
 static void main_dispatcher_handle_mm_time_latency(void *opaque,
@@ -104,6 +105,7 @@ static void main_dispatcher_handle_mm_time_latency(void *opaque,
 {
     MainDispatcherMmTimeLatencyMessage *msg = payload;
     reds_set_client_mm_time_latency(msg->client, msg->latency);
+    red_client_unref(msg->client);
 }
 
 void main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
@@ -115,7 +117,7 @@ void main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
         return;
     }
 
-    msg.client = client;
+    msg.client = red_client_ref(client);
     dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
                             &msg);
 }
@@ -129,7 +131,7 @@ void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency)
         return;
     }
 
-    msg.client = client;
+    msg.client = red_client_ref(client);
     msg.latency = latency;
     dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
                             &msg);
commit aab45618cc12799d5f7351ef8832ae73b33057c7
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Fri Jul 26 13:45:16 2013 -0400

    red_channel: add ref count to RedClient

diff --git a/server/red_channel.c b/server/red_channel.c
index 0d74413..9168b8a 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1932,10 +1932,29 @@ RedClient *red_client_new(int migrated)
     pthread_mutex_init(&client->lock, NULL);
     client->thread_id = pthread_self();
     client->during_target_migrate = migrated;
+    client->refs = 1;
 
     return client;
 }
 
+RedClient *red_client_ref(RedClient *client)
+{
+    spice_assert(client);
+    client->refs++;
+    return client;
+}
+
+RedClient *red_client_unref(RedClient *client)
+{
+    if (!--client->refs) {
+        spice_debug("release client=%p", client);
+        pthread_mutex_destroy(&client->lock);
+        free(client);
+        return NULL;
+    }
+    return client;
+}
+
 /* client mutex should be locked before this call */
 static void red_channel_client_set_migration_seamless(RedChannelClient *rcc)
 {
@@ -2012,9 +2031,7 @@ void red_client_destroy(RedClient *client)
         spice_assert(rcc->send_data.size == 0);
         red_channel_client_destroy(rcc);
     }
-
-    pthread_mutex_destroy(&client->lock);
-    free(client);
+    red_client_unref(client);
 }
 
 /* client->lock should be locked */
diff --git a/server/red_channel.h b/server/red_channel.h
index ba299b6..0dd73ea 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -561,10 +561,25 @@ struct RedClient {
                                   is called */
     int seamless_migrate;
     int num_migrated_channels; /* for seamless - number of channels that wait for migrate data*/
+    int refs;
 };
 
 RedClient *red_client_new(int migrated);
 
+/*
+ * disconnects all the client's channels (should be called from the client's thread)
+ */
+void red_client_destroy(RedClient *client);
+
+RedClient *red_client_ref(RedClient *client);
+
+/*
+ * releases the client resources when refs == 0.
+ * We assume the red_client_derstroy was called before
+ * we reached refs==0
+ */
+RedClient *red_client_unref(RedClient *client);
+
 MainChannelClient *red_client_get_main(RedClient *client);
 // main should be set once before all the other channels are created
 void red_client_set_main(RedClient *client, MainChannelClient *mcc);
@@ -580,7 +595,5 @@ void red_client_semi_seamless_migrate_complete(RedClient *client); /* dst side *
 int red_client_during_migrate_at_target(RedClient *client);
 
 void red_client_migrate(RedClient *client);
-// disconnects all the client's channels (should be called from the client's thread)
-void red_client_destroy(RedClient *client);
 
 #endif
commit 47e722b85ccd0b6876ca189a3d6f6f05289fe3c3
Author: Yonit Halperin <yhalperi at redhat.com>
Date:   Wed Jul 24 14:54:23 2013 -0400

    red_channel: prevent adding and pushing pipe items after a channel_client has diconnected
    
    Fixes: leaks of pipe items & "red_client_destroy: assertion `rcc->send_data.size == 0'"
    
    red_channel_disconnect clears the pipe. It is called only once. After,
    it was called, not items should be added to the pipe.
    
    An example of when this assert can occur:
    on_new_cursor_channel (red_worker.c), pushes 2 pipe items.
    When it pushes the first pipe item, if the client has disconnected,
    it can hit a socket error, and then, red_channel_client_disconnect is called.
    The second call to adding a pipe item, will add the item to
    the pipe. red_channel_client_pipe_add_type also calls
    red_channel_client_push, which will update the send_data.size.
    Then, the push will also hit a socket error, but red_channel_client_disconnect
    won't clear the pending pipe item again, since it was already called.
    When red_client_destory is called, we hit assertion `rcc->send_data.size
    == 0'.
    Note that if a pipe item is added to the pipe after
    red_channel_client_disconnect was called, but without pushing it,
    we should hit "spice_assert(rcc->pipe_size == 0)".

diff --git a/server/red_channel.c b/server/red_channel.c
index 33af388..0d74413 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1527,9 +1527,23 @@ void red_channel_pipe_item_init(RedChannel *channel, PipeItem *item, int type)
     item->type = type;
 }
 
-void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
+static inline int validate_pipe_add(RedChannelClient *rcc, PipeItem *item)
 {
     spice_assert(rcc && item);
+    if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) {
+        spice_debug("rcc is disconnected %p", rcc);
+        red_channel_client_release_item(rcc, item, FALSE);
+        return FALSE;
+    }
+    return TRUE;
+}
+
+void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
+{
+
+    if (!validate_pipe_add(rcc, item)) {
+        return;
+    }
     rcc->pipe_size++;
     ring_add(&rcc->pipe, &item->link);
 }
@@ -1543,10 +1557,10 @@ void red_channel_client_pipe_add_push(RedChannelClient *rcc, PipeItem *item)
 void red_channel_client_pipe_add_after(RedChannelClient *rcc,
                                        PipeItem *item, PipeItem *pos)
 {
-    spice_assert(rcc);
     spice_assert(pos);
-    spice_assert(item);
-
+    if (!validate_pipe_add(rcc, item)) {
+        return;
+    }
     rcc->pipe_size++;
     ring_add_after(&item->link, &pos->link);
 }
@@ -1560,14 +1574,18 @@ int red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
 void red_channel_client_pipe_add_tail_no_push(RedChannelClient *rcc,
                                               PipeItem *item)
 {
-    spice_assert(rcc);
+    if (!validate_pipe_add(rcc, item)) {
+        return;
+    }
     rcc->pipe_size++;
     ring_add_before(&item->link, &rcc->pipe);
 }
 
 void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item)
 {
-    spice_assert(rcc);
+    if (!validate_pipe_add(rcc, item)) {
+        return;
+    }
     rcc->pipe_size++;
     ring_add_before(&item->link, &rcc->pipe);
     red_channel_client_push(rcc);


More information about the Spice-commits mailing list