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 <[email protected]> Change-Id: I99dc595ee5b0288c727b193ed061f5273752bd24 Signed-off-by: Jérémie Galarneau <[email protected]>
lttng · Sep 23, 2021 · d374061 · d374061
1 parent 8f4456a
commit d374061
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,14 +478,14 @@ 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,
 				session->uid,
 				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
@@ -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 @@ lttng_evaluation_session_rotation_get_id(
 	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
@@ -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
@@ -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
@@ -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);
 }