From mboxrd@z Thu Jan 1 00:00:00 1970 From: jonathan.rajotte-julien@efficios.com (Jonathan Rajotte) Date: Thu, 28 Sep 2017 16:37:21 -0400 Subject: [lttng-dev] [PATCH lttng-tools 1/2] Fix: scope ownership of a stream for ust-consumer Message-ID: <20170928203722.29150-1-jonathan.rajotte-julien@efficios.com> 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 Signed-off-by: Jonathan Rajotte --- 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