lua: fix a bug where the lifetime of userdata will result in crash (#35063)

code · wbpcode · web-flow · commit ab911ac2ff97 · 2024-07-07T10:33:15.000+08:00
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 &lt;wbphub@live.com&gt;
Co-authored-by: wbpcode &lt;wbphub@live.com&gt;
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();
   }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -80,12 +80,16 @@ TEST_F(LuaClusterSpecifierPluginTest, NormalLuaCode) {`
`80`	`80`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};`
`81`	`81`	`auto route = plugin_->route(mock_route, headers);`
`82`	`82`	`EXPECT_EQ("fake_service", route->routeEntry()->clusterName());`
	`83`	`+ // Force the runtime to gc and destroy all the userdata.`
	`84`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`83`	`85`	`}`
`84`	`86`
`85`	`87`	`{`
`86`	`88`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};`
`87`	`89`	`auto route = plugin_->route(mock_route, headers);`
`88`	`90`	`EXPECT_EQ("web_service", route->routeEntry()->clusterName());`
	`91`	`+ // Force the runtime to gc and destroy all the userdata.`
	`92`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`89`	`93`	`}`
`90`	`94`	`}`
`91`	`95`
`@@ -98,12 +102,16 @@ TEST_F(LuaClusterSpecifierPluginTest, ErrorLuaCode) {`
`98`	`102`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};`
`99`	`103`	`auto route = plugin_->route(mock_route, headers);`
`100`	`104`	`EXPECT_EQ("default_service", route->routeEntry()->clusterName());`
	`105`	`+ // Force the runtime to gc and destroy all the userdata.`
	`106`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`101`	`107`	`}`
`102`	`108`
`103`	`109`	`{`
`104`	`110`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};`
`105`	`111`	`auto route = plugin_->route(mock_route, headers);`
`106`	`112`	`EXPECT_EQ("default_service", route->routeEntry()->clusterName());`
	`113`	`+ // Force the runtime to gc and destroy all the userdata.`
	`114`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`107`	`115`	`}`
`108`	`116`	`}`
`109`	`117`
`@@ -116,12 +124,16 @@ TEST_F(LuaClusterSpecifierPluginTest, ReturnTypeNotStringLuaCode) {`
`116`	`124`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "fake"}};`
`117`	`125`	`auto route = plugin_->route(mock_route, headers);`
`118`	`126`	`EXPECT_EQ("fake_service", route->routeEntry()->clusterName());`
	`127`	`+ // Force the runtime to gc and destroy all the userdata.`
	`128`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`119`	`129`	`}`
`120`	`130`
`121`	`131`	`{`
`122`	`132`	`Http::TestRequestHeaderMapImpl headers{{":path", "/"}, {"header_key", "header_value"}};`
`123`	`133`	`auto route = plugin_->route(mock_route, headers);`
`124`	`134`	`EXPECT_EQ("default_service", route->routeEntry()->clusterName());`
	`135`	`+ // Force the runtime to gc and destroy all the userdata.`
	`136`	`+ config_->perLuaCodeSetup()->runtimeGC();`
`125`	`137`	`}`
`126`	`138`	`}`
`127`	`139`