From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Fri, 28 Sep 2012 20:41:25 +0000 (-0400)
Subject: Fix: consumer_allocate_stream error handling
X-Git-Tag: v2.1.0-rc5~42
X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=c80048c6378b9ee7796c4b833a2c07f6050cc6d7

Fix: consumer_allocate_stream error handling

Fix a memory leak and "be nice" when handling stream alloc errors. Upon
CPU hotplug, it is possible that we receive a stream only after all
other streams are finalized, which means it could happen that we discard
that channel, in the unlikely event that we have cpu hotplug
concurrently with destroy.

Moreover, this fix the return path of channel lookup failure: we were
returning an zeroed stream rather than returning an error, which was
certainly not the intended behavior.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: David Goulet <dgoulet@efficios.com>
---

diff --git a/src/common/consumer.c b/src/common/consumer.c
index f4eaf705f..a9e4dee66 100644
--- a/src/common/consumer.c
+++ b/src/common/consumer.c
@@ -345,7 +345,8 @@ struct lttng_consumer_stream *consumer_allocate_stream(
 		uid_t uid,
 		gid_t gid,
 		int net_index,
-		int metadata_flag)
+		int metadata_flag,
+		int *alloc_ret)
 {
 	struct lttng_consumer_stream *stream;
 	int ret;
@@ -353,12 +354,13 @@ struct lttng_consumer_stream *consumer_allocate_stream(
 	stream = zmalloc(sizeof(*stream));
 	if (stream == NULL) {
 		perror("malloc struct lttng_consumer_stream");
-		goto end;
+		*alloc_ret = -ENOMEM;
+		return NULL;
 	}
 	stream->chan = consumer_find_channel(channel_key);
 	if (!stream->chan) {
-		perror("Unable to find channel key");
-		goto end;
+		*alloc_ret = -ENOENT;
+		goto error;
 	}
 	stream->chan->refcount++;
 	stream->key = stream_key;
@@ -387,14 +389,14 @@ struct lttng_consumer_stream *consumer_allocate_stream(
 		stream->cpu = stream->chan->cpucount++;
 		ret = lttng_ustconsumer_allocate_stream(stream);
 		if (ret) {
-			free(stream);
-			return NULL;
+			*alloc_ret = -EINVAL;
+			goto error;
 		}
 		break;
 	default:
 		ERR("Unknown consumer_data type");
-		assert(0);
-		goto end;
+		*alloc_ret = -EINVAL;
+		goto error;
 	}
 
 	/*
@@ -413,9 +415,11 @@ struct lttng_consumer_stream *consumer_allocate_stream(
 			stream->shm_fd, stream->wait_fd,
 			(unsigned long long) stream->mmap_len, stream->out_fd,
 			stream->net_seq_idx);
-
-end:
 	return stream;
+
+error:
+	free(stream);
+	return NULL;
 }
 
 /*
diff --git a/src/common/consumer.h b/src/common/consumer.h
index 0f82a1086..9a93c4279 100644
--- a/src/common/consumer.h
+++ b/src/common/consumer.h
@@ -338,7 +338,8 @@ extern struct lttng_consumer_stream *consumer_allocate_stream(
 		uid_t uid,
 		gid_t gid,
 		int net_index,
-		int metadata_flag);
+		int metadata_flag,
+		int *alloc_ret);
 extern int consumer_add_stream(struct lttng_consumer_stream *stream);
 extern void consumer_del_stream(struct lttng_consumer_stream *stream);
 extern void consumer_change_stream_state(int stream_key,
diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 5a219fc0b..a288df3cd 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -141,6 +141,7 @@ int lttng_kconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 		int fd;
 		struct consumer_relayd_sock_pair *relayd = NULL;
 		struct lttng_consumer_stream *new_stream;
+		int alloc_ret = 0;
 
 		/* block */
 		if (lttng_consumer_poll_socket(consumer_sockpoll) < 0) {
@@ -166,9 +167,23 @@ int lttng_kconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 				msg.u.stream.uid,
 				msg.u.stream.gid,
 				msg.u.stream.net_index,
-				msg.u.stream.metadata_flag);
+				msg.u.stream.metadata_flag,
+				&alloc_ret);
 		if (new_stream == NULL) {
-			lttng_consumer_send_error(ctx, LTTCOMM_CONSUMERD_OUTFD_ERROR);
+			switch (alloc_ret) {
+			case -ENOMEM:
+			case -EINVAL:
+			default:
+				lttng_consumer_send_error(ctx, LTTCOMM_CONSUMERD_OUTFD_ERROR);
+				break;
+			case -ENOENT:
+				/*
+				 * We could not find the channel. Can happen if cpu hotplug
+				 * happens while tearing down.
+				 */
+				DBG3("Could not find channel");
+				break;
+			}
 			goto end_nosignal;
 		}
 
diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index ad4b014c6..9e1a59f5e 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -174,6 +174,7 @@ int lttng_ustconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 		int fds[2];
 		size_t nb_fd = 2;
 		struct consumer_relayd_sock_pair *relayd = NULL;
+		int alloc_ret = 0;
 
 		DBG("UST Consumer adding stream");
 
@@ -204,9 +205,23 @@ int lttng_ustconsumer_recv_cmd(struct lttng_consumer_local_data *ctx,
 				msg.u.stream.uid,
 				msg.u.stream.gid,
 				msg.u.stream.net_index,
-				msg.u.stream.metadata_flag);
+				msg.u.stream.metadata_flag,
+				&alloc_ret);
 		if (new_stream == NULL) {
-			lttng_consumer_send_error(ctx, LTTCOMM_CONSUMERD_OUTFD_ERROR);
+			switch (alloc_ret) {
+			case -ENOMEM:
+			case -EINVAL:
+			default:
+				lttng_consumer_send_error(ctx, LTTCOMM_CONSUMERD_OUTFD_ERROR);
+				break;
+			case -ENOENT:
+				/*
+				 * We could not find the channel. Can happen if cpu hotplug
+				 * happens while tearing down.
+				 */
+				DBG3("Could not find channel");
+				break;
+			}
 			goto end_nosignal;
 		}