[Spice-commits] src/channel-webdav.c

[GitLab Mirror]

 src/channel-webdav.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

New commits:
commit e4a5eb1d5043b0258b969c95faa818b1661f9d9d
Author: Jakub Janků <jjanku at redhat.com>
Date:   Mon Jul 20 12:28:57 2020 +0200

    webdav: fix race caused by libsoup
    
    If we read 0 bytes from the phodav server,
    the client connection was closed by the server.
    
    In that case, remove the client immediately,
    instead of waiting for the data to be written to the mux channel.
    
    Fixes: https://lists.freedesktop.org/archives/spice-devel/2020-July/051765.html
    
    I believe that the underlying issue, that is causing the described problems,
    is that libsoup closes the connection after each request
    although the webdav client wanted it to be persistant (keep-alive).
    
    Filed an issue for libsoup:
    https://gitlab.gnome.org/GNOME/libsoup/-/issues/195
    
    Additionally, spice-webdavd does not close the connection
    when it receives 0-size message for a given webdav client.
    
    As a result, single webdav client can send multiple request using one
    connection to spice-webdavd, but in spice-gtk for each request
    a new client with the same id must be created.
    
    This leads to basically a race condition when there's still the
    client with the given id in the hash table, but the connection
    was already closed by libsoup/phodav server. So a new client should
    be started but the old closed one is used. The write op then
    naturally fails with the following error and the request is lost:
    
        ../spice-gtk-0.38/src/channel-webdav.c:325 webdav-11:0: write failed: Stream is already closed
    
    So check whether the output stream is closed before demuxing.
    
    Also add some debug messages.
    
    Signed-off-by: Jakub Janků <jjanku at redhat.com>
    Acked-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/src/channel-webdav.c b/src/channel-webdav.c
index c4ad144..7de5495 100644
--- a/src/channel-webdav.c
+++ b/src/channel-webdav.c
@@ -222,6 +222,8 @@ static void remove_client(Client *client)
     if (g_cancellable_is_cancelled(client->cancellable))
         return;
 
+    CHANNEL_DEBUG(SPICE_CHANNEL(client->self), "removing client %p", client);
+
     g_cancellable_cancel(client->cancellable);
 
     g_hash_table_remove(client->self->priv->clients, &client->id);
@@ -235,7 +237,6 @@ mux_msg_flushed_cb(GObject *source_object,
     Client *client = user_data;
 
     if (spice_vmc_write_finish(SPICE_CHANNEL(source_object), result, NULL) == -1 ||
-        client->mux.size == 0 ||
         !client_start_read(client)) {
         remove_client(client);
     }
@@ -252,6 +253,8 @@ static void server_reply_cb(GObject *source_object,
     gssize size;
 
     size = g_input_stream_read_finish(G_INPUT_STREAM(source_object), res, &err);
+    CHANNEL_DEBUG(SPICE_CHANNEL(client->self),
+        "received %"G_GSSIZE_FORMAT" B from phodav for client %p", size, client);
     if (err || g_cancellable_is_cancelled(client->cancellable))
         goto end;
 
@@ -259,6 +262,10 @@ static void server_reply_cb(GObject *source_object,
     g_return_if_fail(size >= 0);
     client->mux.size = GUINT16_TO_LE(size);
 
+    if (size == 0) {
+        remove_client(client);
+    }
+
     spice_vmc_write_async(SPICE_CHANNEL(client->self),
                           &client->mux,
                           sizeof(gint64) + sizeof(guint16) + size,
@@ -425,6 +432,12 @@ static void data_read_cb(GObject *source_object,
 
     client = g_hash_table_lookup(c->clients, &c->demux.client);
 
+    if (client && g_output_stream_is_closed(g_io_stream_get_output_stream(client->pipe))) {
+        CHANNEL_DEBUG(self, "found client %p, but it's already closed, removing", client);
+        remove_client(client);
+        client = NULL;
+    }
+
     if (client)
         demux_to_client(client);
     else if (size > 0) {