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 <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/4483
Tested-by: jenkins <jenkins@lttng.org>
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>

diff --git a/src/plugins/ctf/fs-sink/fs-sink-stream.c b/src/plugins/ctf/fs-sink/fs-sink-stream.c
index 1c83562a003c0e249ece3af1bebb8312d5681dd1..b14e6d556d9e02088d58619548f3ddcb5890c0c0 100644 (file)
--- a/src/plugins/ctf/fs-sink/fs-sink-stream.c
+++ b/src/plugins/ctf/fs-sink/fs-sink-stream.c
@@ -104,7 +104,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 7077b6e980072791b9d72775fd82795d60c471ca..b25852facac92bbc190b9ffd807e36d5456b3ac2 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -110,6 +110,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 (file)
index 0000000..107be40
--- /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 10f5bf0823f7c9822c83e56ff4d9b6d1256c4771..fb488d066b5624cc01c37b6881620495ec1b9d17 100644 (file)
--- a/tests/plugins/sink.ctf.fs/Makefile.am
+++ b/tests/plugins/sink.ctf.fs/Makefile.am
@@ -3,4 +3,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 (executable)
index 0000000..03ed417
--- /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"