From 839df1da87b7bdaac5ebfd4e798ea769a94843b2 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 17 Oct 2023 15:33:37 -0400 Subject: [PATCH] ctf: grow stored_values array when necessary MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Reviewed-on: https://review.lttng.org/c/babeltrace/+/10866 Reviewed-by: Philippe Proulx Tested-by: jenkins --- src/plugins/ctf/common/msg-iter/msg-iter.cpp | 16 ++++ tests/data/ctf-traces/live/stored_values.mctf | 92 +++++++++++++++++++ .../src.ctf.lttng-live/stored_values.expect | 65 +++++++++++++ .../src.ctf.lttng-live/stored_values.json | 25 +++++ tests/plugins/src.ctf.lttng-live/test_live | 24 ++++- 5 files changed, 221 insertions(+), 1 deletion(-) create mode 100644 tests/data/ctf-traces/live/stored_values.mctf create mode 100644 tests/data/plugins/src.ctf.lttng-live/stored_values.expect create mode 100644 tests/data/plugins/src.ctf.lttng-live/stored_values.json diff --git a/src/plugins/ctf/common/msg-iter/msg-iter.cpp b/src/plugins/ctf/common/msg-iter/msg-iter.cpp index 4efc5ed6..5f38ec6c 100644 --- a/src/plugins/ctf/common/msg-iter/msg-iter.cpp +++ b/src/plugins/ctf/common/msg-iter/msg-iter.cpp @@ -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 index 00000000..0f617f7c --- /dev/null +++ b/tests/data/ctf-traces/live/stored_values.mctf @@ -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) + + [ 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 + +{ p1_ts = 10 } +{ p2_ts = 20 } + + +m:packet(p1_ts, 1) + + + +m:packet(p2_ts, 2) + + +--- 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 index 00000000..6b5004d5 --- /dev/null +++ b/tests/data/plugins/src.ctf.lttng-live/stored_values.expect @@ -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 index 00000000..93161686 --- /dev/null +++ b/tests/data/plugins/src.ctf.lttng-live/stored_values.json @@ -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 + } + ] + } + ] + } +] diff --git a/tests/plugins/src.ctf.lttng-live/test_live b/tests/plugins/src.ctf.lttng-live/test_live index 301a6a20..e9703fbb 100755 --- a/tests/plugins/src.ctf.lttng-live/test_live +++ b/tests/plugins/src.ctf.lttng-live/test_live @@ -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 -- 2.34.1