[lttng-dev] [PATCH babeltrace-1.5 0/6] Babeltrace 1.5 lttng-live with per-pid ust buffers

[lttng-dev] [PATCH babeltrace-1.5 0/6] Babeltrace 1.5 lttng-live with per-pid ust buffers

[Mathieu Desnoyers]

This series of fixes comes from testing babeltrace 1.5 lttng-live
under valgrind with per-pid lttng buffers. This was done in the
context of testing of the "lttng clear" feature.

Thanks,

Mathieu

Mathieu Desnoyers (6):
  Fix: lttng-live: use-after-free in get_next_index()
  Fix: trace-collection: trace clock use after free
  Fix: lttng-live: lttng_live_open_trace_read memory leak
  Fix: lib/iterator.c: unbalanced ctx put (leak)
  Fix: lttng-live: ctf_live_packet_seek stream hang up handling
  Fix: lttng-live format: do not error out on empty streams hang up

 formats/ctf/ctf.c                      |  7 ++++++-
 formats/lttng-live/lttng-live-comm.c   |  8 ++++++--
 formats/lttng-live/lttng-live-plugin.c |  1 +
 lib/iterator.c                         |  3 +++
 lib/trace-collection.c                 | 21 ++++++++++++++++++---
 5 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 1/6] Fix: lttng-live: use-after-free in get_next_index()

[Mathieu Desnoyers]

Running babeltrace under valgrind with a test-cases doing per-pid
lttng tracing in live mode triggers this use-after-free in
get_next_index() when stream is hung up.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 formats/lttng-live/lttng-live-comm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
index 33a78029..96817f5e 100644
--- a/formats/lttng-live/lttng-live-comm.c
+++ b/formats/lttng-live/lttng-live-comm.c
@@ -1108,8 +1108,8 @@ retry:
 		viewer_stream->in_trace = 0;
 		bt_list_del(&viewer_stream->trace_stream_node);
 		bt_list_del(&viewer_stream->session_stream_node);
-		g_free(viewer_stream);
 		*stream_id = be64toh(rp->stream_id);
+		g_free(viewer_stream);
 		break;
 	case LTTNG_VIEWER_INDEX_ERR:
 		fprintf(stderr, "[error] get_next_index: error\n");
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 2/6] Fix: trace-collection: trace clock use after free

[Mathieu Desnoyers]

The trace collection should copy the trace clock object rather
than take a reference to the first trace's trace clock, because
it may be freed when the trace is removed (e.g. application going
away in per-pid live tracing).

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 lib/trace-collection.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/trace-collection.c b/lib/trace-collection.c
index 035d2dc2..8e4a1432 100644
--- a/lib/trace-collection.c
+++ b/lib/trace-collection.c
@@ -76,7 +76,7 @@ static void clock_add(gpointer key, gpointer value, gpointer user_data)
 {
 	struct clock_match *clock_match = user_data;
 	GHashTable *tc_clocks = clock_match->clocks;
-	struct ctf_clock *t_clock = value;
+	struct ctf_clock *t_clock = value, *clock_copy;
 	GQuark v;
 
 	if (t_clock->absolute)
@@ -104,9 +104,14 @@ static void clock_add(gpointer key, gpointer value, gpointer user_data)
 				clock_match->tc->single_clock_offset_avg =
 					clock_match->tc->offset_first;
 			}
+			clock_copy = g_new0(struct ctf_clock, 1);
+			*clock_copy = *t_clock;
+			if (t_clock->description) {
+				clock_copy->description = g_strdup(t_clock->description);
+			}
 			g_hash_table_insert(tc_clocks,
 				(gpointer) (unsigned long) v,
-				value);
+				clock_copy);
 		} else if (!t_clock->absolute) {
 			int64_t diff_ns;
 
@@ -209,11 +214,21 @@ int bt_trace_collection_remove(struct trace_collection *tc,
 
 }
 
+static
+void clock_free(gpointer data)
+{
+	struct ctf_clock *clock = data;
+
+	g_free(clock->description);
+	g_free(clock);
+}
+
 void bt_init_trace_collection(struct trace_collection *tc)
 {
 	assert(tc);
 	tc->array = g_ptr_array_new();
-	tc->clocks = g_hash_table_new(g_direct_hash, g_direct_equal);
+	tc->clocks = g_hash_table_new_full(g_direct_hash, g_direct_equal,
+			NULL, clock_free);
 	tc->single_clock_offset_avg = 0;
 	tc->offset_first = 0;
 	tc->delta_offset_first_sum = 0;
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 3/6] Fix: lttng-live: lttng_live_open_trace_read memory leak

[Mathieu Desnoyers]

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 formats/lttng-live/lttng-live-plugin.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/formats/lttng-live/lttng-live-plugin.c b/formats/lttng-live/lttng-live-plugin.c
index ed52a995..0bebdd89 100644
--- a/formats/lttng-live/lttng-live-plugin.c
+++ b/formats/lttng-live/lttng-live-plugin.c
@@ -288,6 +288,7 @@ static int lttng_live_open_trace_read(const char *path)
 	}
 
 end_free:
