lib: validate iterator message packets

Validate that messages coming out of iterators have sensible packet
values.  This applies to event and packet end messages: their packet
must match the packet of the previous packet beginning message.

Add a hash table to hold per-stream state, inside the
bt_message_iterator structure.  Since this state will only be used for
dev assertions, for the moment, only create it if BT_DEV_MODE is
defined.

Discard the per-stream state when the iterator seeks (after which the
iterator is expected to produce a new message sequence, starting from
scratch).

If the iterator uses auto-seek to implement "seek ns from origin", we
need to discard the per-stream state twice: once after seeking the
beginning, and once after consuming messages until the desired point.

When a wrong packet is detected, print an error logging message with
some details, before the failed assertion message:

    Babeltrace 2 library postcondition not satisfied.
    ------------------------------------------------------------------------
    Condition ID: `post:message-iterator-class-next-method:message-packet-is-expected`.
    Function: bt_message_iterator_class_next_method().
    ------------------------------------------------------------------------
    Error is:
    Message's packet is not expected: stream-addr=0x60d000001d80, stream-id=0, iterator-addr=0x611000004c80, iterator-upstream-comp-name="source.gpx.GpxSource", iterator-upstream-comp-log-level=WARNING, iterator-upstream-comp-class-type=SOURCE, iterator-upstream-comp-class-name="GpxSource", iterator-upstream-comp-class-partial-descr="", message-addr=0x607000004540, message-type=EVENT, received-packet-addr=0x607000004310, expected-packet-addr=(nil)
    Aborting...

The particular structure of the code is explained by the following
patch, which adds verification that the message sequence is as expected.
Both assertions (packet is expected, and message sequence is as
expected) need to know about the current packet (this state is
maintained by message_packet_is_valid), so must be in the same "for all
messages" loop.  Alternatively, they could both track the current packet
independently, but that would be redundant.

But this form also allows putting more info about the problematic
message in the abort notice, which I think is nice.

Change-Id: I176417d9ae7b04a9c16ff975e008e208b173e3d2
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/10448
Tested-by: jenkins <jenkins@lttng.org>
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>

