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 d33cab652668c38d713c801bc71df132f953fbbe..45bd78a36a86e9657a23d26453a811e38541edcd 100644 (file)
--- 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 3349ff000f415d60073760085f8fce0de156c105..3b776c57634fcfa5f05688a2c7b2c8233fb505b7 100644 (file)
--- 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 0aa7a80c3f94cc3a8a1e519f620ac625d2b5d7b6..643b0e6020913432b47e763790cbf3421080e2cc 100644 (file)
--- 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 89a148a74c2bdd1393bb274afb4ae9f0f545caa1..ac149c845b79c9436674908ee9c5b942db79c0d2 100644 (file)
--- 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 1be0f0ad25c5f20b1c27028160b6d3ce231e79de..cf6a1c5a6539679a3815ca3524e730355b67c127 100644 (file)
--- 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 c79f8547546f6e65e0aa2dd5012c9c187f1108b7..97fed09b1ff380e87712c2565e96344894a8a6aa 100644 (file)
--- 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 965a823d8085f8a1379399df87d4a733cbe696ba..6d8a26b113481914ff01e8e691d23eec23699750 100644 (file)
--- 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 1002e230b64e504b55583e1770aa49b42d38a88c..17964fb1070784b9b65badf61eca9a94da9e819c 100644 (file)
--- 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);
 }