ctf: grow stored_values array when necessary
authorSimon Marchi <simon.marchi@efficios.com>
Tue, 17 Oct 2023 19:33:37 +0000 (15:33 -0400)
committerPhilippe Proulx <eeppeliteloop@gmail.com>
Fri, 20 Oct 2023 23:29:14 +0000 (19:29 -0400)
The CTF message iterator accesses the `stored_values` array out of
bounds in the following situation:

 - In the context of a src.ctf.lttng-live source, a ctf_trace_class gets
   created from some metadata.
 - At this point, ctf_trace_class->stored_value_count indicates that a
   certain number of stored values are necessary given the metadata
   parsed up to now.
 - The message iterators are created with `stored_values` arrays of that
   size.
 - The source receives more metadata, which requires more stored
   values.
 - The message iterator reads some event described by the new metadata,
   that requires the use of a stored value.
 - Since the stored value arrays have not been resized to reflect the
   necessary number of stored value considering the new metadata, the
   message iterator tries to store a value past the end of the array.

Fix this by ensuring the `stored_values` array is large enough before
storing a value in it.

Add a test with a hand-crafted trace that replicates the scenario
described in the simplest manner possible:

 - send a bit of metadata
 - send a bit of data that uses that metadata
 - send a bit more metadata (that requires a new stored value)
 - send a bit of data that uses that new metadata

Without the fix, we get (when babeltrace is built in debug mode):

    (╯°□°)╯︵ ┻━┻  /home/smarchi/src/babeltrace/src/plugins/ctf/common/msg-iter/msg-iter.cpp:1865: bfcr_unsigned_int_cb(): Assertion `(uint64_t) int_fc->storing_index < msg_it->stored_values->len` failed.

... showing the out of bounds array access.

Change-Id: I78e3ca57ac6cae1959425df3c8ffdbfeb534f348
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/10866
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
Tested-by: jenkins <jenkins@lttng.org>
src/plugins/ctf/common/msg-iter/msg-iter.cpp
tests/data/ctf-traces/live/stored_values.mctf [new file with mode: 0644]
tests/data/plugins/src.ctf.lttng-live/stored_values.expect [new file with mode: 0644]
tests/data/plugins/src.ctf.lttng-live/stored_values.json [new file with mode: 0644]
tests/plugins/src.ctf.lttng-live/test_live

index 4efc5ed61b9b6e6786750cbed3461cd3a1f40d6d..5f38ec6c0dd5ebdac1397e228086d07558d7840b 100644 (file)
@@ -1793,6 +1793,20 @@ end:
                  msg_it->default_clock_snapshot);
 }
 
+/*
+ * Ensure the message iterator's `stored_values` array is large enough to
+ * accomodate `storing_index`.
+ *
+ * We may need more slots in the array than initially allocated if more
+ * metadata arrives along the way.
+ */
+static void ensure_stored_values_size(ctf_msg_iter *msg_it, uint64_t storing_index)
+{
+    if (G_UNLIKELY(storing_index >= msg_it->stored_values->len)) {
+        g_array_set_size(msg_it->stored_values, msg_it->meta.tc->stored_value_count);
+    }
+}
+
 static enum bt_bfcr_status bfcr_unsigned_int_cb(uint64_t value, struct ctf_field_class *fc,
                                                 void *data)
 {
@@ -1862,6 +1876,7 @@ update_def_clock:
     }
 
     if (G_UNLIKELY(int_fc->storing_index >= 0)) {
+        ensure_stored_values_size(msg_it, int_fc->storing_index);
         bt_g_array_index(msg_it->stored_values, uint64_t, (uint64_t) int_fc->storing_index) = value;
     }
 
