From cd2ef1ef1d54ced9e4d0d03b865bb7fc6a905f80 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 2 Apr 2014 10:31:34 -0400 Subject: [PATCH] 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 --- src/bin/lttng-relayd/connection.h | 7 +++++++ src/bin/lttng-relayd/main.c | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/bin/lttng-relayd/connection.h b/src/bin/lttng-relayd/connection.h index eeb1d545b..fc4a59075 100644 --- 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 a93151ac4..d82a3412d 100644 --- 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); -- 2.34.1