[lttng-dev] [PATCH lttng-tools] Fix: lttng-consumerd crash due to double free metadata stream

[jonathan.rajotte-julien@efficios.com (Jonathan Rajotte-Julien)]

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