From 68c1c89b4c326b612b68b8128326ff28defc8413 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Sat, 15 Feb 2025 10:17:25 -0800 Subject: [PATCH] dynamic_modules: prevent multiple calls to init function (#38458) Commit Message: dynamic_modules: prevent multiple calls to init function Additional Description: Previously, the program init function was called multiple times despite the semantics of the ABI. This fixes that by utilizing RTLD_NOLOAD flag of dlopen to check a shared library is already loaded. Risk Level: low Testing: done Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Takeshi Yoneda --- .../dynamic_modules/dynamic_modules.cc | 18 +++++++++++--- test/extensions/dynamic_modules/BUILD | 1 + .../dynamic_modules/dynamic_modules_test.cc | 24 +++++++++++++++---- .../dynamic_modules/test_data/c/BUILD | 2 ++ .../test_data/c/program_init_assert.c | 14 +++++++++++ tools/spelling/spelling_dictionary.txt | 1 + 6 files changed, 52 insertions(+), 8 deletions(-) create mode 100644 test/extensions/dynamic_modules/test_data/c/program_init_assert.c diff --git a/source/extensions/dynamic_modules/dynamic_modules.cc b/source/extensions/dynamic_modules/dynamic_modules.cc index 363a5d7a21aa..a0e5463d4945 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.cc +++ b/source/extensions/dynamic_modules/dynamic_modules.cc @@ -18,6 +18,20 @@ constexpr char DYNAMIC_MODULES_SEARCH_PATH[] = "ENVOY_DYNAMIC_MODULES_SEARCH_PAT absl::StatusOr newDynamicModule(const absl::string_view object_file_path, const bool do_not_close) { + const std::filesystem::path file_path_absolute = std::filesystem::absolute(object_file_path); + // From the man page of dlopen(3): + // + // > This can be used to test if the object is already resident (dlopen() returns NULL if it + // > is not, or the object's handle if it is resident). + // + // So we can use RTLD_NOLOAD to check if the module is already loaded to avoid the duplicate call + // to the init function. + void* handle = dlopen(file_path_absolute.c_str(), RTLD_NOLOAD | RTLD_LAZY); + if (handle != nullptr) { + // This means the module is already loaded, and the return value is the handle of the already + // loaded module. We don't need to call the init function again. + return std::make_unique(handle); + } // RTLD_LOCAL is always needed to avoid collisions between multiple modules. // RTLD_LAZY is required for not only performance but also simply to load the module, otherwise // dlopen results in Invalid argument. @@ -25,9 +39,7 @@ absl::StatusOr newDynamicModule(const absl::string_view object if (do_not_close) { mode |= RTLD_NODELETE; } - - const std::filesystem::path file_path_absolute = std::filesystem::absolute(object_file_path); - void* handle = dlopen(file_path_absolute.c_str(), mode); + handle = dlopen(file_path_absolute.c_str(), mode); if (handle == nullptr) { return absl::InvalidArgumentError( absl::StrCat("Failed to load dynamic module: ", object_file_path, " : ", dlerror())); diff --git a/test/extensions/dynamic_modules/BUILD b/test/extensions/dynamic_modules/BUILD index aad4b5db9496..9cb4ac3e2088 100644 --- a/test/extensions/dynamic_modules/BUILD +++ b/test/extensions/dynamic_modules/BUILD @@ -23,6 +23,7 @@ envoy_cc_test( "//test/extensions/dynamic_modules/test_data/c:abi_version_mismatch", "//test/extensions/dynamic_modules/test_data/c:no_op", "//test/extensions/dynamic_modules/test_data/c:no_program_init", + "//test/extensions/dynamic_modules/test_data/c:program_init_assert", "//test/extensions/dynamic_modules/test_data/c:program_init_fail", "//test/extensions/dynamic_modules/test_data/rust:abi_version_mismatch", "//test/extensions/dynamic_modules/test_data/rust:no_op", diff --git a/test/extensions/dynamic_modules/dynamic_modules_test.cc b/test/extensions/dynamic_modules/dynamic_modules_test.cc index 8bd01ae1234a..528c108028f0 100644 --- a/test/extensions/dynamic_modules/dynamic_modules_test.cc +++ b/test/extensions/dynamic_modules/dynamic_modules_test.cc @@ -57,11 +57,25 @@ TEST_P(DynamicModuleTestLanguages, DoNotClose) { EXPECT_EQ(getSomeVariable3.value()(), 4); // Start from 4. } -TEST_P(DynamicModuleTestLanguages, LoadNoOp) { - std::string language = GetParam(); - absl::StatusOr module = - newDynamicModule(testSharedObjectPath("no_op", language), false); - EXPECT_TRUE(module.ok()); +TEST(DynamicModuleTestLanguages, InitFunctionOnlyCalledOnce) { + const auto path = testSharedObjectPath("program_init_assert", "c"); + absl::StatusOr m1 = newDynamicModule(path, false); + EXPECT_TRUE(m1.ok()); + // At this point, m1 is alive, so the init function should have been called. + // When creating a new module with the same path, the init function should not be called again. + absl::StatusOr m2 = newDynamicModule(path, false); + EXPECT_TRUE(m2.ok()); + m1->reset(); + m2->reset(); + + // Even with the do_not_close=true, init function should only be called once. + m1 = newDynamicModule(path, true); + EXPECT_TRUE(m1.ok()); + m1->reset(); // Closing the module, but the module is still alive in the process. + // This m2 should point to the same module as m1 whose handle is already freed, but + // the init function should not be called again. + m2 = newDynamicModule(path, true); + EXPECT_TRUE(m2.ok()); } TEST_P(DynamicModuleTestLanguages, NoProgramInit) { diff --git a/test/extensions/dynamic_modules/test_data/c/BUILD b/test/extensions/dynamic_modules/test_data/c/BUILD index dd6008e7b521..1176dc382266 100644 --- a/test/extensions/dynamic_modules/test_data/c/BUILD +++ b/test/extensions/dynamic_modules/test_data/c/BUILD @@ -11,6 +11,8 @@ test_program(name = "no_op") test_program(name = "no_program_init") +test_program(name = "program_init_assert") + test_program(name = "program_init_fail") test_program(name = "abi_version_mismatch") diff --git a/test/extensions/dynamic_modules/test_data/c/program_init_assert.c b/test/extensions/dynamic_modules/test_data/c/program_init_assert.c new file mode 100644 index 000000000000..d70876aa6350 --- /dev/null +++ b/test/extensions/dynamic_modules/test_data/c/program_init_assert.c @@ -0,0 +1,14 @@ +#include + +#include "source/extensions/dynamic_modules/abi.h" +#include "source/extensions/dynamic_modules/abi_version.h" + +envoy_dynamic_module_type_abi_version_envoy_ptr envoy_dynamic_module_on_program_init(void) { + // This ensures that the init function is only called once during the test. + static bool initialized = false; + if (initialized) { + assert(0); + } + initialized = true; + return kAbiVersion; +} diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index e2c5b64d7040..cebdded1cd37 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -322,6 +322,7 @@ NOLINT NOLINTBEGIN NOLINTEND NOLINTNEXTLINE +NOLOAD NONAME NONBLOCK NONCES