From 1a9956b9616ccb5b80f6c5db4bcbe0bc38589de4 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 15 Jan 2024 16:24:19 -0500 Subject: [PATCH] Fix: lib: handle BT_FUNC_STATUS_NOT_FOUND in bt_plugin_so_create_all_from_sections MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Reported-by: Brice Videau Reviewed-on: https://review.lttng.org/c/babeltrace/+/11679 Reviewed-by: Philippe Proulx Reviewed-by: Brice Videau Tested-by: jenkins --- configure.ac | 1 + src/lib/plugin/plugin-so.c | 12 +++ tests/lib/Makefile.am | 16 +++- .../test-plugin-init-fail-plugin/Makefile.am | 11 +++ .../plugin-init-fail.cpp | 21 +++++ tests/lib/test-plugin-init-fail.cpp | 80 +++++++++++++++++++ tests/lib/test-plugin-init-fail.sh | 22 +++++ 7 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 tests/lib/test-plugin-init-fail-plugin/Makefile.am create mode 100644 tests/lib/test-plugin-init-fail-plugin/plugin-init-fail.cpp create mode 100644 tests/lib/test-plugin-init-fail.cpp create mode 100755 tests/lib/test-plugin-init-fail.sh diff --git a/configure.ac b/configure.ac index 53b92cbc..f1ef5b14 100644 --- 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 a4622c38..698d03d0 100644 --- 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 9392f900..721244fb 100644 --- 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 index 00000000..fb20778e --- /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 index 00000000..44bc1175 --- /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 + +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 index 00000000..8d8fd411 --- /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 + +#include + +#include + +#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 index 00000000..661023a8 --- /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 -- 2.34.1