Fix: use after free of a relayd stream

A race could occur with a stream destruction and a control connection
being destroyed emptying its recv_list. A freed stream could still be in
the list thus having a use after free during the connection destroy.

That was triggering undefined behavior from infinite looping to
segmentation faults.

We've observed this issue on high load stress test. A relayd received
all the stream but NOT the streams sent command which empty the list.
This can happen if a start tracing never occured or failed on the
application side thus the close stream command is sent to the relayd
freeing the stream before it is removed from that list.

Signed-off-by: David Goulet <dgoulet@efficios.com>

diff --git a/src/bin/lttng-relayd/connection.h b/src/bin/lttng-relayd/connection.h
index eeb1d545bb7ac279b81ef11798666bee2689c377..fc4a59075f3dabd4a88eab06f8eef06b58af53d2 100644 (file)
--- a/src/bin/lttng-relayd/connection.h
+++ b/src/bin/lttng-relayd/connection.h
@@ -54,6 +54,13 @@ struct relay_connection {
        uint32_t major;
        uint32_t minor;
        uint64_t session_id;
+
+       /*
+        * This contains streams that are received on that connection. It's used to
+        * store them until we get the streams sent command where they are removed
+        * and flagged ready for the viewer. This is ONLY used by the control
+        * thread thus any action on it should happen in that thread.
+        */
        struct cds_list_head recv_head;
        unsigned int version_check_done:1;
 

diff --git a/src/bin/lttng-relayd/main.c b/src/bin/lttng-relayd/main.c
index a93151ac47f560875f07c9cbf113225f9dd892cc..d82a3412d7b8578be8f5b39abf0a9fd949ff07bc 100644 (file)
--- a/src/bin/lttng-relayd/main.c
+++ b/src/bin/lttng-relayd/main.c
@@ -1340,6 +1340,18 @@ int relay_close_stream(struct lttcomm_relayd_hdr *recv_hdr,
        session->stream_count--;
        assert(session->stream_count >= 0);
 
+       /*
+        * Remove the stream from the connection recv list since we are about to
+        * flag it invalid and thus might be freed. This has to be done here since
+        * only the control thread can do actions on that list.
+        *
+        * Note that this stream might NOT be in the list but we have to try to
+        * remove it here else this can race with the stream destruction freeing
+        * the object and the connection destroy doing a use after free when
+        * deleting the remaining nodes in this list.
+        */
+       cds_list_del(&stream->recv_list);
+
        /* Check if we can close it or else the data will do it. */
        try_close_stream(session, stream);