From: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Date: Mon, 31 Jul 2017 21:51:35 +0000 (-0400)
Subject: Fix: ambiguous ownership of kernel context by multiple channels
X-Git-Tag: v2.8.8~3
X-Git-Url: https://git.lttng.org./?a=commitdiff_plain;h=f712b0897400a37cf3fa9abdbc11aa00df1e6993;p=lttng-tools.git

Fix: ambiguous ownership of kernel context by multiple channels

A kernel context, when added to multiple channels, must be copied
before being added to individual channels. The current code
adds the same ltt_kernel_context structure to multiple kernel
channels which introduces a conceptual ambiguity in the ownership
of the context object.

Concretely, creating multiple kernel channels and adding a context
to all of them (by not specifying a channel name) causes the context
to be added to each channels' list of contexts, overwritting the
context's list node, and causing the channel context lists to become
corrupted. This results in crashes being observed during the
destruction of the session.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---

diff --git a/src/bin/lttng-sessiond/context.c b/src/bin/lttng-sessiond/context.c
index 9c3a394ed..a2f022886 100644
--- a/src/bin/lttng-sessiond/context.c
+++ b/src/bin/lttng-sessiond/context.c
@@ -34,6 +34,8 @@
 
 /*
  * Add kernel context to all channel.
+ *
+ * Assumes the ownership of kctx.
  */
 static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
 		struct ltt_kernel_context *kctx)
@@ -48,7 +50,17 @@ static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
 
 	/* Go over all channels */
 	cds_list_for_each_entry(kchan, &ksession->channel_list.head, list) {
-		ret = kernel_add_channel_context(kchan, kctx);
+		struct ltt_kernel_context *kctx_copy;
+
+		kctx_copy = trace_kernel_copy_context(kctx);
+		if (!kctx_copy) {
+			PERROR("zmalloc ltt_kernel_context");
+			goto error;
+		}
+
+		/* Ownership of kctx_copy is transferred to the callee. */
+		ret = kernel_add_channel_context(kchan, kctx_copy);
+		kctx_copy = NULL;
 		if (ret != 0) {
 			goto error;
 		}
@@ -57,11 +69,14 @@ static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
 	ret = LTTNG_OK;
 
 error:
+	trace_kernel_destroy_context(kctx);
 	return ret;
 }
 
 /*
  * Add kernel context to a specific channel.
+ *
+ * Assumes the ownership of kctx.
  */
 static int add_kctx_to_channel(struct ltt_kernel_context *kctx,
 		struct ltt_kernel_channel *kchan)
@@ -73,7 +88,9 @@ static int add_kctx_to_channel(struct ltt_kernel_context *kctx,
 
 	DBG("Add kernel context to channel '%s'", kchan->channel->name);
 
+	/* Ownership of kctx is transferred to the callee. */
 	ret = kernel_add_channel_context(kchan, kctx);
+	kctx = NULL;
 	if (ret != 0) {
 		goto error;
 	}
@@ -254,6 +271,8 @@ int context_kernel_add(struct ltt_kernel_session *ksession,
 
 	if (*channel_name == '\0') {
 		ret = add_kctx_all_channels(ksession, kctx);
+		/* Ownership of kctx is transferred to the callee. */
+		kctx = NULL;
 		if (ret != LTTNG_OK) {
 			goto error;
 		}
@@ -266,12 +285,14 @@ int context_kernel_add(struct ltt_kernel_session *ksession,
 		}
 
 		ret = add_kctx_to_channel(kctx, kchan);
+		/* Ownership of kctx is transferred to the callee. */
+		kctx = NULL;
 		if (ret != LTTNG_OK) {
 			goto error;
 		}
 	}
 
-	return LTTNG_OK;
+	ret = LTTNG_OK;
 
 error:
 	if (kctx) {
diff --git a/src/bin/lttng-sessiond/kernel.c b/src/bin/lttng-sessiond/kernel.c
index 78978a234..8dc3f8631 100644
--- a/src/bin/lttng-sessiond/kernel.c
+++ b/src/bin/lttng-sessiond/kernel.c
@@ -37,6 +37,8 @@
 
 /*
  * Add context on a kernel channel.
+ *
+ * Assumes the ownership of ctx.
  */
 int kernel_add_channel_context(struct ltt_kernel_channel *chan,
 		struct ltt_kernel_context *ctx)
@@ -67,9 +69,11 @@ int kernel_add_channel_context(struct ltt_kernel_channel *chan,
 
 end:
 	cds_list_add_tail(&ctx->list, &chan->ctx_list);
-	return 0;
-
+	ctx = NULL;
 error:
+	if (ctx) {
+		trace_kernel_destroy_context(ctx);
+	}
 	return ret;
 }
 
diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c
index 29432de49..ce5aca2ed 100644
--- a/src/bin/lttng-sessiond/trace-kernel.c
+++ b/src/bin/lttng-sessiond/trace-kernel.c
@@ -240,11 +240,33 @@ struct ltt_kernel_context *trace_kernel_create_context(
 	if (ctx) {
 		memcpy(&kctx->ctx, ctx, sizeof(kctx->ctx));
 	}
+error:
+	return kctx;
+}
 
-	CDS_INIT_LIST_HEAD(&kctx->list);
+/*
+ * Allocate and init a kernel context object from an existing kernel context
+ * object.
+ *
+ * Return the allocated object or NULL on error.
+ */
+struct ltt_kernel_context *trace_kernel_copy_context(
+		struct ltt_kernel_context *kctx)
+{
+	struct ltt_kernel_context *kctx_copy;
+
+	assert(kctx);
+	kctx_copy = zmalloc(sizeof(*kctx_copy));
+	if (!kctx_copy) {
+		PERROR("zmalloc ltt_kernel_context");
+		goto error;
+	}
+
+	memcpy(kctx_copy, kctx, sizeof(*kctx_copy));
+	memset(&kctx_copy->list, 0, sizeof(kctx_copy->list));
 
 error:
-	return kctx;
+	return kctx_copy;
 }
 
 /*
diff --git a/src/bin/lttng-sessiond/trace-kernel.h b/src/bin/lttng-sessiond/trace-kernel.h
index 93870cbfd..a39ccd3bb 100644
--- a/src/bin/lttng-sessiond/trace-kernel.h
+++ b/src/bin/lttng-sessiond/trace-kernel.h
@@ -142,6 +142,8 @@ struct ltt_kernel_stream *trace_kernel_create_stream(const char *name,
 		unsigned int count);
 struct ltt_kernel_context *trace_kernel_create_context(
 		struct lttng_kernel_context *ctx);
+struct ltt_kernel_context *trace_kernel_copy_context(
+		struct ltt_kernel_context *ctx);
 
 /*
  * Destroy functions free() the data structure and remove from linked list if