From b0563feda603e2f21fb331adb4994edf4080f891 Mon Sep 17 00:00:00 2001 From: Jonathan Rajotte Date: Thu, 16 Sep 2021 11:20:07 -0400 Subject: [PATCH] Fix: lttng_trace_archive_location_serialize is called on freed memory MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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=, source_object_creds=0x7f1cb000a874, client_report=0x475840 , 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=) 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 Change-Id: I99dc595ee5b0288c727b193ed061f5273752bd24 Signed-off-by: Jérémie Galarneau --- include/lttng/location-internal.h | 14 +++++++- src/bin/lttng-sessiond/cmd.c | 1 + .../notification-thread-commands.h | 1 + src/bin/lttng-sessiond/rotation-thread.c | 3 +- src/common/location.c | 32 ++++++++++++++----- src/common/session-rotation.c | 14 ++++++-- src/lib/lttng-ctl/destruction-handle.c | 3 +- src/lib/lttng-ctl/rotate.c | 2 +- 8 files changed, 56 insertions(+), 14 deletions(-) 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 #include #include +#include +/* + * 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 eb5da1b76..3146126b2 100644 --- a/src/bin/lttng-sessiond/cmd.c +++ b/src/bin/lttng-sessiond/cmd.c @@ -3303,6 +3303,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 a90d1ac2b..c167a6911 100644 --- a/src/bin/lttng-sessiond/notification-thread-commands.h +++ b/src/bin/lttng-sessiond/notification-thread-commands.h @@ -60,6 +60,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; } parameters; diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c index f07672c22..fa6808005 100644 --- a/src/bin/lttng-sessiond/rotation-thread.c +++ b/src/bin/lttng-sessiond/rotation-thread.c @@ -25,6 +25,7 @@ #include #include #include +#include #include "rotation-thread.h" #include "lttng-sessiond.h" @@ -478,7 +479,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, @@ -486,6 +486,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("[rotation-thread] Failed to notify notification thread of completed rotation for session %s", session->name); diff --git a/src/common/location.c b/src/common/location.c index 294378c75..e824568cb 100644 --- a/src/common/location.c +++ b/src/common/location.c @@ -8,6 +8,7 @@ #include #include #include +#include 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/common/session-rotation.c b/src/common/session-rotation.c index f8d4439de..3536f9602 100644 --- a/src/common/session-rotation.c +++ b/src/common/session-rotation.c @@ -334,6 +334,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; } @@ -377,11 +380,12 @@ ssize_t create_evaluation_from_buffer( 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; } @@ -536,7 +540,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); } @@ -559,6 +563,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/lib/lttng-ctl/destruction-handle.c b/src/lib/lttng-ctl/destruction-handle.c index b345c2834..8c832969b 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); } -- 2.34.1