From ab003dc0397a13f72c8268c310c0559a06a87fb1 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 20 Feb 2023 15:40:35 -0500 Subject: [PATCH] Fix: ctf: verify that field class is int before calling ctf_field_class_as_int MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Some code paths call ctf_field_class_as_int functions before confirming that the field class is indeed an int (or enum, which is a sub-type of int). This causes assertion failures, because these functions assert that the field class they receive have the right type. One case can be hit using the invalid-sequence-length-field-class trace, included with this patch: $ /home/smarchi/build/babeltrace/tests/../src/cli/babeltrace2 -c sink.text.details -p with-trace-name=no,with-stream-name=no /home/smarchi/src/babeltrace/tests/data/ctf-traces/fail/invalid-sequence-length-field-class (╯°□°)╯︵ ┻━┻ /home/smarchi/src/babeltrace/src/plugins/ctf/common/metadata/ctf-meta.hpp:355: ctf_field_class_as_int(): Assertion `!fc || (fc->type == CTF_FIELD_CLASS_TYPE_INT || fc->type == CTF_FIELD_CLASS_TYPE_ENUM)` failed. This particular crash happens here: #4 0x00007ffff535de9d in bt_common_assert_failed (file=0x7ffff5374a60 "/home/smarchi/src/babeltrace/src/plugins/ctf/common/metadata/ctf-meta.hpp", line=355, func=0x7ffff5374a20 "ctf_field_class_as_int", assertion=0x7ffff53749a0 "!fc || (fc->type == CTF_FIELD_CLASS_TYPE_INT || fc->type == CTF_FIELD_CLASS_TYPE_ENUM)") at /home/smarchi/src/babeltrace/src/common/assert.c:40 #5 0x00007ffff5271cca in ctf_field_class_as_int (fc=0x603000002b60) at /home/smarchi/src/babeltrace/src/plugins/ctf/common/metadata/ctf-meta.hpp:355 #6 0x00007ffff527a54b in validate_target_field_path (target_field_path=0x7fffffffce60, target_fc=0x603000002b60, ctx=0x7fffffffd050) at /home/smarchi/src/babeltrace/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp:877 #7 0x00007ffff527bb7f in resolve_sequence_or_variant_field_class (fc=0x607000000950, ctx=0x7fffffffd050) at /home/smarchi/src/babeltrace/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp:969 In validate_target_field_path, when handing a sequence, we call ctf_field_class_as_int on target_fc (the length field class) before confirming it is really an int. Fix that by moving the call to ctf_field_class_as_int below that check. I went around and looked at all the uses of ctf_field_class_as_int, and fixed some more instances of the same problem. In some cases, I moved calls to ctf_field_class_as_int after BT_COMP_LOGT calls. While not necessary, my thinking is that should the assert in ctf_field_class_as_int ever fail for these calls, it could be able to see the output of the BT_COMP_LOGT, before crashing. Add a test with a sequence whose length specifier is not an integer. I also wrote an equivalent test for a variant whose selector is not an enumeration. It doesn't cause a crash, but I thought it would be a good test to have anyway, I don't think we exercise that. Change-Id: I0d375e9727572d97f129cdefdaad7bfa1a6102dc Reviewed-on: https://review.lttng.org/c/babeltrace/+/9528 Tested-by: jenkins Reviewed-by: Philippe Proulx CI-Build: Simon Marchi --- .../ctf/common/metadata/ctf-meta-resolve.cpp | 4 +-- src/plugins/ctf/common/metadata/ctf-meta.hpp | 19 ++++++------- .../common/metadata/visitor-generate-ir.cpp | 17 +++++------ src/plugins/ctf/common/msg-iter/msg-iter.cpp | 9 ++++-- .../metadata | 25 +++++++++++++++++ .../metadata | 28 +++++++++++++++++++ tests/plugins/src.ctf.fs/fail/test_fail | 12 +++++++- 7 files changed, 87 insertions(+), 27 deletions(-) create mode 100644 tests/data/ctf-traces/fail/invalid-sequence-length-field-class/metadata create mode 100644 tests/data/ctf-traces/fail/invalid-variant-selector-field-class/metadata diff --git a/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp b/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp index 5b65414e..fee1dda9 100644 --- a/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp +++ b/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp @@ -874,8 +874,6 @@ static int validate_target_field_path(struct ctf_field_path *target_field_path, break; case CTF_FIELD_CLASS_TYPE_SEQUENCE: { - struct ctf_field_class_int *int_fc = ctf_field_class_as_int(target_fc); - if (target_fc->type != CTF_FIELD_CLASS_TYPE_INT && target_fc->type != CTF_FIELD_CLASS_TYPE_ENUM) { _BT_COMP_OR_COMP_CLASS_LOGE_APPEND_CAUSE( @@ -886,6 +884,8 @@ static int validate_target_field_path(struct ctf_field_path *target_field_path, goto end; } + ctf_field_class_int *int_fc = ctf_field_class_as_int(target_fc); + if (int_fc->is_signed) { _BT_COMP_OR_COMP_CLASS_LOGE_APPEND_CAUSE( "Sequence field class's length field class is not an unsigned integer field class: " diff --git a/src/plugins/ctf/common/metadata/ctf-meta.hpp b/src/plugins/ctf/common/metadata/ctf-meta.hpp index ae20f881..6bf43756 100644 --- a/src/plugins/ctf/common/metadata/ctf-meta.hpp +++ b/src/plugins/ctf/common/metadata/ctf-meta.hpp @@ -871,22 +871,19 @@ static inline struct ctf_field_class_int * ctf_field_class_struct_borrow_member_int_field_class_by_name( struct ctf_field_class_struct *struct_fc, const char *name) { - struct ctf_field_class_int *int_fc = NULL; + ctf_field_class *member_fc = + ctf_field_class_struct_borrow_member_field_class_by_name(struct_fc, name); - int_fc = ctf_field_class_as_int( - ctf_field_class_struct_borrow_member_field_class_by_name(struct_fc, name)); - if (!int_fc) { - goto end; + if (!member_fc) { + return nullptr; } - if (int_fc->base.base.type != CTF_FIELD_CLASS_TYPE_INT && - int_fc->base.base.type != CTF_FIELD_CLASS_TYPE_ENUM) { - int_fc = NULL; - goto end; + if (member_fc->type != CTF_FIELD_CLASS_TYPE_INT && + member_fc->type != CTF_FIELD_CLASS_TYPE_ENUM) { + return nullptr; } -end: - return int_fc; + return ctf_field_class_as_int(member_fc); } static inline void _ctf_named_field_class_unescape_orig_name(struct ctf_named_field_class *named_fc) diff --git a/src/plugins/ctf/common/metadata/visitor-generate-ir.cpp b/src/plugins/ctf/common/metadata/visitor-generate-ir.cpp index 33fa1ccb..df757406 100644 --- a/src/plugins/ctf/common/metadata/visitor-generate-ir.cpp +++ b/src/plugins/ctf/common/metadata/visitor-generate-ir.cpp @@ -3298,21 +3298,21 @@ static int auto_map_field_to_trace_clock_class(struct ctf_visitor_generate_ir *c struct ctf_field_class *fc) { struct ctf_clock_class *clock_class_to_map_to = NULL; - struct ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); - int ret = 0; uint64_t clock_class_count; if (!fc) { - goto end; + return 0; } if (fc->type != CTF_FIELD_CLASS_TYPE_INT && fc->type != CTF_FIELD_CLASS_TYPE_ENUM) { - goto end; + return 0; } + ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); + if (int_fc->mapped_clock_class) { /* Already mapped */ - goto end; + return 0; } clock_class_count = ctx->ctf_tc->clock_classes->len; @@ -3328,7 +3328,6 @@ static int auto_map_field_to_trace_clock_class(struct ctf_visitor_generate_ir *c BT_ASSERT(clock_class_to_map_to); clock_class_to_map_to->frequency = UINT64_C(1000000000); g_string_assign(clock_class_to_map_to->name, "default"); - BT_ASSERT(ret == 0); g_ptr_array_add(ctx->ctf_tc->clock_classes, clock_class_to_map_to); break; case 1: @@ -3346,15 +3345,13 @@ static int auto_map_field_to_trace_clock_class(struct ctf_visitor_generate_ir *c _BT_COMP_OR_COMP_CLASS_LOGE_APPEND_CAUSE( "Timestamp field found with no mapped clock class, " "but there's more than one clock class in the trace at this point."); - ret = -1; - goto end; + return -1; } BT_ASSERT(clock_class_to_map_to); int_fc->mapped_clock_class = clock_class_to_map_to; -end: - return ret; + return 0; } static int auto_map_fields_to_trace_clock_class(struct ctf_visitor_generate_ir *ctx, diff --git a/src/plugins/ctf/common/msg-iter/msg-iter.cpp b/src/plugins/ctf/common/msg-iter/msg-iter.cpp index e4190724..7b507ef3 100644 --- a/src/plugins/ctf/common/msg-iter/msg-iter.cpp +++ b/src/plugins/ctf/common/msg-iter/msg-iter.cpp @@ -1803,13 +1803,14 @@ static enum bt_bfcr_status bfcr_unsigned_int_cb(uint64_t value, struct ctf_field enum bt_bfcr_status status = BT_BFCR_STATUS_OK; bt_field *field = NULL; - ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); BT_COMP_LOGT("Unsigned integer function called from BFCR: " "msg-it-addr=%p, bfcr-addr=%p, fc-addr=%p, " "fc-type=%d, fc-in-ir=%d, value=%" PRIu64, msg_it, msg_it->bfcr, fc, fc->type, fc->in_ir, value); + ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); + if (G_LIKELY(int_fc->meaning == CTF_FIELD_CLASS_MEANING_NONE)) { goto update_def_clock; } @@ -1890,13 +1891,14 @@ static enum bt_bfcr_status bfcr_unsigned_int_char_cb(uint64_t value, struct ctf_ bt_self_component *self_comp = msg_it->self_comp; enum bt_bfcr_status status = BT_BFCR_STATUS_OK; bt_field *string_field = NULL; - ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); char str[2] = {'\0', '\0'}; BT_COMP_LOGT("Unsigned integer character function called from BFCR: " "msg-it-addr=%p, bfcr-addr=%p, fc-addr=%p, " "fc-type=%d, fc-in-ir=%d, value=%" PRIu64, msg_it, msg_it->bfcr, fc, fc->type, fc->in_ir, value); + + ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); BT_ASSERT_DBG(int_fc->meaning == CTF_FIELD_CLASS_MEANING_NONE); BT_ASSERT_DBG(!int_fc->mapped_clock_class); BT_ASSERT_DBG(int_fc->storing_index < 0); @@ -1938,12 +1940,13 @@ static enum bt_bfcr_status bfcr_signed_int_cb(int64_t value, struct ctf_field_cl enum bt_bfcr_status status = BT_BFCR_STATUS_OK; bt_field *field = NULL; ctf_msg_iter *msg_it = (ctf_msg_iter *) data; - ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); BT_COMP_LOGT("Signed integer function called from BFCR: " "msg-it-addr=%p, bfcr-addr=%p, fc-addr=%p, " "fc-type=%d, fc-in-ir=%d, value=%" PRId64, msg_it, msg_it->bfcr, fc, fc->type, fc->in_ir, value); + + ctf_field_class_int *int_fc = ctf_field_class_as_int(fc); BT_ASSERT_DBG(int_fc->meaning == CTF_FIELD_CLASS_MEANING_NONE); if (G_UNLIKELY(int_fc->storing_index >= 0)) { diff --git a/tests/data/ctf-traces/fail/invalid-sequence-length-field-class/metadata b/tests/data/ctf-traces/fail/invalid-sequence-length-field-class/metadata new file mode 100644 index 00000000..d56b8c80 --- /dev/null +++ b/tests/data/ctf-traces/fail/invalid-sequence-length-field-class/metadata @@ -0,0 +1,25 @@ +/* CTF 1.8 */ +typealias integer { size = 8; align = 8; signed = false; } := uint8_t; + +trace { + major = 1; + minor = 8; + byte_order = le; +}; + +stream { + event.header := struct { + uint8_t id; + }; +}; + +event { + name = ze_event; + id = 1; + fields := struct { + struct { + uint8_t hello; + } len; + uint8_t x[len]; + }; +}; diff --git a/tests/data/ctf-traces/fail/invalid-variant-selector-field-class/metadata b/tests/data/ctf-traces/fail/invalid-variant-selector-field-class/metadata new file mode 100644 index 00000000..3cfa3422 --- /dev/null +++ b/tests/data/ctf-traces/fail/invalid-variant-selector-field-class/metadata @@ -0,0 +1,28 @@ +/* CTF 1.8 */ +typealias integer { size = 8; align = 8; signed = false; } := uint8_t; + +trace { + major = 1; + minor = 8; + byte_order = le; +}; + +stream { + event.header := struct { + uint8_t id; + }; +}; + +event { + name = ze_event; + id = 1; + fields := struct { + struct { + uint8_t hello; + } selector; + variant { + uint8_t a; + uint8_t b; + } v; + }; +}; diff --git a/tests/plugins/src.ctf.fs/fail/test_fail b/tests/plugins/src.ctf.fs/fail/test_fail index 1c75a0d2..443db0c5 100755 --- a/tests/plugins/src.ctf.fs/fail/test_fail +++ b/tests/plugins/src.ctf.fs/fail/test_fail @@ -49,7 +49,7 @@ test_fail() { } -plan_tests 12 +plan_tests 20 test_fail \ "invalid-packet-size/trace" \ @@ -66,4 +66,14 @@ test_fail \ "/dev/null" \ "^ At line 3 in metadata stream: syntax error, unexpected CTF_RSBRAC: token=\"]\"" +test_fail \ + "invalid-sequence-length-field-class" \ + "/dev/null" \ + "Sequence field class's length field class is not an unsigned integer field class: " + +test_fail \ + "invalid-variant-selector-field-class" \ + "/dev/null" \ + "Variant field class's tag field class is not an enumeration field class: " + rm -f "${stdout_file}" "${stderr_file}" -- 2.34.1