+	g_array_free(ctx->session_ids, TRUE);
 	g_hash_table_destroy(ctx->session->ctf_traces);
 	free_session_streams(ctx->session);
 	g_free(ctx->session);
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 4/6] Fix: lib/iterator.c: unbalanced ctx put (leak)

[Mathieu Desnoyers]

Missing context put in iterator init error path.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 lib/iterator.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/iterator.c b/lib/iterator.c
index 639a2d29..77093217 100644
--- a/lib/iterator.c
+++ b/lib/iterator.c
@@ -778,6 +778,8 @@ int bt_iter_init(struct bt_iter *iter,
 error:
 	bt_heap_free(iter->stream_heap);
 error_heap_init:
+	bt_context_put(ctx);
+	iter->ctx = NULL;
 	g_free(iter->stream_heap);
 	iter->stream_heap = NULL;
 error_ctx:
@@ -812,6 +814,7 @@ void bt_iter_fini(struct bt_iter *iter)
 	}
 	iter->ctx->current_iterator = NULL;
 	bt_context_put(iter->ctx);
+	iter->ctx = NULL;
 }
 
 void bt_iter_destroy(struct bt_iter *iter)
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 5/6] Fix: lttng-live: ctf_live_packet_seek stream hang up handling

[Mathieu Desnoyers]

When get_next_index sets the index position to EOF,
ctf_live_packet_seek() should in turn set the stream position to EOF
to propagate the hung up state.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 formats/lttng-live/lttng-live-comm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/formats/lttng-live/lttng-live-comm.c b/formats/lttng-live/lttng-live-comm.c
index 96817f5e..484c025d 100644
--- a/formats/lttng-live/lttng-live-comm.c
+++ b/formats/lttng-live/lttng-live-comm.c
@@ -1276,7 +1276,11 @@ retry:
 				cur_index->packet_size, cur_index->offset,
 				cur_index->content_size,
 				cur_index->ts_cycles.timestamp_end);
-
+		if (cur_index->offset == EOF) {
+			pos->offset = EOF;
+			ret = -BT_PACKET_SEEK_ERROR;
+			goto end;
+		}
 	}
 
 	/*
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 6/6] Fix: lttng-live format: do not error out on empty streams hang up

[Mathieu Desnoyers]

Attaching to a stream hung up before providing any trace packet
causes ctf_open_mmap_stream_read() to return an error.

This kind of scenario can happen with the upcoming "lttng clear"
feature.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
 formats/ctf/ctf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
index 980ebc9a..1ba9017f 100644
--- a/formats/ctf/ctf.c
+++ b/formats/ctf/ctf.c
@@ -2571,8 +2571,13 @@ int ctf_open_mmap_stream_read(struct ctf_trace *td,
 	}
 
 	ret = prepare_mmap_stream_definition(td, file_stream, packet_seek);
-	if (ret)
+	if (ret) {
+		/* We need to check for EOF here for empty files. */
+		if (unlikely(file_stream->pos.offset == EOF)) {
+			ret = 0;
+		}
 		goto error_index;
+	}
 
 	/*
 	 * For now, only a single clock per trace is supported.
-- 
2.17.1

[lttng-dev] [PATCH babeltrace-1.5 6/6] Fix: lttng-live format: do not error out on empty streams hang up

[Jérémie Galarneau]

All patches of this series were merged in stable-1.5.

Thanks!
J?r?mie

On Thu, 5 Dec 2019 at 01:58, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
>
> Attaching to a stream hung up before providing any trace packet
> causes ctf_open_mmap_stream_read() to return an error.
>
> This kind of scenario can happen with the upcoming "lttng clear"
> feature.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> ---
>  formats/ctf/ctf.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
> index 980ebc9a..1ba9017f 100644
> --- a/formats/ctf/ctf.c
> +++ b/formats/ctf/ctf.c
> @@ -2571,8 +2571,13 @@ int ctf_open_mmap_stream_read(struct ctf_trace *td,
>         }
>
>         ret = prepare_mmap_stream_definition(td, file_stream, packet_seek);
> -       if (ret)
> +       if (ret) {
> +               /* We need to check for EOF here for empty files. */
> +               if (unlikely(file_stream->pos.offset == EOF)) {
> +                       ret = 0;
> +               }
>                 goto error_index;
> +       }
>
>         /*
>          * For now, only a single clock per trace is supported.
> --
> 2.17.1
>


-- 
J?r?mie Galarneau
EfficiOS Inc.
http://www.efficios.com