diff mbox series

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

Message ID 20170928203722.29150-1-jonathan.rajotte-julien@efficios.com
State Accepted, archived
Delegated to: Jérémie Galarneau
Headers show
Series [lttng-tools,1/2] Fix: scope ownership of a stream for ust-consumer | expand

Commit Message

Jonathan Rajotte Sept. 28, 2017, 8:37 p.m. UTC
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(-)

Comments

Jérémie Galarneau Dec. 3, 2017, 2:46 p.m. UTC | #1
Merged with minor grammar fixes to the comments and commit message. Read on.

Thanks!
Jérémie

On 28 September 2017 at 22:37, Jonathan Rajotte
<jonathan.rajotte-julien at efficios.com> wrote:
> A failure on lttng_pipe write during send_stream_to_thread() lead to

lttng_pipe -> lttng_pipe_write()

lead -> leads

> a null-pointer dereference of the stream handle during
> consumer_del_channel(). The chain of events:
>

The chain of events leading to the problem is:

>  - 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

perform -> performs

>    cleanup of the stream.
>
>    Note: At this point the stream is still in the channel local stream

channel -> channel's

>          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.

channel -> channel's

>    This result in a second call to consumer_stream_destroy() via

result -> results

>    clean_channel_stream_list(). Which, itself, results in accesses to
>    freed memory.
>
> The fix:

The fix consists in:

>
>  - Use cds_list_del() inside send_stream_to_thread() after public

Use -> Using

>    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

Modify -> Modifying

to -> through

>    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

to prevent the stream from being both local and global.

> +        * 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
>
diff mbox series

Patch

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;