diff --git a/src/common/macros.h b/src/common/macros.h
index ad4218c445f98caf104223b1499783f70ceb209d..903219b8779d228aae93efae7a03fc56cba61129 100644 (file)
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -29,6 +29,13 @@ extern "C" {
 #define BT_EXPORT __attribute__((visibility("default")))
 #endif
 
+/* Enable `txt` if developer mode is enabled. */
+#ifdef BT_DEV_MODE
+#define BT_IF_DEV_MODE(txt) txt
+#else
+#define BT_IF_DEV_MODE(txt)
+#endif
+
 /*
  * Yield `ref`'s value while setting `ref` to NULL.
  *

diff --git a/src/lib/graph/iterator.c b/src/lib/graph/iterator.c
index 9bcbab55ee6b59627c710c6146566fb6ca593d3d..dc1c778d919aab3657623b4244519a459e0d5939 100644 (file)
--- a/src/lib/graph/iterator.c
+++ b/src/lib/graph/iterator.c
@@ -66,6 +66,23 @@
                (_iter)->state == BT_MESSAGE_ITERATOR_STATE_LAST_SEEKING_RETURNED_ERROR, \
                "Message iterator is in the wrong state: %!+i", (_iter))
 
+#ifdef BT_DEV_MODE
+struct per_stream_state
+{
+       bt_packet *cur_packet;
+};
+#endif
+
+static void
+clear_per_stream_state (struct bt_message_iterator *iterator)
+{
+#ifdef BT_DEV_MODE
+       g_hash_table_remove_all(iterator->per_stream_state);
+#else
+       BT_USE_EXPR(iterator);
+#endif
+}
+
 static inline
 void set_msg_iterator_state(struct bt_message_iterator *iterator,
                enum bt_message_iterator_state state)
@@ -135,6 +152,10 @@ void bt_message_iterator_destroy(struct bt_object *obj)
                iterator->msgs = NULL;
        }
 
+#ifdef BT_DEV_MODE
+       g_hash_table_destroy(iterator->per_stream_state);
+#endif
+
        g_free(iterator);
 }
 
@@ -341,6 +362,16 @@ int create_self_component_input_port_message_iterator(
 
        g_ptr_array_set_size(iterator->msgs, MSG_BATCH_SIZE);
        iterator->last_ns_from_origin = INT64_MIN;
+
+#ifdef BT_DEV_MODE
+       /* The per-stream state is only used for dev assertions right now. */
+       iterator->per_stream_state = g_hash_table_new_full(
+               g_direct_hash,
+               g_direct_equal,
+               NULL,
+               g_free);
+#endif
+
        iterator->auto_seek.msgs = g_queue_new();
        if (!iterator->auto_seek.msgs) {
                BT_LIB_LOGE_APPEND_CAUSE("Failed to allocate a GQueue.");
@@ -788,13 +819,156 @@ end:
        return result;
 }
 
+#ifdef BT_DEV_MODE
+static
+const bt_stream *get_stream_from_msg(const struct bt_message *msg)
+{
+       struct bt_stream *stream;
+
+       switch (msg->type) {
+        case BT_MESSAGE_TYPE_STREAM_BEGINNING:
+        case BT_MESSAGE_TYPE_STREAM_END:
+       {
+               struct bt_message_stream *msg_stream =
+                       (struct bt_message_stream *) msg;
+               stream = msg_stream->stream;
+               break;
+       }
+        case BT_MESSAGE_TYPE_EVENT:
+       {
+               struct bt_message_event *msg_event =
+                       (struct bt_message_event *) msg;
+               stream = msg_event->event->stream;
+               break;
+       }
+        case BT_MESSAGE_TYPE_PACKET_BEGINNING:
+        case BT_MESSAGE_TYPE_PACKET_END:
+       {
+               struct bt_message_packet *msg_packet =
+                       (struct bt_message_packet *) msg;
+               stream = msg_packet->packet->stream;
+               break;
+       }
+        case BT_MESSAGE_TYPE_DISCARDED_EVENTS:
+        case BT_MESSAGE_TYPE_DISCARDED_PACKETS:
+       {
+               struct bt_message_discarded_items *msg_discarded =
+                       (struct bt_message_discarded_items *) msg;
+               stream = msg_discarded->stream;
+               break;
+       }
+        case BT_MESSAGE_TYPE_MESSAGE_ITERATOR_INACTIVITY:
+               stream = NULL;
+               break;
+       default:
+               bt_common_abort();
+        }
+
+       return stream;
+}
+
+static
+struct per_stream_state *get_per_stream_state(
+               struct bt_message_iterator *iterator,
+               const struct bt_stream *stream)
+{
+       struct per_stream_state *state = g_hash_table_lookup(
+               iterator->per_stream_state, stream);
+
+       if (!state) {
+               state = g_new0(struct per_stream_state, 1);
+               g_hash_table_insert(iterator->per_stream_state,
+                       (gpointer) stream, state);
+       }
+
+       return state;
+}
+#endif
+
+#define NEXT_METHOD_NAME       "bt_message_iterator_class_next_method"
+
+#ifdef BT_DEV_MODE
+static
+void assert_post_dev_expected_packet(struct bt_message_iterator *iterator,
+               const struct bt_message *msg)
+{
+       const bt_stream *stream = get_stream_from_msg(msg);
+       struct per_stream_state *state;
+       const bt_packet *actual_packet = NULL;
+       const bt_packet *expected_packet = NULL;
+
+       if (!stream) {
+               goto end;
+       }
+
+       state = get_per_stream_state(iterator, stream);
+
+       switch (msg->type) {
+       case BT_MESSAGE_TYPE_EVENT:
+       {
+               const struct bt_message_event *msg_event =
+                       (const struct bt_message_event *) msg;
+
+               actual_packet = msg_event->event->packet;
+               expected_packet = state->cur_packet;
+               break;
+       }
+       case BT_MESSAGE_TYPE_PACKET_BEGINNING:
+       {
+               const struct bt_message_packet *msg_packet =
+                       (const struct bt_message_packet *) msg;
+
+               BT_ASSERT(!state->cur_packet);
+               state->cur_packet = msg_packet->packet;
+               break;
+       }
+       case BT_MESSAGE_TYPE_PACKET_END:
+       {
+               const struct bt_message_packet *msg_packet =
+                       (const struct bt_message_packet *) msg;
+
+               actual_packet = msg_packet->packet;
+               expected_packet = state->cur_packet;
+               BT_ASSERT(state->cur_packet);
+               state->cur_packet = NULL;
+               break;
+       }
+       default:
+               break;
+       }
+
+       BT_ASSERT_POST_DEV(NEXT_METHOD_NAME,
+               "message-packet-is-expected",
+               actual_packet == expected_packet,
+               "Message's packet is not expected: %![stream-]s, %![iterator-]i, "
+               "%![message-]n, %![received-packet-]a, %![expected-packet-]a",
+               stream, iterator, msg, actual_packet, expected_packet);
+
+end:
+       return;
+}
+
+static
+void assert_post_dev_next(
+               struct bt_message_iterator *iterator,
+               bt_message_iterator_class_next_method_status status,
+               bt_message_array_const msgs, uint64_t msg_count)
+{
+       if (status == BT_MESSAGE_ITERATOR_CLASS_NEXT_METHOD_STATUS_OK) {
+               uint64_t i;
+
+               for (i = 0; i < msg_count; i++) {
+                       assert_post_dev_expected_packet(iterator, msgs[i]);
+               }
+       }
+}
+#endif
+
 /*
  * Call the `next` method of the iterator.  Do some validation on the returned
  * messages.
  */
 
-#define NEXT_METHOD_NAME       "bt_message_iterator_class_next_method"
-
 static
 enum bt_message_iterator_class_next_method_status
 call_iterator_next_method(
@@ -822,6 +996,10 @@ call_iterator_next_method(
                        "Clock snapshots are not monotonic");
        }
 
+#ifdef BT_DEV_MODE
+       assert_post_dev_next(iterator, status, msgs, *user_count);
+#endif
+
        BT_ASSERT_POST_DEV_NO_ERROR_IF_NO_ERROR_STATUS(NEXT_METHOD_NAME,
                status);
 
@@ -1169,6 +1347,8 @@ bt_message_iterator_seek_beginning(struct bt_message_iterator *iterator)
                        iterator, bt_common_func_status_string(status));
        }
 
