[Spice-devel] [PATCH 2/3] server, separate SpiceChannelEventInfo from RedStream

[Alon Levy]

fixes rhbz 790749 use after free of SpiceChannelEventInfo.

The lifetime of the SpiceChannelEventInfo was that of RedsStream, but it
is used by main_dispatcher_handle_channel_event after the RedsStream is
freed for the cursor and display channels. Making SCEI allocation be at
RedsStream allocation, and deallocation after the DESTROY event is
processed by core->channel_event, fixes use after free.
---
 server/main_dispatcher.c |    3 +++
 server/reds.c            |   43 ++++++++++++++++++++++---------------------
 server/reds.h            |    6 +++++-
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index cf44b40..f5b8b4c 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -52,6 +52,9 @@ static void main_dispatcher_self_handle_channel_event(
                                                 SpiceChannelEventInfo *info)
 {
     main_dispatcher.core->channel_event(event, info);
+    if (event == SPICE_CHANNEL_EVENT_DISCONNECTED) {
+        free(info);
+    }
 }
 
 static void main_dispatcher_handle_channel_event(void *opaque,
diff --git a/server/reds.c b/server/reds.c
index ab0dfaf..daadb56 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -330,7 +330,7 @@ static void reds_stream_channel_event(RedsStream *s, int event)
 {
     if (core->base.minor_version < 3 || core->channel_event == NULL)
         return;
-    main_dispatcher_channel_event(event, &s->info);
+    main_dispatcher_channel_event(event, s->info);
 }
 
 static ssize_t stream_write_cb(RedsStream *s, const void *buf, size_t size)
@@ -1487,11 +1487,11 @@ static void reds_show_new_channel(RedLinkInfo *link, int connection_id)
                link->stream->ssl == NULL ? "Non Secure" : "Secure");
     /* add info + send event */
     if (link->stream->ssl) {
-        link->stream->info.flags |= SPICE_CHANNEL_EVENT_FLAG_TLS;
+        link->stream->info->flags |= SPICE_CHANNEL_EVENT_FLAG_TLS;
     }
-    link->stream->info.connection_id = connection_id;
-    link->stream->info.type = link->link_mess->channel_type;
-    link->stream->info.id   = link->link_mess->channel_id;
+    link->stream->info->connection_id = connection_id;
+    link->stream->info->type = link->link_mess->channel_type;
+    link->stream->info->id   = link->link_mess->channel_id;
     reds_stream_channel_event(link->stream, SPICE_CHANNEL_EVENT_INITIALIZED);
 }
 
@@ -2402,13 +2402,13 @@ static void reds_start_auth_sasl(RedLinkInfo *link)
     RedsSASL *sasl = &link->stream->sasl;
 
     /* Get local & remote client addresses in form  IPADDR;PORT */
-    if (!(localAddr = addr_to_string("%s;%s", &link->stream->info.laddr_ext,
-                                              link->stream->info.llen_ext))) {
+    if (!(localAddr = addr_to_string("%s;%s", &link->stream->info->laddr_ext,
+                                              link->stream->info->llen_ext))) {
         goto error;
     }
 
-    if (!(remoteAddr = addr_to_string("%s;%s", &link->stream->info.paddr_ext,
-                                               link->stream->info.plen_ext))) {
+    if (!(remoteAddr = addr_to_string("%s;%s", &link->stream->info->paddr_ext,
+                                               link->stream->info->plen_ext))) {
         free(localAddr);
         goto error;
     }
@@ -2715,24 +2715,25 @@ static RedLinkInfo *reds_init_client_connection(int socket)
 
     link = spice_new0(RedLinkInfo, 1);
     stream = spice_new0(RedsStream, 1);
+    stream->info = spice_new0(SpiceChannelEventInfo, 1);
     link->stream = stream;
 
     stream->socket = socket;
     /* gather info + send event */
 
     /* deprecated fields. Filling them for backward compatibility */
-    stream->info.llen = sizeof(stream->info.laddr);
-    stream->info.plen = sizeof(stream->info.paddr);
-    getsockname(stream->socket, (struct sockaddr*)(&stream->info.laddr), &stream->info.llen);
-    getpeername(stream->socket, (struct sockaddr*)(&stream->info.paddr), &stream->info.plen);
-
-    stream->info.flags |= SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT;
-    stream->info.llen_ext = sizeof(stream->info.laddr_ext);
-    stream->info.plen_ext = sizeof(stream->info.paddr_ext);
-    getsockname(stream->socket, (struct sockaddr*)(&stream->info.laddr_ext),
-                &stream->info.llen_ext);
-    getpeername(stream->socket, (struct sockaddr*)(&stream->info.paddr_ext),
-                &stream->info.plen_ext);
+    stream->info->llen = sizeof(stream->info->laddr);
+    stream->info->plen = sizeof(stream->info->paddr);
+    getsockname(stream->socket, (struct sockaddr*)(&stream->info->laddr), &stream->info->llen);
+    getpeername(stream->socket, (struct sockaddr*)(&stream->info->paddr), &stream->info->plen);
+
+    stream->info->flags |= SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT;
+    stream->info->llen_ext = sizeof(stream->info->laddr_ext);
+    stream->info->plen_ext = sizeof(stream->info->paddr_ext);
+    getsockname(stream->socket, (struct sockaddr*)(&stream->info->laddr_ext),
+                &stream->info->llen_ext);
+    getpeername(stream->socket, (struct sockaddr*)(&stream->info->paddr_ext),
+                &stream->info->plen_ext);
 
     reds_stream_channel_event(stream, SPICE_CHANNEL_EVENT_CONNECTED);
 
diff --git a/server/reds.h b/server/reds.h
index 2a4e351..1fd18a7 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -77,7 +77,11 @@ struct RedsStream {
     RedsSASL sasl;
 #endif
 
-    SpiceChannelEventInfo info;
+    /* life time of info:
+     * allocated when creating RedsStream.
+     * deallocated when main_dispatcher handles the SPICE_CHANNEL_EVENT_DISCONNECTED
+     * event, either from same thread or by call back from main thread. */
+    SpiceChannelEventInfo* info;
 
     /* private */
     ssize_t (*read)(RedsStream *s, void *buf, size_t nbyte);
-- 
1.7.9