[Spice-devel] [vdagent-linux] udscs: Fix memory ownership issues with udscs_read_callback

[Christophe Fergeau]

Before this commit, udscs_read_callback is supposed to free the 'data'
buffer unless it calls udscs_destroy_connection(). It's currently not
doing this, causing a potential double free in error cases. The udscs
code would also leak the 'data' buffer if no udscs_read_callback is set.

This commit moves ownership of the 'data' buffer to the generic code
calling the udscs_read_callback and fixes the memory management of this
'data' buffer. As the only udscs_read_callback currently implemented is
not keeping pointers to 'data' after it returns, this should not cause
any issues.
---
 src/udscs.c             | 2 ++
 src/udscs.h             | 7 ++++---
 src/vdagentd/vdagentd.c | 4 ----
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/udscs.c b/src/udscs.c
index 427a844..b468e71 100644
--- a/src/udscs.c
+++ b/src/udscs.c
@@ -132,6 +132,7 @@ void udscs_destroy_connection(struct udscs_connection **connp)
     }
 
     free(conn->data.buf);
+    conn->data.buf = NULL;
 
     if (conn->next)
         conn->next->prev = conn->prev;
@@ -235,6 +236,7 @@ static void udscs_read_complete(struct udscs_connection **connp)
         if (!*connp) /* Was the connection disconnected by the callback ? */
             return;
     }
+    free(conn->data.buf);
 
     conn->header_read = 0;
     memset(&conn->data, 0, sizeof(conn->data));
diff --git a/src/udscs.h b/src/udscs.h
index d65630d..6e960f8 100644
--- a/src/udscs.h
+++ b/src/udscs.h
@@ -39,9 +39,10 @@ struct udscs_message_header {
 };
 
 /* Callbacks with this type will be called when a complete message has been
- * received. The callback is responsible for free()-ing the data buffer. It
- * may call udscs_destroy_connection, in which case *connp must be made NULL
- * (which udscs_destroy_connection takes care of).
+ * received. The callback does not own the data buffer and should not free it.
+ * The data buffer will be freed shortly after the read callback returns.
+ * The callback may call udscs_destroy_connection, in which case *connp must be
+ * made NULL (which udscs_destroy_connection takes care of).
  */
 typedef void (*udscs_read_callback)(struct udscs_connection **connp,
     struct udscs_message_header *header, uint8_t *data);
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 18e82a7..a1faf23 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -727,7 +727,6 @@ static void agent_read_complete(struct udscs_connection **connp,
         if (header->arg1 == 0 && header->arg2 == 0) {
             syslog(LOG_INFO, "got old session agent xorg resolution message, "
                              "ignoring");
-            free(data);
             return;
         }
 
@@ -735,7 +734,6 @@ static void agent_read_complete(struct udscs_connection **connp,
             syslog(LOG_ERR, "guest xorg resolution message has wrong size, "
                             "disconnecting agent");
             udscs_destroy_connection(connp);
-            free(data);
             return;
         }
 
@@ -760,7 +758,6 @@ static void agent_read_complete(struct udscs_connection **connp,
     case VDAGENTD_CLIPBOARD_RELEASE:
         if (do_agent_clipboard(*connp, header, data)) {
             udscs_destroy_connection(connp);
-            free(data);
             return;
         }
         break;
@@ -783,7 +780,6 @@ static void agent_read_complete(struct udscs_connection **connp,
         syslog(LOG_ERR, "unknown message from vdagent: %u, ignoring",
                header->type);
     }
-    free(data);
 }
 
 /* main */
-- 
2.9.3