From: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
Date: Thu, 16 Sep 2021 15:20:07 +0000 (-0400)
Subject: Fix: lttng_trace_archive_location_serialize is called on freed memory
X-Git-Url: http://drtracing.org/?a=commitdiff_plain;h=ddbee47f5457a9f06116a86143e3e4831c5419ce;p=lttng-tools.git

Fix: lttng_trace_archive_location_serialize is called on freed memory

Observed issue
==============

The following backtrace have been reported [1].

 #0  __GI_raise (sig=sig@entry=6) at /usr/src/debug/glibc/2.31+gitAUTOINC+f84949f1c4-r0/git/sysdeps/unix/sysv/linux/raise.c:50
 #1  0x0000003123025528 in __GI_abort () at /usr/src/debug/glibc/2.31+gitAUTOINC+f84949f1c4-r0/git/stdlib/abort.c:79
 #2  0x0000000000419884 in lttng_trace_archive_location_serialize (location=0x7f1c9c001160, buffer=0x7f1cb961c320) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/common/location.c:230
 #3  0x00000000004c8f06 in lttng_evaluation_session_rotation_serialize (evaluation=0x7f1cb000a7f0, payload=0x7f1cb961c320) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/common/conditions/session-rotation.c:539
 #4  0x00000000004a80fa in lttng_evaluation_serialize (evaluation=0x7f1cb000a7f0, payload=0x7f1cb961c320) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/common/evaluation.c:42
 #5  0x00000000004bc24f in lttng_notification_serialize (notification=0x7f1cb961c310, payload=0x7f1cb961c320) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/common/notification.c:63
 #6  0x0000000000458b7d in notification_client_list_send_evaluation (client_list=0x7f1cb0008f90, trigger=0x7f1ca40113d0, evaluation=<optimized out>, source_object_creds=0x7f1cb000a874, client_report=0x475840 <client_handle_transmission_status>, user_data=0x7f1cb0006010) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/bin/lttng-sessiond/notification-thread-events.c:4379
 #7  0x0000000000476586 in action_executor_generic_handler (item=0x7f1cb0009600, work_item=0x7f1cb000a820, executor=0x7f1cb0006010) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/bin/lttng-sessiond/action-executor.c:696
 #8  action_work_item_execute (work_item=0x7f1cb000a820, executor=0x7f1cb0006010) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/bin/lttng-sessiond/action-executor.c:715
 #9  action_executor_thread (_data=0x7f1cb0006010) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/bin/lttng-sessiond/action-executor.c:797
 #10 0x0000000000462327 in launch_thread (data=0x7f1cb00060b0) at /usr/src/debug/lttng-tools/2.13.0-r0/lttng-tools-2.13.0/src/bin/lttng-sessiond/thread.c:66
 #11 0x0000003123408ea4 in start_thread (arg=<optimized out>) at /usr/src/debug/glibc/2.31+gitAUTOINC+f84949f1c4-r0/git/nptl/pthread_create.c:477
 #12 0x00000031230f8dcf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

This can be easily reproduced with the following session and trigger
configuration:

 lttng create test
 lttng enable-event -u -a
 lttng start
 # Register two similar triggers via a dummy C program since rotation
 # completed condition is not exposed on the CLI for now. Yielding the
 # following triggers:
 lttng list-triggers
 - name: trigger0
   owner uid: 1000
   condition: session rotation completed
     session name: test
     errors: none
  action:notify
   errors: none
 - name: trigger1
   owner uid: 1000
   condition: session rotation completed
     session name: test
     errors: none
  action:notify
   errors: none

  lttng rotate <- abort happens here.

Cause
=====

The problem lies in how the location (`lttng_trace_archive_location`)
object is assigned to the `lttng_evaluation` objects. A single location
object can end up being shared between multiple `lttng_evaluation` objects
since we iterate over all triggers and create an `lttng_evaluation` object
with the location each time as needed.

See `src/bin/lttng-sessiond/notification-thread-events.c:1956`.

The location object is then freed when the first notification is
completely serialized. The second serialization end up having a
reference to a freed `lttng_trace_archive_location` object.

Solution
========

Implement ref counting for the lttng_trace_archive_location object.

Note
=======

This also fixes a leak that was present in `cmd_destroy_session_reply`.

The location is created by `session_get_trace_archive_location` and is
never `destroyed`/`put`.

Known drawbacks
=========

None.

References
==========

[1] https://bugs.lttng.org/issues/1325

Fixes: #1325

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
Change-Id: I99dc595ee5b0288c727b193ed061f5273752bd24
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---

diff --git a/include/lttng/location-internal.h b/include/lttng/location-internal.h
index d33cab652..45bd78a36 100644
--- a/include/lttng/location-internal.h
+++ b/include/lttng/location-internal.h
@@ -13,8 +13,16 @@
 #include <common/buffer-view.h>
 #include <common/macros.h>
 #include <sys/types.h>
