From 1e03d33b36559cd925774ea25c627cefd43ce9ba Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 2 Dec 2020 17:36:05 -0500 Subject: [PATCH] Fix: sink.ctf.fs: fix logic of make_unique_stream_file_name The logic in make_unique_stream_file_name is wrong. It tries names as long as the candidate exists _and_ is named "metadata". The intent here is that we keep trying names as long as the candidate name names an already existing file _or_ is named "metadata". So the && should be a ||. The impact of this bug is that if two streams have the same name, they'll write to the same file, with troubling consequences (see bug 1279 [1]). Add a test where `sink.ctf.fs` writes a trace with two streams with the same name, and one stream named "metadata". [1] https://bugs.lttng.org/issues/1279 Change-Id: Ifaa30459574229aa5e607095f65033d2caae188f Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/4483 Tested-by: jenkins Reviewed-by: Philippe Proulx Reviewed-on: https://review.lttng.org/c/babeltrace/+/4492 --- src/plugins/ctf/fs-sink/fs-sink-stream.c | 2 +- tests/Makefile.am | 1 + .../sink.ctf.fs/stream-names/bt_plugin_foo.py | 50 +++++++++++ tests/plugins/sink.ctf.fs/Makefile.am | 3 +- tests/plugins/sink.ctf.fs/test_stream_names | 88 +++++++++++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 tests/data/plugins/sink.ctf.fs/stream-names/bt_plugin_foo.py create mode 100755 tests/plugins/sink.ctf.fs/test_stream_names diff --git a/src/plugins/ctf/fs-sink/fs-sink-stream.c b/src/plugins/ctf/fs-sink/fs-sink-stream.c index 93807d74..16d7eb38 100644 --- a/src/plugins/ctf/fs-sink/fs-sink-stream.c +++ b/src/plugins/ctf/fs-sink/fs-sink-stream.c @@ -120,7 +120,7 @@ GString *make_unique_stream_file_name(struct fs_sink_trace *trace, BT_ASSERT(name); - while (stream_file_name_exists(trace, name->str) && + while (stream_file_name_exists(trace, name->str) || strcmp(name->str, "metadata") == 0) { g_string_printf(name, "%s-%u", san_base->str, suffix); suffix++; diff --git a/tests/Makefile.am b/tests/Makefile.am index 02a93eb8..f310d11b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -107,6 +107,7 @@ TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_support_info TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_trace_info TESTS_PLUGINS += plugins/src.ctf.fs/query/test_query_metadata_info TESTS_PLUGINS += plugins/sink.ctf.fs/test_assume_single_trace +TESTS_PLUGINS += plugins/sink.ctf.fs/test_stream_names endif endif diff --git a/tests/data/plugins/sink.ctf.fs/stream-names/bt_plugin_foo.py b/tests/data/plugins/sink.ctf.fs/stream-names/bt_plugin_foo.py new file mode 100644 index 00000000..107be40d --- /dev/null +++ b/tests/data/plugins/sink.ctf.fs/stream-names/bt_plugin_foo.py @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Copyright (C) 2020 EfficiOS Inc. +# + +import bt2 + + +class TheSourceIterator(bt2._UserMessageIterator): + def __init__(self, config, port): + tc, sc, ec = port.user_data + + trace = tc() + + # Make two streams with the same name, to verify the stream filenames + # are de-duplicated properly. Make one with the name "metadata" to + # verify the resulting data file is not named "metadata". + stream1 = trace.create_stream(sc, name='the-stream') + stream2 = trace.create_stream(sc, name='the-stream') + stream3 = trace.create_stream(sc, name='metadata') + + self._msgs = [ + self._create_stream_beginning_message(stream1), + self._create_stream_beginning_message(stream2), + self._create_stream_beginning_message(stream3), + self._create_event_message(ec, stream1), + self._create_event_message(ec, stream2), + self._create_event_message(ec, stream3), + self._create_stream_end_message(stream1), + self._create_stream_end_message(stream2), + self._create_stream_end_message(stream3), + ] + + def __next__(self): + if len(self._msgs) == 0: + raise StopIteration + + return self._msgs.pop(0) + + +@bt2.plugin_component_class +class TheSource(bt2._UserSourceComponent, message_iterator_class=TheSourceIterator): + def __init__(self, config, params, obj): + tc = self._create_trace_class() + sc = tc.create_stream_class() + ec = sc.create_event_class(name='the-event') + self._add_output_port('out', user_data=(tc, sc, ec)) + + +bt2.register_plugin(__name__, "foo") diff --git a/tests/plugins/sink.ctf.fs/Makefile.am b/tests/plugins/sink.ctf.fs/Makefile.am index 76ce3c03..e8a0c019 100644 --- a/tests/plugins/sink.ctf.fs/Makefile.am +++ b/tests/plugins/sink.ctf.fs/Makefile.am @@ -1,4 +1,5 @@ SUBDIRS = succeed dist_check_SCRIPTS = \ - test_assume_single_trace + test_assume_single_trace \ + test_stream_names diff --git a/tests/plugins/sink.ctf.fs/test_stream_names b/tests/plugins/sink.ctf.fs/test_stream_names new file mode 100755 index 00000000..03ed4174 --- /dev/null +++ b/tests/plugins/sink.ctf.fs/test_stream_names @@ -0,0 +1,88 @@ +#!/bin/bash +# +# SPDX-License-Identifier: GPL-2.0-only +# +# Copyright (C) 2020 EfficiOS Inc. +# + +# This file tests corner cases related to stream names: +# +# - two streams with the same name +# - a stream named "metadata" + +SH_TAP=1 + +if [ "x${BT_TESTS_SRCDIR:-}" != "x" ]; then + UTILSSH="$BT_TESTS_SRCDIR/utils/utils.sh" +else + UTILSSH="$(dirname "$0")/../../utils/utils.sh" +fi + +# shellcheck source=../../utils/utils.sh +source "$UTILSSH" + +# Directory containing the Python test source. +data_dir="$BT_TESTS_DATADIR/plugins/sink.ctf.fs/stream-names" + +temp_stdout=$(mktemp) +temp_expected_stdout=$(mktemp) +temp_stderr=$(mktemp) +temp_output_dir=$(mktemp -d) +trace_dir="$temp_output_dir/trace" + +plan_tests 9 + +bt_cli "$temp_stdout" "$temp_stderr" \ + "--plugin-path=${data_dir}" \ + -c src.foo.TheSource \ + -c sink.ctf.fs -p "path=\"${temp_output_dir}\"" +ok "$?" "run babeltrace" + +# Check stdout. +if [ "$BT_OS_TYPE" = "mingw" ]; then + echo "Created CTF trace \`$(cygpath -m ${temp_output_dir})\\trace\`." > "$temp_expected_stdout" +else + echo "Created CTF trace \`${trace_dir}\`." > "$temp_expected_stdout" +fi +bt_diff "$temp_expected_stdout" "$temp_stdout" +ok "$?" "expected message on stdout" + +# Check stderr. +bt_diff "/dev/null" "$temp_stderr" +ok "$?" "stderr is empty" + +# Verify only the expected files exist. +files=("$trace_dir"/*) +num_files=${#files[@]} +is "$num_files" "4" "expected number of files in output directory" + +test -f "$trace_dir/metadata" +ok "$?" "metadata file exists" + +test -f "$trace_dir/metadata-0" +ok "$?" "metadata-0 file exists" + +test -f "$trace_dir/the-stream" +ok "$?" "the-stream file exists" + +test -f "$trace_dir/the-stream-0" +ok "$?" "the-stream-0 file exists" + +# Read back the output trace to make sure it's properly formed. +cat <<- 'END' > "$temp_expected_stdout" +the-event: +the-event: +the-event: +END +bt_diff_cli "$temp_expected_stdout" /dev/null "$trace_dir" +ok "$?" "read back output trace" + +rm -f "$temp_stdout" +rm -f "$temp_stderr" +rm -f "$temp_expected_stdout" +rm -f "$trace_dir/metadata" +rm -f "$trace_dir/metadata-0" +rm -f "$trace_dir/the-stream" +rm -f "$trace_dir/the-stream-0" +rmdir "$trace_dir" +rmdir "$temp_output_dir" -- 2.34.1