Fix: consumerd: use-after-free of metadata bucket

Observed issue
==============

When consumer_stream_destroy() is called from, for example, the error
path in setup_metadata(), consumer_stream_free() can end up being called
twice on the same stream.  Since the stream->metadata_bucket is not set
to NULL after being destroyed, it leads to a use-after-free:

 ERROR: AddressSanitizer: heap-use-after-free on address 0x604000000318
 READ of size 8 at 0x604000000318 thread T7
     #0 in metadata_bucket_destroy
     #1 in consumer_stream_free
     #2 in consumer_stream_destroy
     #3 in setup_metadata
     #4 in lttng_ustconsumer_recv_cmd
     lttng#5 in lttng_consumer_recv_cmd
     lttng#6 in consumer_thread_sessiond_poll
     lttng#7 in start_thread nptl/pthread_create.c:481
     lttng#8 in clone (/lib/x86_64-linux-gnu/libc.so.6+0xfcbde)

 0x604000000318 is located 8 bytes inside of 48-byte region [0x604000000310,0x604000000340)
 freed by thread T7 here:
     #0 in __interceptor_free
     #1 in metadata_bucket_destroy
     #2 in consumer_stream_free
     #3 in consumer_stream_destroy
     #4 in clean_channel_stream_list
     lttng#5 in consumer_del_channel
     lttng#6 in consumer_stream_destroy
     lttng#7 in setup_metadata
     lttng#8 in lttng_ustconsumer_recv_cmd
     lttng#9 in lttng_consumer_recv_cmd
     lttng#10 in consumer_thread_sessiond_poll
     lttng#11 in start_thread nptl/pthread_create.c:481

 previously allocated by thread T7 here:
     #0 in __interceptor_calloc
     #1 in zmalloc
     #2 in metadata_bucket_create
     #3 in consumer_stream_enable_metadata_bucketization
     #4 in lttng_ustconsumer_set_stream_ops
     lttng#5 in lttng_ustconsumer_on_recv_stream
     lttng#6 in lttng_consumer_on_recv_stream
     lttng#7 in create_ust_streams
     lttng#8 in ask_channel
     lttng#9 in lttng_ustconsumer_recv_cmd
     lttng#10 in lttng_consumer_recv_cmd
     lttng#11 in consumer_thread_sessiond_poll
     lttng#12 in start_thread nptl/pthread_create.c:481

 Thread T7 created by T0 here:
     #0 in __interceptor_pthread_create
     #1 in main
     #2 in __libc_start_main ../csu/libc-start.c:332

 SUMMARY: AddressSanitizer: heap-use-after-free in metadata_bucket_destroy

