Fix: to enable access to sequence_text field type data from API. #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, The 1.x release series is in maintenance mode at this point. Is there a reason you can't use Babeltrace 2's API (currently master)? Thanks. |
|
Hi, At the time we took dependency on the Babeltrace API I think version 2 wasn't out from what I remember. Thanks, |
|
It is not out yet, but it is in pre-release phase (meaning the final release is coming soon). |
|
I see, I will try taking a look at the code in master when I have some time to see if we can change our code to the API v2. It would be great if you could point me to at least one example API so I can get started more easily. Thanks. |
|
~1.5y later and babeltrace is still not out yet the issue fixed by this patch still remains. Please merge this and release a new version of the stable API - it's packaged by many distros! |
milianw
added a commit
to KDABLabs/ctf2ctf
that referenced
this pull request
Apr 2, 2019
Parse the tracef msg (requires a patched babeltrace API with efficios/babeltrace#98) and take the first word in there as the 'new' event name if it matches one of our begin/end patterns. Fixes: #12
jgalar
added a commit
that referenced
this pull request
Apr 4, 2019
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] #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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently reading from sequence_text field type is not exposed by the API (at least I couldn't find any possible way of doing this).
sequence_definition->string is properly allocated and populated when sequence_text is read from trace events.
However, bt_ctf_get_string does not consider SEQUENCE_TYPE and does not return the string in such type of field.