From abe10ab0ccaecabf33dd8662bcbfc7c2dbc6699e Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 19:23:36 +0000 Subject: [PATCH 1/7] dynamic_modules: prevent multiple calls to init function Signed-off-by: Takeshi Yoneda --- .../dynamic_modules/dynamic_modules.cc | 26 +++++++++++++++---- .../dynamic_modules/dynamic_modules.h | 6 ++++- 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 ++++++++++ 6 files changed, 62 insertions(+), 11 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..431f83f82f2f 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, do_not_close); + } // 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,15 +39,13 @@ 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())); } - DynamicModulePtr dynamic_module = std::make_unique(handle); + DynamicModulePtr dynamic_module = std::make_unique(handle, do_not_close); const auto init_function = dynamic_module->getFunctionPointer( @@ -69,7 +81,11 @@ absl::StatusOr newDynamicModuleByName(const absl::string_view return newDynamicModule(file_path_absolute.string(), do_not_close); } -DynamicModule::~DynamicModule() { dlclose(handle_); } +DynamicModule::~DynamicModule() { + if (!do_not_close_) { + dlclose(handle_); + } +} void* DynamicModule::getSymbol(const absl::string_view symbol_ref) const { // TODO(mathetake): maybe we should accept null-terminated const char* instead of string_view to diff --git a/source/extensions/dynamic_modules/dynamic_modules.h b/source/extensions/dynamic_modules/dynamic_modules.h index e0c222549741..a6b6ce6a60f4 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.h +++ b/source/extensions/dynamic_modules/dynamic_modules.h @@ -19,7 +19,8 @@ namespace DynamicModules { */ class DynamicModule { public: - DynamicModule(void* handle) : handle_(handle) {} + DynamicModule(void* handle, const bool do_not_close) + : handle_(handle), do_not_close_(do_not_close){}; ~DynamicModule(); /** @@ -50,6 +51,9 @@ class DynamicModule { // The raw dlopen handle that can be used to look up symbols. void* handle_; + + // If true, the dlopen handle will not be closed with dlclose. + bool do_not_close_; }; using DynamicModulePtr = std::unique_ptr; 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..20d608cc765d 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->release(); + // This m2 should point to the same module as m1 (which is already loaded), 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; +} From 5f07337cd7a1b6ccc0b5d150762b1599c2b7e12d Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 19:26:57 +0000 Subject: [PATCH 2/7] comment Signed-off-by: Takeshi Yoneda --- test/extensions/dynamic_modules/dynamic_modules_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/dynamic_modules/dynamic_modules_test.cc b/test/extensions/dynamic_modules/dynamic_modules_test.cc index 20d608cc765d..529222d30115 100644 --- a/test/extensions/dynamic_modules/dynamic_modules_test.cc +++ b/test/extensions/dynamic_modules/dynamic_modules_test.cc @@ -71,8 +71,8 @@ TEST(DynamicModuleTestLanguages, InitFunctionOnlyCalledOnce) { // Even with the do_not_close=true, init function should only be called once. m1 = newDynamicModule(path, true); EXPECT_TRUE(m1.ok()); - m1->release(); - // This m2 should point to the same module as m1 (which is already loaded), but + m1->release(); // 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 released, but // the init function should not be called again. m2 = newDynamicModule(path, true); EXPECT_TRUE(m2.ok()); From 907964e7c2c00e449b467bd49fc2bbfc1f754cc4 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 19:42:41 +0000 Subject: [PATCH 3/7] dictionary Signed-off-by: Takeshi Yoneda --- tools/spelling/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index e2c5b64d7040..1d4077fd072c 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -103,6 +103,7 @@ Preconnecting RCVBUF RTCP RTLD +RTLD_NOLOAD RTP SOH SPC From 344c4a2095eef29f0529d174c145bf7fefb139f8 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 19:52:22 +0000 Subject: [PATCH 4/7] fix Signed-off-by: Takeshi Yoneda --- tools/spelling/spelling_dictionary.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 1d4077fd072c..cebdded1cd37 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -103,7 +103,6 @@ Preconnecting RCVBUF RTCP RTLD -RTLD_NOLOAD RTP SOH SPC @@ -323,6 +322,7 @@ NOLINT NOLINTBEGIN NOLINTEND NOLINTNEXTLINE +NOLOAD NONAME NONBLOCK NONCES From 899b5756f051c25a972cfacddee262d778487c7a Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 20:27:32 +0000 Subject: [PATCH 5/7] more Signed-off-by: Takeshi Yoneda --- source/extensions/dynamic_modules/dynamic_modules.cc | 10 +++------- source/extensions/dynamic_modules/dynamic_modules.h | 6 +----- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/source/extensions/dynamic_modules/dynamic_modules.cc b/source/extensions/dynamic_modules/dynamic_modules.cc index 431f83f82f2f..a0e5463d4945 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.cc +++ b/source/extensions/dynamic_modules/dynamic_modules.cc @@ -30,7 +30,7 @@ absl::StatusOr newDynamicModule(const absl::string_view object 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, do_not_close); + 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 @@ -45,7 +45,7 @@ absl::StatusOr newDynamicModule(const absl::string_view object absl::StrCat("Failed to load dynamic module: ", object_file_path, " : ", dlerror())); } - DynamicModulePtr dynamic_module = std::make_unique(handle, do_not_close); + DynamicModulePtr dynamic_module = std::make_unique(handle); const auto init_function = dynamic_module->getFunctionPointer( @@ -81,11 +81,7 @@ absl::StatusOr newDynamicModuleByName(const absl::string_view return newDynamicModule(file_path_absolute.string(), do_not_close); } -DynamicModule::~DynamicModule() { - if (!do_not_close_) { - dlclose(handle_); - } -} +DynamicModule::~DynamicModule() { dlclose(handle_); } void* DynamicModule::getSymbol(const absl::string_view symbol_ref) const { // TODO(mathetake): maybe we should accept null-terminated const char* instead of string_view to diff --git a/source/extensions/dynamic_modules/dynamic_modules.h b/source/extensions/dynamic_modules/dynamic_modules.h index a6b6ce6a60f4..5e4dc4492d67 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.h +++ b/source/extensions/dynamic_modules/dynamic_modules.h @@ -19,8 +19,7 @@ namespace DynamicModules { */ class DynamicModule { public: - DynamicModule(void* handle, const bool do_not_close) - : handle_(handle), do_not_close_(do_not_close){}; + DynamicModule(void* handle) : handle_(handle){}; ~DynamicModule(); /** @@ -51,9 +50,6 @@ class DynamicModule { // The raw dlopen handle that can be used to look up symbols. void* handle_; - - // If true, the dlopen handle will not be closed with dlclose. - bool do_not_close_; }; using DynamicModulePtr = std::unique_ptr; From d86733405ab9c3ca6bfc80e0ab7b8e82d0c15e7e Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 20:29:02 +0000 Subject: [PATCH 6/7] more Signed-off-by: Takeshi Yoneda --- source/extensions/dynamic_modules/dynamic_modules.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/dynamic_modules/dynamic_modules.h b/source/extensions/dynamic_modules/dynamic_modules.h index 5e4dc4492d67..e0c222549741 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.h +++ b/source/extensions/dynamic_modules/dynamic_modules.h @@ -19,7 +19,7 @@ namespace DynamicModules { */ class DynamicModule { public: - DynamicModule(void* handle) : handle_(handle){}; + DynamicModule(void* handle) : handle_(handle) {} ~DynamicModule(); /** From 2b8383b79bd4fb8030ad89b48281f1c15488c636 Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Fri, 14 Feb 2025 20:46:48 +0000 Subject: [PATCH 7/7] reset Signed-off-by: Takeshi Yoneda --- test/extensions/dynamic_modules/dynamic_modules_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/dynamic_modules/dynamic_modules_test.cc b/test/extensions/dynamic_modules/dynamic_modules_test.cc index 529222d30115..528c108028f0 100644 --- a/test/extensions/dynamic_modules/dynamic_modules_test.cc +++ b/test/extensions/dynamic_modules/dynamic_modules_test.cc @@ -71,8 +71,8 @@ TEST(DynamicModuleTestLanguages, InitFunctionOnlyCalledOnce) { // Even with the do_not_close=true, init function should only be called once. m1 = newDynamicModule(path, true); EXPECT_TRUE(m1.ok()); - m1->release(); // 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 released, but + 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());