+       clear_per_stream_state(iterator);
+
        set_iterator_state_after_seeking(iterator, status);
        return status;
 }
@@ -1826,6 +2006,8 @@ bt_message_iterator_seek_ns_from_origin(
                                iterator, bt_common_func_status_string(status));
                }
 
+               clear_per_stream_state(iterator);
+
                switch (status) {
                case BT_FUNC_STATUS_OK:
                        break;
@@ -1982,6 +2164,8 @@ bt_message_iterator_seek_ns_from_origin(
                }
        }
 
+       clear_per_stream_state(iterator);
+
        /*
         * The following messages returned by the next method (including
         * post_auto_seek_next) must be after (or at) `ns_from_origin`.

diff --git a/src/lib/graph/message/iterator.h b/src/lib/graph/message/iterator.h
index 0825753438680ff23abb062dd0c3826359e94161..ed093633a5e208ebd162e3a209ded54f73669f67 100644 (file)
--- a/src/lib/graph/message/iterator.h
+++ b/src/lib/graph/message/iterator.h
@@ -152,6 +152,8 @@ struct bt_message_iterator {
                bt_uuid_t uuid;
        } clock_expectation;
 
+       BT_IF_DEV_MODE(GHashTable *per_stream_state);
+
        /*
         * Data necessary for auto seek (the seek-to-beginning then fast-forward
         * seek strategy).