@@ -1948,6 +1963,7 @@ static enum bt_bfcr_status bfcr_signed_int_cb(int64_t value, struct ctf_field_cl
     BT_ASSERT_DBG(int_fc->meaning == CTF_FIELD_CLASS_MEANING_NONE);
 
     if (G_UNLIKELY(int_fc->storing_index >= 0)) {
+        ensure_stored_values_size(msg_it, int_fc->storing_index);
         bt_g_array_index(msg_it->stored_values, uint64_t, (uint64_t) int_fc->storing_index) =
             (uint64_t) value;
     }
diff --git a/tests/data/ctf-traces/live/stored_values.mctf b/tests/data/ctf-traces/live/stored_values.mctf
new file mode 100644 (file)
index 0000000..0f617f7
--- /dev/null
@@ -0,0 +1,92 @@
+--- metadata
+/* CTF 1.8 */
+
+typealias integer { size = 8; align = 8; signed = false; } := uint8_t;
+
+trace {
+       major = 1;
+       minor = 8;
+       byte_order = le;
+};
+
+struct packet_context {
+       uint8_t timestamp_begin;
+       uint8_t timestamp_end;
+       uint8_t content_size;
+       uint8_t packet_size;
+};
+
+struct event_header {
+       uint8_t id;
+       uint8_t timestamp;
+};
+
+stream {
+       event.header := struct event_header;
+       packet.context := struct packet_context;
+};
+
+event {
+       name = "event1";
+       id = 1;
+       fields := struct {
+               uint8_t len;
+               uint8_t seq[len];
+       };
+};
+
+event {
+       name = "event2";
+       id = 2;
+       fields := struct {
+               uint8_t len;
+               uint8_t seq[len];
+       };
+};
+
+--- channel0_0
+!macro packet(ts_beg, event_id)
+  <beg>
+  [         ts_beg : 8] # timestamp begin
+  [     ts_beg + 1 : 8] # timestamp end
+  [8 * (end - beg) : 8] # content size in bits
+  [8 * (end - beg) : 8] # packet size in bits
+
+  [       event_id : 8] # event id
+  [         ts_beg : 8] # timestamp
+  [              0 : 8] # len field
+  <end>
+!end
+
+{ p1_ts = 10 }
+{ p2_ts = 20 }
+
+<p1>
+m:packet(p1_ts, 1)
+<p1_end>
+
+<p2>
+m:packet(p2_ts, 2)
+<p2_end>
+
+--- index/channel0_0.idx
+!be
+
+[0xC1F1DCC1 : 32] # Magic number
+[         1 : 32] # Major
+[         0 : 32] # Minor
+[        56 : 32] # Index entry size (56 bytes)
+
+# Packet 1
+!macro entry(beg_label, end_label, ts_beg)
+  [                  beg_label : 64] # offset in bytes
+  [8 * (end_label - beg_label) : 64] # total size in bits
+  [8 * (end_label - beg_label) : 64] # content size in bits
+  [                     ts_beg : 64] # timestamp begin
+  [                 ts_beg + 1 : 64] # timestamp end
+  [                          0 : 64] # events discarded
+  [                          0 : 64] # stream class id
+!end
+
+m:entry(p1, p1_end, p1_ts)
+m:entry(p2, p2_end, p2_ts)
diff --git a/tests/data/plugins/src.ctf.lttng-live/stored_values.expect b/tests/data/plugins/src.ctf.lttng-live/stored_values.expect
new file mode 100644 (file)
index 0000000..6b5004d
--- /dev/null
@@ -0,0 +1,65 @@
+Trace class:
+  Stream class (ID 0):
+    Supports packets: Yes
+    Packets have beginning default clock snapshot: Yes
+    Packets have end default clock snapshot: Yes
+    Supports discarded events: No
+    Supports discarded packets: No
+    Default clock class:
+      Name: default
+      Frequency (Hz): 1,000,000,000
+      Precision (cycles): 0
+      Offset (s): 0
+      Offset (cycles): 0
+      Origin is Unix epoch: No
+    Event class `event1` (ID 1):
+      Payload field class: Structure (2 members):
+        len: Unsigned integer (8-bit, Base 10)
+        seq: Dynamic array (with length field) (Length field path [Event payload: 0]):
+          Element: Unsigned integer (8-bit, Base 10)
+    Event class `event2` (ID 2):
+      Payload field class: Structure (2 members):
+        len: Unsigned integer (8-bit, Base 10)
+        seq: Dynamic array (with length field) (Length field path [Event payload: 0]):
+          Element: Unsigned integer (8-bit, Base 10)
+
+[Unknown]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Stream beginning:
+  Name: stream-1
+  Trace:
+    Stream (ID 1, Class ID 0)
+
+[10 cycles, 10 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Packet beginning
+
+[10 cycles, 10 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Event `event1` (Class ID 1):
+  Payload:
+    len: 0
+    seq: Empty
+
+[11 cycles, 11 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Packet end
+
+[20 cycles, 20 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Packet beginning
+
+[20 cycles, 20 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Event `event2` (Class ID 2):
+  Payload:
+    len: 0
+    seq: Empty
+
+[21 cycles, 21 ns from origin]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Packet end
+
+[Unknown]
+{Trace 0, Stream class ID 0, Stream ID 1}
+Stream end
diff --git a/tests/data/plugins/src.ctf.lttng-live/stored_values.json b/tests/data/plugins/src.ctf.lttng-live/stored_values.json
new file mode 100644 (file)
index 0000000..9316168
--- /dev/null
@@ -0,0 +1,25 @@
+[
+    {
+        "name": "stored_values",
+        "id": 2,
+        "hostname": "hostname",
+        "live-timer-freq": 1,
+        "client-count": 0,
+        "traces": [
+            {
+                "path": "stored_values",
+                "metadata-sections": [
+                    {
+                        "line": 1,
+                        "timestamp": 1
+                    },
+                    "empty",
+                    {
+                        "line": 37,
+                        "timestamp": 20
+                    }
+                ]
+            }
+        ]
+    }
+]
index 301a6a2042f789a6c21310a161cb5c4d8b61c2e5..e9703fbbe18c13e58585e0017b48f346a9108d72 100755 (executable)
@@ -372,7 +372,28 @@ test_split_metadata() {
                "$expected_stderr" "$trace_dir_native" "${server_args[@]}"
 }
 
-plan_tests 16
+test_stored_values() {
+       # Split metadata, where the new metadata requires additional stored
+       # value slots in CTF message iterators.
+       local test_text="split metadata requiring additionnal stored values"
+       local cli_args_template="-i lttng-live net://localhost:@PORT@/host/hostname/stored_values -c sink.text.details"
+       local server_args=("$test_data_dir/stored_values.json")
+       local expected_stdout="${test_data_dir}/stored_values.expect"
+       local expected_stderr="/dev/null"
+       local tmp_dir
+
+       tmp_dir=$(mktemp -d -t 'test_stored_value.XXXXXXX')
+
+       # Generate test trace.
+       gen_mctf_trace "${trace_dir}/live/stored_values.mctf" "$tmp_dir/stored_values"
+
+       run_test "$test_text" "$cli_args_template" "$expected_stdout" \
+               "$expected_stderr" "$tmp_dir" "${server_args[@]}"
+
+       rm -rf "$tmp_dir"
+}
+
+plan_tests 18
 
 test_list_sessions
 test_base
@@ -381,3 +402,4 @@ test_rate_limited
 test_compare_to_ctf_fs
 test_inactivity_discarded_packet
 test_split_metadata
+test_stored_values
This page took 0.03125 seconds and 4 git commands to generate.