| 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 | expand |
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
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
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:
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(-)