This can be easily reproduced by forcing a failure during the setup
of the metadata reproducible using the following change:

  diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
  index fa1c71299..97ed59632 100644

  --- a/src/common/ust-consumer/ust-consumer.c
  +++ b/src/common/ust-consumer/ust-consumer.c
  @@ -908,8 +908,7 @@ static int setup_metadata(struct lttng_consumer_local_data *ctx, uint64_t key)

           /* Send metadata stream to relayd if needed. */
           if (metadata->metadata_stream->net_seq_idx != (uint64_t) -1ULL) {
  -                ret = consumer_send_relayd_stream(metadata->metadata_stream,
  -                                metadata->pathname);
  +                ret = -1;
                   if (ret < 0) {
                           ret = LTTCOMM_CONSUMERD_ERROR_METADATA;
                           goto error;

Cause
=====

Channels have a list of streams that are being "setup" and are not
yet monitored for consumption. During this setup phase, the streams are
owned by the channel. On destruction of the channel, any stream in that
list will thus be cleaned-up.

When destroying a consumer stream, a reference to its channel is 'put'.
This can result in the destruction of the channel.

In the situation described above, the release of the channel's reference
is done before the stream is removed from the channel's stream list.
This causes the channel's clean-up to invoke (again) the current
stream's clean-up, resulting in the double-free of the metadata bucket.

This problem is present in a number of error paths.

Solution
========

Some error paths already manually removed the consumer stream from it's
channel's stream list before invoking consumer_stream_destroy(). The
various error paths that have to deal with this possible situation are
changed to simply invoke consumer_stream_destroy().

consumer_stream_destroy() is modified to always remove the stream from
its channel's list before performing the rest of the clean-up. This
ensures that those double clean-ups can't occur.

Drawbacks
=========

None.

Reported-by: Vincent Whitchurch <[email protected]>
Tested-by: Vincent Whitchurch <[email protected]>
Signed-off-by: Jérémie Galarneau <[email protected]>
Change-Id: Ibeca9b675b86fc46be3f57826f7158de4da43df8

src/common/consumer/consumer-stream.cpp

@@ -669,6 +669,7 @@ struct lttng_consumer_stream *consumer_stream_create(
 		goto error;
 	}
 
+	stream->send_node = CDS_LIST_HEAD_INIT(stream->send_node);
 	stream->chan = channel;
 	stream->key = stream_key;
 	stream->trace_chunk = trace_chunk;
@@ -1060,6 +1061,8 @@ void consumer_stream_destroy(struct lttng_consumer_stream *stream,
 {
 	LTTNG_ASSERT(stream);
 
+	cds_list_del_init(&stream->send_node);
+
 	/* Stream is in monitor mode. */
 	if (stream->monitor) {
 		struct lttng_consumer_channel *free_chan = NULL;

src/common/consumer/consumer.cpp

@@ -174,7 +174,6 @@ static void clean_channel_stream_list(struct lttng_consumer_channel *channel)
 	/* Delete streams that might have been left in the stream list. */
 	cds_list_for_each_entry_safe(stream, stmp, &channel->streams.head,
 			send_node) {
-		cds_list_del(&stream->send_node);
 		/*
 		 * Once a stream is added to this list, the buffers were created so we
 		 * have a guarantee that this call will succeed. Setting the monitor

src/common/consumer/consumer.h

@@ -423,6 +423,11 @@ struct lttng_consumer_stream {
 	struct lttng_ht_node_u64 node_channel_id;
 	/* HT node used in consumer_data.stream_list_ht */
 	struct lttng_ht_node_u64 node_session_id;
+	/*
+	 * List used by channels to reference streams that are not yet globally
+	 * visible.
+	 */
+	struct cds_list_head send_node;
 	/* Pointer to associated channel. */
 	struct lttng_consumer_channel *chan;
 	/*
@@ -555,7 +560,6 @@ struct lttng_consumer_stream {
 	char name[LTTNG_SYMBOL_NAME_LEN];
 	/* Internal state of libustctl. */
 	struct lttng_ust_ctl_consumer_stream *ustream;
-	struct cds_list_head send_node;
 	/* On-disk circular buffer */
 	uint64_t tracefile_size_current;
 	uint64_t tracefile_count_current;

src/common/kernel-consumer/kernel-consumer.cpp

@@ -435,7 +435,6 @@ static int lttng_kconsumer_snapshot_metadata(
 	ret = 0;
 error_snapshot:
 	metadata_stream->read_subbuffer_ops.unlock(metadata_stream);
-	cds_list_del(&metadata_stream->send_node);
 	consumer_stream_destroy(metadata_stream, NULL);
 	metadata_channel->metadata_stream = NULL;
 	rcu_read_unlock();

src/common/ust-consumer/ust-consumer.cpp

@@ -67,7 +67,7 @@ static void destroy_channel(struct lttng_consumer_channel *channel)
 
 		health_code_update();
 
-		cds_list_del(&stream->send_node);
+		cds_list_del_init(&stream->send_node);
 		lttng_ust_ctl_destroy_stream(stream->ustream);
 		lttng_trace_chunk_put(stream->trace_chunk);
 		free(stream);
@@ -203,7 +203,7 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream,
 	 * global.
 	 */
 	stream->globally_visible = 1;
-	cds_list_del(&stream->send_node);
+	cds_list_del_init(&stream->send_node);
 
 	ret = lttng_pipe_write(stream_pipe, &stream, sizeof(stream));
 	if (ret < 0) {
@@ -950,7 +950,6 @@ static int setup_metadata(struct lttng_consumer_local_data *ctx, uint64_t key)
 	 * will make sure to clean that list.
 	 */
 	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:
@@ -1034,7 +1033,6 @@ static int snapshot_metadata(struct lttng_consumer_channel *metadata_channel,
 	 * new metadata stream.
 	 */
 	consumer_stream_destroy(metadata_stream, NULL);
-	cds_list_del(&metadata_stream->send_node);
 	metadata_channel->metadata_stream = NULL;
 
 error: