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 <t.y.mathetake@gmail.com>

source/extensions/dynamic_modules/dynamic_modules.cc

@@ -18,16 +18,28 @@ constexpr char DYNAMIC_MODULES_SEARCH_PATH[] = "ENVOY_DYNAMIC_MODULES_SEARCH_PAT
 
 absl::StatusOr<DynamicModulePtr> 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<DynamicModule>(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.
   int mode = RTLD_LOCAL | RTLD_LAZY;
   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()));

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",

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<DynamicModulePtr> module =
-      newDynamicModule(testSharedObjectPath("no_op", language), false);
-  EXPECT_TRUE(module.ok());
+TEST(DynamicModuleTestLanguages, InitFunctionOnlyCalledOnce) {
+  const auto path = testSharedObjectPath("program_init_assert", "c");
+  absl::StatusOr<DynamicModulePtr> 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<DynamicModulePtr> 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) {

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")

test/extensions/dynamic_modules/test_data/c/program_init_assert.c

@@ -0,0 +1,14 @@
+#include <assert.h>
+
+#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;
+}

tools/spelling/spelling_dictionary.txt

@@ -322,6 +322,7 @@ NOLINT
 NOLINTBEGIN
 NOLINTEND
 NOLINTNEXTLINE
+NOLOAD
 NONAME
 NONBLOCK
 NONCES