Fix: lib: handle BT_FUNC_STATUS_NOT_FOUND in bt_plugin_so_create_all_from_sections

When an SO plugin's init function returns an error and
fail_on_load_error is false, we hit an assert:

    (╯°□°)╯︵ ┻━┻  /home/smarchi/src/babeltrace/src/lib/plugin/plugin-so.c:1351: bt_plugin_so_create_all_from_sections(): Assertion `!plugin` failed.

When an error happens in the plugin's init function, bt_plugin_so_init returns BT_FUNC_STATUS_NOT_FOUND.  We leave the `plugin` variable set, which triggers the assert.

Fix this by putting/resetting `plugin` in that case.  Since there was an
error initializing the plugin, we won't return it to the user, so I
think that release it is the right thing to do here.

Add a test with a plugin whose init function fail.  Test both the
fail_on_load_error true and false cases.

Change-Id: I8e219f728a4b58d0e32307f907243892a02fdccf
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reported-by: Brice Videau <bvideau@anl.gov>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/11679
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
Reviewed-by: Brice Videau <bvideau@anl.gov>
Tested-by: jenkins <jenkins@lttng.org>

diff --git a/configure.ac b/configure.ac
index 53b92cbcf64b117cc9c460d6f95b5f93efecb196..f1ef5b148f503bdb475ea865ea029c39bbfefe04 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -823,6 +823,7 @@ AC_CONFIG_FILES([
   tests/ctf-writer/Makefile
   tests/lib/Makefile
   tests/lib/conds/Makefile
+  tests/lib/test-plugin-init-fail-plugin/Makefile
   tests/lib/test-plugins-plugins/Makefile
   tests/lib/utils/Makefile
   tests/Makefile

diff --git a/src/lib/plugin/plugin-so.c b/src/lib/plugin/plugin-so.c
index a4622c38f740bd2a97782ac9ffc11f92ed958ae5..698d03d0a0acb61ab667bcddcc6fb8b0a6d83697 100644 (file)
--- a/src/lib/plugin/plugin-so.c
+++ b/src/lib/plugin/plugin-so.c
@@ -734,6 +734,12 @@ int bt_plugin_so_init(struct bt_plugin *plugin,
                                status = init_status;
                                goto end;
                        } else {
+                               /*
+                                * Since we don't return an error,
+                                * there's no way to communicate this
+                                * error to the caller.
+                                */
+                               bt_current_thread_clear_error();
                                BT_LIB_LOGW(
                                        "User's plugin initialization function failed: "
                                        "status=%s",
@@ -1335,6 +1341,12 @@ int bt_plugin_so_create_all_from_sections(
                        /* Add to plugin set */
                        bt_plugin_set_add_plugin(*plugin_set_out, plugin);
                        BT_OBJECT_PUT_REF_AND_RESET(plugin);
+               } else if (status == BT_FUNC_STATUS_NOT_FOUND) {
+                       /*
+                        * There was an error initializing the plugin,
+                        * but `fail_on_load_error` is false.
+                        */
+                       BT_OBJECT_PUT_REF_AND_RESET(plugin);
                } else if (status < 0) {
                        /*
                         * bt_plugin_so_init() handles

diff --git a/tests/lib/Makefile.am b/tests/lib/Makefile.am
index 9392f9005441c4c85f867f8aeff9b06d25e3dbe2..721244fb3e112a78f2e4acc1f89dbf2ceab2e6ab 100644 (file)
--- a/tests/lib/Makefile.am
+++ b/tests/lib/Makefile.am
@@ -52,11 +52,19 @@ test_remove_destruction_listener_in_destruction_listener_SOURCES = \
        test-remove-destruction-listener-in-destruction-listener.c
 
 if !ENABLE_BUILT_IN_PLUGINS
-noinst_PROGRAMS += test-plugins
-test_plugins_LDADD = $(COMMON_TEST_LDADD) \
-       $(top_builddir)/src/lib/libbabeltrace2.la
+noinst_PROGRAMS += test-plugins test-plugin-init-fail
+SUBDIRS += test-plugins-plugins test-plugin-init-fail-plugin
+
 test_plugins_SOURCES = test-plugins.c
-SUBDIRS += test-plugins-plugins
+test_plugins_LDADD = \
+       $(COMMON_TEST_LDADD) \
+       $(top_builddir)/src/lib/libbabeltrace2.la
+
+test_plugin_init_fail_SOURCES = test-plugin-init-fail.cpp
+test_plugin_init_fail_LDADD = \
+       $(COMMON_TEST_LDADD) \
+       $(top_builddir)/src/cpp-common/vendor/fmt/libfmt.la \
+       $(top_builddir)/src/lib/libbabeltrace2.la
 endif
 
 dist_check_SCRIPTS = test-plugins.sh test-fields.sh

diff --git a/tests/lib/test-plugin-init-fail-plugin/Makefile.am b/tests/lib/test-plugin-init-fail-plugin/Makefile.am
new file mode 100644 (file)
index 0000000..fb20778
--- /dev/null
+++ b/tests/lib/test-plugin-init-fail-plugin/Makefile.am
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: MIT
+
+noinst_LTLIBRARIES = plugin-init-fail.la
+
+plugin_init_fail_la_SOURCES = plugin-init-fail.cpp
+plugin_init_fail_la_LDFLAGS = \
+       $(AM_LDFLAGS) \
+       $(LT_NO_UNDEFINED) \
+       -rpath / -avoid-version -module $(LD_NOTEXT)
+plugin_init_fail_la_LIBADD = \
+       $(top_builddir)/src/lib/libbabeltrace2.la

diff --git a/tests/lib/test-plugin-init-fail-plugin/plugin-init-fail.cpp b/tests/lib/test-plugin-init-fail-plugin/plugin-init-fail.cpp
new file mode 100644 (file)
index 0000000..44bc117
--- /dev/null
+++ b/tests/lib/test-plugin-init-fail-plugin/plugin-init-fail.cpp
@@ -0,0 +1,21 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2024 EfficiOS, Inc.
+ */
+
+#include <babeltrace2/babeltrace.h>
+
+static bt_plugin_initialize_func_status plugin_init(bt_self_plugin *)
+{
+    BT_CURRENT_THREAD_ERROR_APPEND_CAUSE_FROM_UNKNOWN("plugin-init-fail",
+                                                      "This is the error message");
+    return BT_PLUGIN_INITIALIZE_FUNC_STATUS_ERROR;
+}
+
+BT_PLUGIN_MODULE();
+BT_PLUGIN(test_init_fail);
+BT_PLUGIN_DESCRIPTION("Babeltrace plugin with init function that fails");
+BT_PLUGIN_AUTHOR("Sophie Couturier");
+BT_PLUGIN_LICENSE("Beerware");
+BT_PLUGIN_INITIALIZE_FUNC(plugin_init);

diff --git a/tests/lib/test-plugin-init-fail.cpp b/tests/lib/test-plugin-init-fail.cpp
new file mode 100644 (file)
index 0000000..8d8fd41
--- /dev/null
+++ b/tests/lib/test-plugin-init-fail.cpp
@@ -0,0 +1,80 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2024 EfficiOS, Inc.
+ */
+
+#include <cpp-common/bt2/plugin.hpp>
+
+#include <cpp-common/vendor/fmt/format.h>
+
+#include <babeltrace2/babeltrace.h>
+
+#include "cpp-common/bt2c/c-string-view.hpp"
+
+#include "tap/tap.h"
+
+namespace {
+
+void testFailOnLoadErrorTrue(const char * const pluginDir)
+{
+    plan_tests(1);
+
+    try {
+        bt2::findAllPluginsFromDir(pluginDir, false, true);
+        bt_common_abort();
+    } catch (const bt2::Error& exc) {
+        fmt::print("{}\n", exc.what());
+
+        const auto error = bt_current_thread_take_error();
+
+        /*
+         * The last error cause must be the one which the initialization
+         * function of our plugin appended.
+         */
+        const auto cause = bt_error_borrow_cause_by_index(error, 0);
+        const bt2c::CStringView msg {bt_error_cause_get_message(cause)};
+
+        ok(msg == "This is the error message", "Message of error cause 0 is expected");
+        bt_error_release(error);
+    }
+}
+
+void testFailOnLoadErrorFalse(const char * const pluginDir)
+{
+    plan_tests(1);
+
+    const auto plugins = bt2::findAllPluginsFromDir(pluginDir, false, false);
+
+    ok(!plugins, "No plugin set returned");
+}
+
+} /* namespace */
+
+int main(const int argc, const char ** const argv)
+{
+    if (argc != 3) {
+        fmt::print(stderr,
+                   "Usage: {} INIT-FAIL-PLUGIN-DIR FAIL-ON-LOAD-ERROR\n"
+                   "\n"
+                   "FAIL-ON-LOAD-ERROR must be `yes` or `no`\n",
+                   argv[0]);
+        return 1;
+    }
+
+    const auto pluginDir = argv[1];
+    const bt2c::CStringView failOnLoadErrorStr {argv[2]};
+
+    if (failOnLoadErrorStr == "yes") {
+        testFailOnLoadErrorTrue(pluginDir);
+    } else if (failOnLoadErrorStr == "no") {
+        testFailOnLoadErrorFalse(pluginDir);
+    } else {
+        fmt::print(stderr,
+                   "ERROR: Invalid value `{}` for FAIL-ON-LOAD-ERROR (expecting `yes` or `no`).\n",
+                   failOnLoadErrorStr);
+        return 1;
+    }
+
+    return exit_status();
+}

diff --git a/tests/lib/test-plugin-init-fail.sh b/tests/lib/test-plugin-init-fail.sh
new file mode 100755 (executable)
index 0000000..661023a
--- /dev/null
+++ b/tests/lib/test-plugin-init-fail.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+#
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Copyright (C) 2024 EfficiOS, Inc.
+#
+
+if [ -n "${BT_TESTS_SRCDIR:-}" ]; then
+       UTILSSH="$BT_TESTS_SRCDIR/utils/utils.sh"
+else
+       UTILSSH="$(dirname "$0")/../utils/utils.sh"
+fi
+
+# shellcheck source=../utils/utils.sh
+source "$UTILSSH"
+
+ret=0
+
+"${BT_TESTS_BUILDDIR}/lib/test-plugin-init-fail" "${BT_TESTS_BUILDDIR}/lib/test-plugin-init-fail-plugin/.libs" yes || ret=1
+"${BT_TESTS_BUILDDIR}/lib/test-plugin-init-fail" "${BT_TESTS_BUILDDIR}/lib/test-plugin-init-fail-plugin/.libs" no || ret=1
+
+exit $ret