lua: fix a bug where the lifetime of userdata will result in crash (e…

…nvoyproxy#35063) Commit Message: lua: fix a bug where the lifetime of userdata will result in crash Additional Description: The RouteHandleWrapper will be desotryed when the lua doing the gc. And the embedded HeaderMapRef will call the reset() when it's destructed and will refer the invalid lua_State pointer and finally result in a crash. Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: added. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <[email protected]> Co-authored-by: wbpcode <[email protected]>
daixiang0 · Jul 7, 2024 · ab911ac · ab911ac
1 parent 4b0d8d9
commit ab911ac
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 0 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -237,6 +237,9 @@ bug_fixes:
 - area: datadog
   change: |
     Bumped the version of datadog to resolve a crashing bug in earlier versions of the library.
+- area: lua
+  change: |
+    Fixed a bug where the user data will reference a dangling pointer to the Lua state and cause a crash.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/router/cluster_specifiers/lua/lua_cluster_specifier.h b/source/extensions/router/cluster_specifiers/lua/lua_cluster_specifier.h
@@ -25,6 +25,8 @@ class PerLuaCodeSetup : Logger::Loggable<Logger::Id::lua> {
 
   int clusterFunctionRef() { return lua_state_.getGlobalRef(cluster_function_slot_); }
 
+  void runtimeGC() { lua_state_.runtimeGC(); }
+
 private:
   uint64_t cluster_function_slot_{};
 
@@ -58,6 +60,11 @@ class RouteHandleWrapper : public Filters::Common::Lua::BaseLuaObject<RouteHandl
 
   static ExportedFunctions exportedFunctions() { return {{"headers", static_luaHeaders}}; }
 
+  // All embedded references should be reset when the object is marked dead. This is to ensure that
+  // we won't do the resetting in the destructor, which may be called after the referenced
+  // coroutine's lua_State is closed. And if that happens, the resetting will cause a crash.
+  void onMarkDead() override { headers_wrapper_.reset(); }
+
 private:
   /**
    * @return a handle to the headers.

diff --git a/test/extensions/router/cluster_specifiers/lua/lua_cluster_specifier_test.cc b/test/extensions/router/cluster_specifiers/lua/lua_cluster_specifier_test.cc
@@ -80,12 +80,16 @@ TEST_F(LuaClusterSpecifierPluginTest, NormalLuaCode) {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("fake_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 
   {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("web_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 }
 
@@ -98,12 +102,16 @@ TEST_F(LuaClusterSpecifierPluginTest, ErrorLuaCode) {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("default_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 
   {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("default_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 }
 
@@ -116,12 +124,16 @@ TEST_F(LuaClusterSpecifierPluginTest, ReturnTypeNotStringLuaCode) {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("fake_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 
   {
     Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};
     auto route = plugin_->route(mock_route, headers);
     EXPECT_EQ("default_service", route->routeEntry()->clusterName());
+    // Force the runtime to gc and destroy all the userdata.
+    config_->perLuaCodeSetup()->runtimeGC();
   }
 }