[lttng-dev] [PATCH lttng-tools 1/2] Fix: scope ownership of a stream for ust-consumer

[lttng-dev] [PATCH lttng-tools 1/2] Fix: scope ownership of a stream for ust-consumer

[Jonathan Rajotte]

A failure on lttng_pipe write during send_stream_to_thread() lead to
a null-pointer dereference of the stream handle during
consumer_del_channel(). The chain of events:

 - Failure during lttng_pipe_write() inside send_stream_to_thread().

 - Call to consumer_stream_destroy() via consumer_del_stream_for_data()
   or consumer_del_stream_for_metadata().

 - The stream is monitor and globally visible at this point leading to
   performing a call to destroy_close_stream() which perform the first
   cleanup of the stream.

   Note: At this point the stream is still in the channel local stream
         list (stream.send_node).

 - The call to unref_channel() returns a reference to a channel for which
   a cleanup call must be done.

 - The cleanup call for the channel is performed using
   consumer_del_channel().

 - At this point the stream is still in the channel local stream list.
   This result in a second call to consumer_stream_destroy() via
   clean_channel_stream_list(). Which, itself, results in accesses to
   freed memory.

The fix:

 - Use cds_list_del() inside send_stream_to_thread() after public
   exposition of the stream to ensure that the stream ownership/visibility
   is clear. A stream cannot be globally visible and local
   (stream.send_node) to a channel at the same time.
 - Modify error paths to acknowledge the ownership transfer to
   send_stream_to_thread().

Reported-by: Liguang Li <liguang.li at windriver.com>
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/common/ust-consumer/ust-consumer.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index 366f8550..ab240f7d 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -225,9 +225,12 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream,
 
 	/*
 	 * From this point on, the stream's ownership has been moved away from
-	 * the channel and becomes globally visible.
+	 * the channel and becomes globally visible. Hence, remove it from the
+	 * local stream list to prevent scenario where the stream is local and
+	 * global.
 	 */
 	stream->globally_visible = 1;
+	cds_list_del(&stream->send_node);
 
 	ret = lttng_pipe_write(stream_pipe, &stream, sizeof(stream));
 	if (ret < 0) {
@@ -239,7 +242,9 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream,
 		} else {
 			consumer_del_stream_for_data(stream);
 		}
+		goto error;
 	}
+
 error:
 	return ret;
 }
@@ -721,14 +726,8 @@ static int send_streams_to_thread(struct lttng_consumer_channel *channel,
 			 * If we are unable to send the stream to the thread, there is
 			 * a big problem so just stop everything.
 			 */
-			/* Remove node from the channel stream list. */
-			cds_list_del(&stream->send_node);
 			goto error;
 		}
-
-		/* Remove node from the channel stream list. */
-		cds_list_del(&stream->send_node);
-
 	}
 
 error:
@@ -918,6 +917,10 @@ static int setup_metadata(struct lttng_consumer_local_data *ctx, uint64_t key)
 		}
 	}
 
+	/*
+	 * Ownership of metadata stream is passed along. Freeing is handled by
+	 * the callee.
+	 */
 	ret = send_streams_to_thread(metadata, ctx);
 	if (ret < 0) {
 		/*
@@ -925,7 +928,7 @@ static int setup_metadata(struct lttng_consumer_local_data *ctx, uint64_t key)
 		 * a big problem so just stop everything.
 		 */
 		ret = LTTCOMM_CONSUMERD_FATAL;
-		goto error;
+		goto send_streams_error;
 	}
 	/* List MUST be empty after or else it could be reused. */
 	assert(cds_list_empty(&metadata->streams.head));
@@ -943,6 +946,7 @@ error:
 	consumer_stream_destroy(metadata->metadata_stream, NULL);
 	cds_list_del(&metadata->metadata_stream->send_node);
 	metadata->metadata_stream = NULL;
+send_streams_error:
 error_no_stream:
 end:
 	return ret;
-- 
2.11.0

[lttng-dev] [PATCH lttng-tools 2/2] Cleanup: consumer_add_metadata_stream always return 0

[Jonathan Rajotte]

Since c869f647b0c4476645ab9ee01e362401fb8c1e42, the return value of
consumer_add_metadata_stream is always zero. Hence, we can remove some
dead error handling code.

consumer_add_metadata_stream success is based on assert() check for
relevant data.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/common/consumer/consumer.c               |  9 ++-------
 src/common/consumer/consumer.h               |  4 ++--
 src/common/kernel-consumer/kernel-consumer.c | 18 +++---------------
 src/common/ust-consumer/ust-consumer.c       | 14 ++------------
 4 files changed, 9 insertions(+), 36 deletions(-)

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index a8c36990..96ad8512 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -630,10 +630,9 @@ end:
 /*
  * Add a stream to the global list protected by a mutex.
  */
