From 29e191fceb6167bc486c8a492ec7652670de8355 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 27 Nov 2023 16:23:10 -0500 Subject: [PATCH] Fix: lib: strengthen clock expectation check for no Unix epoch / no UUID case When an iterator sees a clock class with an origin that is not the Unix and has no UUID, it sets its clock expectation to CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID. In this mode, when validating subsequent clock classes, it validates that the clock class' origin is not the Unix epoch and that it has no UUID. This is too loose: an iterator could send messages with two distinct clocks with unknown origins and no UUID, and it would pass validation. However, these two clock classes are not correletable. Fix that by making the iterator remember which specific clock class instance it saw first. Each subsequent message must have exactly that clock class instance. To be sure that a clock class an iterator has saved doesn't get freed, and then a new one reallocated at the same address, the iterator takes a strong reference to the clock class. This ensures that the saved clock class at least outlives the iterator. TODO: I would like to write a test for this Change-Id: I339936b730fe2f4e64dbc58d56557ffcd23cce16 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/11448 Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/lib/graph/iterator.c | 23 +++++++++++++---------- src/lib/graph/iterator.h | 37 +++++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/lib/graph/iterator.c b/src/lib/graph/iterator.c index 8da13282..f882d6c3 100644 --- a/src/lib/graph/iterator.c +++ b/src/lib/graph/iterator.c @@ -118,6 +118,12 @@ void bt_message_iterator_destroy(struct bt_object *obj) "%!+i", iterator); bt_message_iterator_try_finalize(iterator); + if (iterator->clock_expectation.type == + CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID) { + BT_CLOCK_CLASS_PUT_REF_AND_RESET( + iterator->clock_expectation.clock_class); + } + if (iterator->connection) { /* * Remove ourself from the originating connection so @@ -700,6 +706,8 @@ bool clock_classes_are_compatible_one(struct bt_message_iterator *iterator, bt_uuid_copy(iterator->clock_expectation.uuid, clock_class_uuid); } else { iterator->clock_expectation.type = CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID; + iterator->clock_expectation.clock_class = clock_class; + bt_clock_class_get_ref(iterator->clock_expectation.clock_class); } break; @@ -768,26 +776,21 @@ bool clock_classes_are_compatible_one(struct bt_message_iterator *iterator, case CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID: if (!clock_class) { BT_ASSERT_COND_DEV_MSG( - "Expecting a clock class, got none."); + "Expecting clock class %![cc-]+K, got none.", + iterator->clock_expectation.clock_class); result = false; goto end; } - if (bt_clock_class_origin_is_unix_epoch(clock_class)) { + if (clock_class != iterator->clock_expectation.clock_class) { BT_ASSERT_COND_DEV_MSG( - "Expecting a clock class without Unix epoch origin: %![cc-]+K", + "Expecting clock class %![cc-]+K, got %![cc-]+K.", + iterator->clock_expectation.clock_class, clock_class); result = false; goto end; } - if (clock_class_uuid) { - BT_ASSERT_COND_DEV_MSG( - "Expecting a clock class without UUID: %![cc-]+K", - clock_class); - result = false; - goto end; - } break; } } diff --git a/src/lib/graph/iterator.h b/src/lib/graph/iterator.h index ed093633..581f9c1f 100644 --- a/src/lib/graph/iterator.h +++ b/src/lib/graph/iterator.h @@ -142,14 +142,35 @@ struct bt_message_iterator { CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID, } type; - /* - * Expected UUID of the clock, if `type`is CLOCK_EXPECTATION_ORIGIN_OTHER_UUID. - * - * If the clock's origin is the unix epoch, the UUID is - * irrelevant (as the clock will be correlatable with other - * clocks having the same origin). - */ - bt_uuid_t uuid; + + union { + /* + * Expected UUID of the clock, if `type`is + * CLOCK_EXPECTATION_ORIGIN_OTHER_UUID. + * + * If the clock's origin is the unix epoch, the UUID is + * irrelevant (as the clock will be correlatable with other + * clocks having the same origin). + */ + bt_uuid_t uuid; + + /* + * Expected clock class, if `type` is + * CLOCK_EXPECTATION_ORIGIN_OTHER_NO_UUID. + * + * If the first clock class seen has an unknown origin + * and no UUID, then all subsequent clock classes seen + * must be the same instance. + * + * To make sure that the clock class pointed by this + * field doesn't get freed and another one reallocated + * at the same address (which could potentially bypass + * the clock expectation check), we keep a strong + * reference, ensuring that the clock class lives at + * least as long as the iterator. + */ + const bt_clock_class *clock_class; + }; } clock_expectation; BT_IF_DEV_MODE(GHashTable *per_stream_state); -- 2.34.1