[lttng-tools] Fix: lttng-consumerd crash due to double free metadata stream
diff mbox series

Message ID 1506506279-23472-1-git-send-email-liguang.li@windriver.com
State New
Delegated to: Jérémie Galarneau
Headers show
Series
  • [lttng-tools] Fix: lttng-consumerd crash due to double free metadata stream
Related show

Commit Message

Liguang Li Sept. 27, 2017, 9:57 a.m. UTC
When setup metadata failed, the exception handling function will free
the metadata stream twice.

Signed-off-by: Liguang Li <liguang.li at windriver.com>
---
 src/common/ust-consumer/ust-consumer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Jonathan Rajotte Sept. 28, 2017, 8:57 p.m. UTC | #1
Hi Liguang,

Thanks for submitting the patch.

On Wed, Sep 27, 2017 at 05:57:59PM +0800, Liguang Li wrote:
> When setup metadata failed, the exception handling function will free
> the metadata stream twice.

This is lacking context and a good explanation of the problem at hand.
Providing a good reproducer is a good first step to allow easier review of
patches. This also remove uncertainty on the origin of the issue at the first
place.

In this case, I'll assume that the issue happen when the call to
lttng_pipe_write inside send_stream_to_thread fails. I would be even more
interested on the conditions(scenario) required for this call to fail in the
first place. It would be nice if you could provide more information regarding
that.

Still, faking a failure on lttng_pipe_write when a metadata stream is to be sent
allows me to reproduce an issue similar to the one you are describing.

> 
> Signed-off-by: Liguang Li <liguang.li at windriver.com>
> ---
>  src/common/ust-consumer/ust-consumer.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
> index 366f855..3836ef9 100644
> --- a/src/common/ust-consumer/ust-consumer.c
> +++ b/src/common/ust-consumer/ust-consumer.c
> @@ -234,6 +234,8 @@ static int send_stream_to_thread(struct lttng_consumer_stream *stream,
>  		ERR("Consumer write %s stream to pipe %d",
>  				stream->metadata_flag ? "metadata" : "data",
>  				lttng_pipe_get_writefd(stream_pipe));
> +		/* Remove node from the channel stream list. */
> +		cds_list_del(&stream->send_node);
>  		if (stream->metadata_flag) {
>  			consumer_del_stream_for_metadata(stream);
>  		} else {
> @@ -721,8 +723,6 @@ 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;
>  		}
>  
> @@ -925,7 +925,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 end;
>  	}
>  	/* List MUST be empty after or else it could be reused. */
>  	assert(cds_list_empty(&metadata->streams.head));
> @@ -940,8 +940,14 @@ error:
>  	 * the stream is still in the local stream list of the channel. This call
>  	 * will make sure to clean that list.
>  	 */
> -	consumer_stream_destroy(metadata->metadata_stream, NULL);
>  	cds_list_del(&metadata->metadata_stream->send_node);
> +
> +	/* Destroy tracer buffers of the stream. */
> +	consumer_stream_destroy_buffers(metadata->metadata_stream);
> +	/* Close down everything including the relayd if one. */
> +	consumer_stream_close(metadata->metadata_stream);

Those two calls are already performed inside destroy_close_stream() which is
always called inside consumer_stream_destroy(). No need to perform
them.

You can find a reworked version of this fix here [1]. Could you validate that it
does indeed fixes the issue?

[1] https://lists.lttng.org/pipermail/lttng-dev/2017-September/027488.html

> +
> +	consumer_stream_destroy(metadata->metadata_stream, NULL);
>  	metadata->metadata_stream = NULL;
>  error_no_stream:
>  end:
> -- 
> 2.7.4
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Jonathan Rajotte Oct. 11, 2017, 6:39 p.m. UTC | #2
Hi,

On Thu, Sep 28, 2017 at 04:57:42PM -0400, Jonathan Rajotte-Julien wrote:
> Hi Liguang,
> 
> Thanks for submitting the patch.
> 
> On Wed, Sep 27, 2017 at 05:57:59PM +0800, Liguang Li wrote:
> > When setup metadata failed, the exception handling function will free
> > the metadata stream twice.
> 
> This is lacking context and a good explanation of the problem at hand.
> Providing a good reproducer is a good first step to allow easier review of
> patches. This also remove uncertainty on the origin of the issue at the first
> place.
> 
> In this case, I'll assume that the issue happen when the call to
> lttng_pipe_write inside send_stream_to_thread fails. I would be even more
> interested on the conditions(scenario) required for this call to fail in the
> first place. It would be nice if you could provide more information regarding
> that.
> 
> Still, faking a failure on lttng_pipe_write when a metadata stream is to be sent
> allows me to reproduce an issue similar to the one you are describing.
> 
> Those two calls are already performed inside destroy_close_stream() which is
> always called inside consumer_stream_destroy(). No need to perform
> them.
> 
> You can find a reworked version of this fix here [1]. Could you validate that it
> does indeed fixes the issue?
> 
> [1] https://lists.lttng.org/pipermail/lttng-dev/2017-September/027488.html

Any feedback regarding the provided patch?

Cheers

Patch
diff mbox series

diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index 366f855..3836ef9 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -234,6 +234,8 @@  static int send_stream_to_thread(struct lttng_consumer_stream *stream,
 		ERR("Consumer write %s stream to pipe %d",
 				stream->metadata_flag ? "metadata" : "data",
 				lttng_pipe_get_writefd(stream_pipe));
+		/* Remove node from the channel stream list. */
+		cds_list_del(&stream->send_node);
 		if (stream->metadata_flag) {
 			consumer_del_stream_for_metadata(stream);
 		} else {
@@ -721,8 +723,6 @@  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;
 		}
 
@@ -925,7 +925,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 end;
 	}
 	/* List MUST be empty after or else it could be reused. */
 	assert(cds_list_empty(&metadata->streams.head));
@@ -940,8 +940,14 @@  error:
 	 * the stream is still in the local stream list of the channel. This call
 	 * will make sure to clean that list.
 	 */
-	consumer_stream_destroy(metadata->metadata_stream, NULL);
 	cds_list_del(&metadata->metadata_stream->send_node);
+
+	/* Destroy tracer buffers of the stream. */
+	consumer_stream_destroy_buffers(metadata->metadata_stream);
+	/* Close down everything including the relayd if one. */
+	consumer_stream_close(metadata->metadata_stream);
+
+	consumer_stream_destroy(metadata->metadata_stream, NULL);
 	metadata->metadata_stream = NULL;
 error_no_stream:
 end: