Fix: ctf: verify that field class is int before calling ctf_field_class_as_int

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 <jenkins@lttng.org>
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
CI-Build: Simon Marchi <simon.marchi@efficios.com>

diff --git a/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp b/src/plugins/ctf/common/metadata/ctf-meta-resolve.cpp
index 5b65414ee6c3dffe0d837ba60448617c996ff5f1..fee1dda94a4a73d26bffcbb77f9fcfdf24288a85 100644 (file)
--- 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 ae20f8814a6053276a91ce30fbd2ee758e75b03e..6bf43756e0a0033694e5243b88fed1b241e966ec 100644 (file)
--- 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 33fa1ccb3a2b076aa2cc80ccb86f06da19bfa8a7..df7574060c5c023818cfff5bef607db94a35e2f3 100644 (file)
--- 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 e4190724dc152b734b6b7e4cb07d04de73c0e859..7b507ef3b3b39d5f9d84423016ecce8907e2225a 100644 (file)
--- 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 (file)
index 0000000..d56b8c8
--- /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 (file)
index 0000000..3cfa342
--- /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<selector> {
+                       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 1c75a0d28a6391fafa48ffe390b6075cb908732d..443db0c507067be9c069a3f29ad2d785ca47de9c 100755 (executable)
--- 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}"