From ab911ac2ff971f805ec822ad4d4ff6b42a61cc7c Mon Sep 17 00:00:00 2001
From: code <wangbaiping@corp.netease.com>
Date: Sun, 7 Jul 2024 10:33:15 +0800
Subject: [PATCH] lua: fix a bug where the lifetime of userdata will result in
 crash (#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 <wbphub@live.com>
Co-authored-by: wbpcode <wbphub@live.com>
---
 changelogs/current.yaml                              |  3 +++
 .../cluster_specifiers/lua/lua_cluster_specifier.h   |  7 +++++++
 .../lua/lua_cluster_specifier_test.cc                | 12 ++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/changelogs/current.yaml b/changelogs/current.yaml
index acdd85b1cdf7..da2cb13d9efc 100644
--- 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
index aad6aec5d7af..d8ca8f39b2fc 100644
--- 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
index 8cd35173320f..30c8b624246a 100644
--- 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();
   }
 }