From: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Date: Thu, 4 Apr 2019 14:19:54 +0000 (-0400)
Subject: Fix: array and sequence field's 'elems' members can be left NULL
X-Git-Tag: v1.5.7~3
X-Git-Url: http://git.efficios.com/?a=commitdiff_plain;h=e7f1ad36b82bb8651c91c6c6faf8b92f145f32bc;hp=e7f1ad36b82bb8651c91c6c6faf8b92f145f32bc;p=babeltrace.git

Fix: array and sequence field's 'elems' members can be left NULL

Issue
---

The behaviour of a number of "rw" functions associated with array and
sequence field types differ when their element's declaration meets the
following criteria:
  - is an integer,
  - is byte-aligned,
  - is byte-sized,
  - is UTF-8 or ASCII encoded.

Those criteria are used to determine if the elements of either arrays
or sequences should be interpreted as characters.

1) The implementation of sequence and array definitions creation
   functions do not initialize their 'elems' member (a g_ptr_array),
   instead initializing a 'string' member (a g_string).

2) The 'ctf' format plug-in does not initialize the 'elems' array with
   the decoded integer definitions, instead only initializing the
   'string' member with the field's contents.

3) The 'ctf-text' format plug-in uses the internal headers to
   access the 'string' member of those definitions directly.

The 'string' member of both sequence and array definitions is meant as
a helper to allow the access to their contents in textual form.

However, while an array's content is made available under that form
through the public bt_ctf_get_char_array() function, there is no
equivalent accessor for the sequence type, as reported by a number of
users [1][2]. The 'ctf-text' format implementation works around this
limitation by making use of the internal headers to access the string
member directly.

Moreover, bypassing the creation and initialization of the 'elems'
member of both array and sequence definitions results in a crash when
bt_ctf_get_field_list() is used with these types when they contain
character elements, as reported on the mailing list [1].

Solution
---

This fix eliminates the bypass used by the definition creation
functions and 'ctf' format plug-in, ensuring that both the 'string'
and 'elems' members are initialized even if the elements fit the
"character" criteria.

This fixes the crash on a unchecked NULL pointer in
bt_ctf_get_field_list() reported in [1] when trying to access the
sequence's contents.

The checks for the various criteria that make an integer a
character have been moved to an internal function, bt_int_is_char()
since their (incorrect) duplication obscured the underlying problem.

For instance, a sequence's 'string' member is allocated if the
elements are ASCII or UTF-8 encoded integers, but only used when
the elements are byte-sized and byte-aligned (as opposed to the
intended behaviour in types/array.c).

With this fix applied, sequence elements can be accessed normally
through the existing bt_ctf_get_field_list() or bt_ctf_get_index()
functions.

Example:
```
/*
 * Print a sequence's content if it is text.
 * Error handling omitted.
 */
void print_sequence(const struct bt_ctf_event *event,
                const struct bt_definition *sequence_definition)
{
        int signedness, integer_len;
        unsigned int i, element_count;
        const struct bt_definition * const *elements;
        const struct bt_definition *element;
        const struct bt_declaration *element_declaration;
        enum ctf_string_encoding encoding;

        bt_ctf_get_field_list(event, sequence_definition, &elements,
                &element_count);
        if (element_count == 0) {
                return;
        }

        /* Is this a text sequence? */
        element = elements[0];
        element_declaration = bt_ctf_get_decl_from_def(element);
        if (bt_ctf_field_type(element_declaration) != CTF_TYPE_INTEGER) {
                /* Not a text sequence. */
                return;
        }

        signedness = bt_ctf_get_int_signedness(element_declaration);
        encoding = bt_ctf_get_encoding(element_declaration);
        integer_len = bt_ctf_get_int_len(element_declaration);
        if (integer_len != 8 ||
                (encoding != CTF_STRING_UTF8 && encoding != CTF_STRING_ASCII)) {
                /* Not a text sequence. */
                return;
        }

        putchar('"');
        for (i = 0; i < element_count; i++) {
                int val = signedness ?
                        bt_ctf_get_int64(elements[i]) :
                        bt_ctf_get_uint64(elements[i]);

                putchar(val);
        }
        putchar('"');
}
```

Notes
---

Since it is not possible for a user of the public API to determine the
alignment of a field in the ctf trace, it is not possible to check for
the criteria that make an array a "character array".

An array's element declaration could indicate that it is a byte-sized
and UTF8/ASCII encoded integer. Yet, the 'string' member could be left
NULL if the element's alignment is not '8'.

Hence, while it is possible to use the bt_ctf_get_char_array()
function on array definitions that look like "character arrays", users
should be careful in doing so.

bt_ctf_get_char_array() returning NULL should not be assumed to
indicate that an array is empty. Under such circumstances, reader code
should fall-back to using bt_ctf_get_field_list() to access the
array's contents, as shown in the example above.

[1] https://lists.lttng.org/pipermail/lttng-dev/2019-April/028704.html
[2] https://github.com/efficios/babeltrace/pull/98

Reported-by: romendmsft <romend@microsoft.com>
Reported-by: Milian Wolff <milian.wolff@kdab.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---