+#include <urcu/ref.h>
 
+/*
+ * The public API assumes that trace archive locations are always
+ * provided as "constant". This means that the user of liblttng-ctl never
+ * has to destroy a trace archive location. Hence, users of liblttng-ctl
+ * have no visibility of the reference counting of archive locations.
+ */
 struct lttng_trace_archive_location {
+	struct urcu_ref ref;
 	enum lttng_trace_archive_location_type type;
 	union {
 		struct {
@@ -88,7 +96,11 @@ ssize_t lttng_trace_archive_location_serialize(
 		struct lttng_dynamic_buffer *buffer);
 
 LTTNG_HIDDEN
-void lttng_trace_archive_location_destroy(
+void lttng_trace_archive_location_get(
+		struct lttng_trace_archive_location *location);
+
+LTTNG_HIDDEN
+void lttng_trace_archive_location_put(
 		struct lttng_trace_archive_location *location);
 
 #endif /* LTTNG_LOCATION_INTERNAL_H */
diff --git a/src/bin/lttng-sessiond/cmd.c b/src/bin/lttng-sessiond/cmd.c
index 3349ff000..3b776c576 100644
--- a/src/bin/lttng-sessiond/cmd.c
+++ b/src/bin/lttng-sessiond/cmd.c
@@ -3241,6 +3241,7 @@ void cmd_destroy_session_reply(const struct ltt_session *session,
 	payload_size_before_location = payload.size;
 	comm_ret = lttng_trace_archive_location_serialize(location,
 			&payload);
+	lttng_trace_archive_location_put(location);
 	if (comm_ret < 0) {
 		ERR("Failed to serialize the location of the trace archive produced during the destruction of session \"%s\"",
 				session->name);
diff --git a/src/bin/lttng-sessiond/notification-thread-commands.h b/src/bin/lttng-sessiond/notification-thread-commands.h
index 0aa7a80c3..643b0e602 100644
--- a/src/bin/lttng-sessiond/notification-thread-commands.h
+++ b/src/bin/lttng-sessiond/notification-thread-commands.h
@@ -73,6 +73,7 @@ struct notification_thread_command {
 			uid_t uid;
 			gid_t gid;
 			uint64_t trace_archive_chunk_id;
+			/* Weak reference. */
 			struct lttng_trace_archive_location *location;
 		} session_rotation;
 		/* Add/Remove tracer event source fd. */
diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index 89a148a74..ac149c845 100644
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -24,6 +24,7 @@
 #include <common/kernel-ctl/kernel-ctl.h>
 #include <lttng/notification/channel-internal.h>
 #include <lttng/rotate-internal.h>
+#include <lttng/location-internal.h>
 
 #include "rotation-thread.h"
 #include "lttng-sessiond.h"
@@ -477,7 +478,6 @@ int check_session_rotation_pending(struct ltt_session *session,
 
 	if (!session->quiet_rotation) {
 		location = session_get_trace_archive_location(session);
-		/* Ownership of location is transferred. */
 		ret = notification_thread_command_session_rotation_completed(
 				notification_thread_handle,
 				session->name,
@@ -485,6 +485,7 @@ int check_session_rotation_pending(struct ltt_session *session,
 				session->gid,
 				session->last_archived_chunk_id.value,
 				location);
+		lttng_trace_archive_location_put(location);
 		if (ret != LTTNG_OK) {
 			ERR("Failed to notify notification thread of completed rotation for session %s",
 					session->name);
diff --git a/src/common/conditions/session-rotation.c b/src/common/conditions/session-rotation.c
index 1be0f0ad2..cf6a1c5a6 100644
--- a/src/common/conditions/session-rotation.c
+++ b/src/common/conditions/session-rotation.c
@@ -344,6 +344,9 @@ struct lttng_evaluation *lttng_evaluation_session_rotation_create(
 			sizeof(evaluation->parent));
 	lttng_evaluation_init(&evaluation->parent, type);
 	evaluation->id = id;
+	if (location) {
+		lttng_trace_archive_location_get(location);
+	}
 	evaluation->location = location;
 	return &evaluation->parent;
 }
@@ -390,11 +393,12 @@ ssize_t create_evaluation_from_payload(
 		goto error;
 	}
 
+	lttng_trace_archive_location_put(location);
 	ret = size;
 	*_evaluation = evaluation;
 	return ret;
 error:
-	lttng_trace_archive_location_destroy(location);
+	lttng_trace_archive_location_put(location);
 	evaluation = NULL;
 	return -1;
 }
@@ -550,7 +554,7 @@ void lttng_evaluation_session_rotation_destroy(
 
 	rotation = container_of(evaluation,
 			struct lttng_evaluation_session_rotation, parent);
-	lttng_trace_archive_location_destroy(rotation->location);
+	lttng_trace_archive_location_put(rotation->location);
 	free(rotation);
 }
 
@@ -573,6 +577,12 @@ end:
 	return status;
 }
 
+/*
+ * The public API assumes that trace archive locations are always provided as
+ * "constant". This means that the user of liblttng-ctl never has to destroy a
+ * trace archive location. Hence, users of liblttng-ctl have no visibility of
+ * the reference counting of archive locations.
+ */
 enum lttng_evaluation_status
 lttng_evaluation_session_rotation_completed_get_location(
 		const struct lttng_evaluation *evaluation,
diff --git a/src/common/location.c b/src/common/location.c
index c79f85475..97fed09b1 100644
--- a/src/common/location.c
+++ b/src/common/location.c
@@ -8,6 +8,7 @@
 #include <lttng/location-internal.h>
 #include <common/macros.h>
 #include <stdlib.h>
+#include <common/error.h>
 
 static
 struct lttng_trace_archive_location *lttng_trace_archive_location_create(
@@ -20,18 +21,17 @@ struct lttng_trace_archive_location *lttng_trace_archive_location_create(
 		goto end;
 	}
 
+	urcu_ref_init(&location->ref);
 	location->type = type;
 end:
 	return location;
 }
 
-LTTNG_HIDDEN
-void lttng_trace_archive_location_destroy(
-		struct lttng_trace_archive_location *location)
+static
+void trace_archive_location_destroy_ref(struct urcu_ref *ref)
 {
-	if (!location) {
-		return;
-	}
+	struct lttng_trace_archive_location *location =
+			container_of(ref, struct lttng_trace_archive_location, ref);
 
 	switch (location->type) {
 	case LTTNG_TRACE_ARCHIVE_LOCATION_TYPE_LOCAL:
@@ -48,6 +48,22 @@ void lttng_trace_archive_location_destroy(
 	free(location);
 }
 
+LTTNG_HIDDEN
+void lttng_trace_archive_location_get(struct lttng_trace_archive_location *location)
+{
+	urcu_ref_get(&location->ref);
+}
+
+LTTNG_HIDDEN
+void lttng_trace_archive_location_put(struct lttng_trace_archive_location *location)
+{
+	if (!location) {
+		return;
+	}
+
+	urcu_ref_put(&location->ref, trace_archive_location_destroy_ref);
+}
+
 LTTNG_HIDDEN
 struct lttng_trace_archive_location *lttng_trace_archive_location_local_create(
 		const char *absolute_path)
@@ -72,7 +88,7 @@ struct lttng_trace_archive_location *lttng_trace_archive_location_local_create(
 end:
 	return location;
 error:
-	lttng_trace_archive_location_destroy(location);
+	lttng_trace_archive_location_put(location);
 	return NULL;
 }
 
@@ -110,7 +126,7 @@ struct lttng_trace_archive_location *lttng_trace_archive_location_relay_create(
 end:
 	return location;
 error:
-	lttng_trace_archive_location_destroy(location);
+	lttng_trace_archive_location_put(location);
 	return NULL;
 }
 
diff --git a/src/lib/lttng-ctl/destruction-handle.c b/src/lib/lttng-ctl/destruction-handle.c
index 965a823d8..6d8a26b11 100644
--- a/src/lib/lttng-ctl/destruction-handle.c
+++ b/src/lib/lttng-ctl/destruction-handle.c
@@ -59,7 +59,7 @@ void lttng_destruction_handle_destroy(struct lttng_destruction_handle *handle)
 	}
 	lttng_poll_clean(&handle->communication.events);
 	lttng_dynamic_buffer_reset(&handle->communication.buffer);
-	lttng_trace_archive_location_destroy(handle->location);
+	lttng_trace_archive_location_put(handle->location);
 	free(handle);
 }
 
@@ -173,6 +173,7 @@ int handle_state_transition(struct lttng_destruction_handle *handle)
 			ret = -1;
 			break;
 		} else {
+			/* Ownership is transferred to the destruction handle. */
 			handle->location = location;
 			handle->communication.state = COMMUNICATION_STATE_END;
 		}
diff --git a/src/lib/lttng-ctl/rotate.c b/src/lib/lttng-ctl/rotate.c
index 1002e230b..17964fb10 100644
--- a/src/lib/lttng-ctl/rotate.c
+++ b/src/lib/lttng-ctl/rotate.c
@@ -169,7 +169,7 @@ void lttng_rotation_handle_destroy(
 	if (!rotation_handle) {
 		return;
 	}
-	lttng_trace_archive_location_destroy(rotation_handle->archive_location);
+	lttng_trace_archive_location_put(rotation_handle->archive_location);
 	free(rotation_handle);
 }