-int consumer_add_data_stream(struct lttng_consumer_stream *stream)
+void consumer_add_data_stream(struct lttng_consumer_stream *stream)
 {
 	struct lttng_ht *ht = data_ht;
-	int ret = 0;
 
 	assert(stream);
 	assert(ht);
@@ -683,8 +682,6 @@ int consumer_add_data_stream(struct lttng_consumer_stream *stream)
 	pthread_mutex_unlock(&stream->chan->timer_lock);
 	pthread_mutex_unlock(&stream->chan->lock);
 	pthread_mutex_unlock(&consumer_data.lock);
-
-	return ret;
 }
 
 void consumer_del_data_stream(struct lttng_consumer_stream *stream)
@@ -2106,10 +2103,9 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
  * Action done with the metadata stream when adding it to the consumer internal
  * data structures to handle it.
  */
-int consumer_add_metadata_stream(struct lttng_consumer_stream *stream)
+void consumer_add_metadata_stream(struct lttng_consumer_stream *stream)
 {
 	struct lttng_ht *ht = metadata_ht;
-	int ret = 0;
 	struct lttng_ht_iter iter;
 	struct lttng_ht_node_u64 *node;
 
@@ -2169,7 +2165,6 @@ int consumer_add_metadata_stream(struct lttng_consumer_stream *stream)
 	pthread_mutex_unlock(&stream->chan->lock);
 	pthread_mutex_unlock(&stream->chan->timer_lock);
 	pthread_mutex_unlock(&consumer_data.lock);
-	return ret;
 }
 
 /*
diff --git a/src/common/consumer/consumer.h b/src/common/consumer/consumer.h
index d115a597..eb9b8c46 100644
--- a/src/common/consumer/consumer.h
+++ b/src/common/consumer/consumer.h
@@ -754,9 +754,9 @@ void consumer_destroy_relayd(struct consumer_relayd_sock_pair *relayd);
 unsigned long consumer_get_consume_start_pos(unsigned long consumed_pos,
 		unsigned long produced_pos, uint64_t nb_packets_per_stream,
 		uint64_t max_sb_size);
-int consumer_add_data_stream(struct lttng_consumer_stream *stream);
+void consumer_add_data_stream(struct lttng_consumer_stream *stream);
 void consumer_del_stream_for_data(struct lttng_consumer_stream *stream);
-int consumer_add_metadata_stream(struct lttng_consumer_stream *stream);
+void consumer_add_metadata_stream(struct lttng_consumer_stream *stream);
 void consumer_del_stream_for_metadata(struct lttng_consumer_stream *stream);
 int consumer_create_index_file(struct lttng_consumer_stream *stream);
 
diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 7bcb86a3..b761a879 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -748,26 +748,14 @@ int lttng_kconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 
 		/* Get the right pipe where the stream will be sent. */
 		if (new_stream->metadata_flag) {
-			ret = consumer_add_metadata_stream(new_stream);
-			if (ret) {
-				ERR("Consumer add metadata stream %" PRIu64 " failed. Continuing",
-						new_stream->key);
-				consumer_stream_free(new_stream);
-				goto end_nosignal;
-			}
+			consumer_add_metadata_stream(new_stream);
 			stream_pipe = ctx->consumer_metadata_pipe;
 		} else {
-			ret = consumer_add_data_stream(new_stream);
-			if (ret) {
-				ERR("Consumer add stream %" PRIu64 " failed. Continuing",
-						new_stream->key);
-				consumer_stream_free(new_stream);
-				goto end_nosignal;
-			}
+			consumer_add_data_stream(new_stream);
 			stream_pipe = ctx->consumer_data_pipe;
 		}
 
-		/* Vitible to other threads */
+		/* Visible to other threads */
 		new_stream->globally_visible = 1;
 
 		health_code_update();
diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index ab240f7d..bd8e2c50 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -206,20 +206,10 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream,
 
 	/* Get the right pipe where the stream will be sent. */
 	if (stream->metadata_flag) {
-		ret = consumer_add_metadata_stream(stream);
-		if (ret) {
-			ERR("Consumer add metadata stream %" PRIu64 " failed.",
-					stream->key);
-			goto error;
-		}
+		consumer_add_metadata_stream(stream);
 		stream_pipe = ctx->consumer_metadata_pipe;
 	} else {
-		ret = consumer_add_data_stream(stream);
-		if (ret) {
-			ERR("Consumer add stream %" PRIu64 " failed.",
-					stream->key);
-			goto error;
-		}
+		consumer_add_data_stream(stream);
 		stream_pipe = ctx->consumer_data_pipe;
 	}
 
-- 
2.11.0