From: Simon Marchi <simon.marchi@efficios.com>
Date: Mon, 27 Nov 2023 21:23:10 +0000 (-0500)
Subject: Fix: lib: strengthen clock expectation check for no Unix epoch / no UUID case
X-Git-Url: http://drtracing.org/?a=commitdiff_plain;h=29e191fceb61;p=babeltrace.git

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 <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/11448
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
Tested-by: jenkins <jenkins@lttng.org>
---

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);