[Spice-commits] 2 commits - server/reds.c server/tests

[Frediano Ziglio]

 server/reds.c             |    7 ++++---
 server/tests/Makefile.am  |    1 +
 server/tests/test-leaks.c |   12 ++++++++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

New commits:
commit 2e06a6e9400eb1d97b452eaebd3189a289033096
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Sep 11 09:25:41 2017 +0100

    reds: Fix leaks if reds_init_client_ssl_connection fails
    
    If a client is unable to complete the TLS handshake phase
    reds_init_client_ssl_connection leaked some memory as the stream is not
    correctly freed.
    This also causes the stream to send the SPICE_CHANNEL_EVENT_DISCONNECTED
    event. Otherwise only SPICE_CHANNEL_EVENT_CONNECTED was sent.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/reds.c b/server/reds.c
index 24ec2bdd..a73d00f7 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2503,9 +2503,10 @@ static RedLinkInfo *reds_init_client_ssl_connection(RedsState *reds, int socket)
     return link;
 
 error:
-    free(link->stream);
-    BN_free(link->tiTicketing.bn);
-    free(link);
+    /* close the stream but do not close the socket, this API is
+     * supposed to not close it if it fails */
+    link->stream->socket = -1;
+    reds_link_free(link);
     return NULL;
 }
 
commit 8856d518ee3ebf4220d347da637d06d00332966d
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Sep 11 09:15:33 2017 +0100

    tests: Check leaks in spice_server_add_ssl_client
    
    Currently is possible to trigger a leak by passing an invalid
    connection.
    This can happen if the client opens a connection and then closes it
    without writing or reading any data.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 01e63626..d50c590c 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -138,6 +138,7 @@ test_vdagent_CPPFLAGS =			\
 test_codecs_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
 test_qxl_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
 test_fail_on_null_core_interface_CPPFLAGS = $(test_vdagent_CPPFLAGS)
+test_leaks_CPPFLAGS = $(test_vdagent_CPPFLAGS)
 
 if HAVE_GSTREAMER
 test_gst_SOURCES = test-gst.c \
diff --git a/server/tests/test-leaks.c b/server/tests/test-leaks.c
index 74ab2afd..04dcac4f 100644
--- a/server/tests/test-leaks.c
+++ b/server/tests/test-leaks.c
@@ -29,6 +29,7 @@
  * For cleaner output you should suppress GLib checks with glib.supp file.
  */
 #include <config.h>
+#include <unistd.h>
 #include <spice.h>
 
 #include "test-glib-compat.h"
@@ -42,6 +43,7 @@ static void server_leaks(void)
     int result;
     SpiceCoreInterface *core;
     SpiceServer *server = spice_server_new();
+    int sv[2];
 
     g_assert_nonnull(server);
 
@@ -64,6 +66,16 @@ static void server_leaks(void)
     result = spice_server_set_channel_security(server, "main", SPICE_CHANNEL_SECURITY_SSL);
     g_assert_cmpint(result, ==, 0);
 
+    /* spice_server_add_ssl_client should not leak when it's given a disconnected socket */
+    g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,
+                          "*SSL_accept failed*");
+    g_assert_cmpint(socketpair(AF_LOCAL, SOCK_STREAM, 0, sv), ==, 0);
+    close(sv[1]);
+    result = spice_server_add_ssl_client(server, sv[0], 1);
+    g_assert_cmpint(result, ==, -1);
+    /* if the function fails, it should not close the socket */
+    g_assert_cmpint(close(sv[0]), ==, 0);
+
     spice_server_destroy(server);
     basic_event_loop_destroy();
 }