Fix segfault in deprecation logging due to tail call, log by default (#10174)

builtin/settingtypes.txt

@@ -1220,10 +1220,10 @@ movement_gravity (Gravity) float 9.81
 [**Advanced]
 
 #    Handling for deprecated Lua API calls:
-#    -    legacy: (try to) mimic old behaviour (default for release).
-#    -    log: mimic and log backtrace of deprecated call (default for debug).
+#    -    none: Do not log deprecated calls
+#    -    log: mimic and log backtrace of deprecated call (default).
 #    -    error: abort on usage of deprecated call (suggested for mod developers).
-deprecated_lua_api_handling (Deprecated Lua API handling) enum legacy legacy,log,error
+deprecated_lua_api_handling (Deprecated Lua API handling) enum log none,log,error
 
 #    Number of extra blocks that can be loaded by /clearobjects at once.
 #    This is a trade-off between sqlite transaction overhead and

src/defaultsettings.cpp

@@ -358,11 +358,7 @@ void set_default_settings(Settings *settings)
 	settings->setDefault("disallow_empty_password", "false");
 	settings->setDefault("disable_anticheat", "false");
 	settings->setDefault("enable_rollback_recording", "false");
-#ifdef NDEBUG
-	settings->setDefault("deprecated_lua_api_handling", "legacy");
-#else
 	settings->setDefault("deprecated_lua_api_handling", "log");
-#endif
 
 	settings->setDefault("kick_msg_shutdown", "Server shutting down.");
 	settings->setDefault("kick_msg_crash", "This server has experienced an internal error. You will now be disconnected.");

src/script/common/c_internal.cpp

@@ -155,24 +155,28 @@ static void script_log(lua_State *L, const std::string &message,
 		infostream << script_get_backtrace(L) << std::endl;
 }
 
-void log_deprecated(lua_State *L, const std::string &message, int stack_depth)
+DeprecatedHandlingMode get_deprecated_handling_mode()
 {
 	static thread_local bool configured = false;
-	static thread_local bool do_log = false;
-	static thread_local bool do_error = false;
+	static thread_local DeprecatedHandlingMode ret = DeprecatedHandlingMode::Ignore;
 
 	// Only read settings on first call
 	if (!configured) {
 		std::string value = g_settings->get("deprecated_lua_api_handling");
 		if (value == "log") {
-			do_log = true;
+			ret = DeprecatedHandlingMode::Log;
 		} else if (value == "error") {
-			do_log = true;
-			do_error = true;
+			ret = DeprecatedHandlingMode::Error;
 		}
 		configured = true;
 	}
 
-	if (do_log)
-		script_log(L, message, warningstream, do_error, stack_depth);
+	return ret;
+}
+
+void log_deprecated(lua_State *L, const std::string &message, int stack_depth)
+{
+	DeprecatedHandlingMode mode = get_deprecated_handling_mode();
+	if (mode != DeprecatedHandlingMode::Ignore)
+		script_log(L, message, warningstream, mode == DeprecatedHandlingMode::Error, stack_depth);
 }

src/script/common/c_internal.h

@@ -114,5 +114,25 @@ void script_error(lua_State *L, int pcall_result, const char *mod, const char *f
 void script_run_callbacks_f(lua_State *L, int nargs,
 	RunCallbacksMode mode, const char *fxn);
 
+enum class DeprecatedHandlingMode {
+	Ignore,
+	Log,
+	Error
+};
+
+/**
+ * Reads `deprecated_lua_api_handling` in settings, returns cached value.
+ *
+ * @return DeprecatedHandlingMode
+ */
+DeprecatedHandlingMode get_deprecated_handling_mode();
+
+/**
+ * Handles a deprecation warning based on user settings
+ *
+ * @param L Lua State
+ * @param message The deprecation method
+ * @param stack_depth How far on the stack to the first user function (ie: not builtin or core)
+ */
 void log_deprecated(lua_State *L, const std::string &message,
 	int stack_depth=1);

src/script/lua_api/l_base.cpp

@@ -100,32 +100,21 @@ bool ModApiBase::registerFunction(lua_State *L, const char *name,
 	return true;
 }
 
-std::unordered_map<std::string, luaL_Reg> ModApiBase::m_deprecated_wrappers;
-bool ModApiBase::m_error_deprecated_calls = false;
-
-int ModApiBase::l_deprecated_function(lua_State *L)
+int ModApiBase::l_deprecated_function(lua_State *L, const char *good, const char *bad, lua_CFunction func)
 {
 	thread_local std::vector<u64> deprecated_logged;
 
+	DeprecatedHandlingMode dep_mode = get_deprecated_handling_mode();
+	if (dep_mode == DeprecatedHandlingMode::Ignore)
+		return func(L);
+
 	u64 start_time = porting::getTimeUs();
 	lua_Debug ar;
 
-	// Get function name for lookup
-	FATAL_ERROR_IF(!lua_getstack(L, 0, &ar), "lua_getstack() failed");
-	FATAL_ERROR_IF(!lua_getinfo(L, "n", &ar), "lua_getinfo() failed");
-
-	// Combine name with line and script backtrace
+	// Get caller name with line and script backtrace
 	FATAL_ERROR_IF(!lua_getstack(L, 1, &ar), "lua_getstack() failed");
 	FATAL_ERROR_IF(!lua_getinfo(L, "Sl", &ar), "lua_getinfo() failed");
 
-	// Get parent class to get the wrappers map
-	luaL_checktype(L, 1, LUA_TUSERDATA);
-	void *ud = lua_touserdata(L, 1);
-	ModApiBase *o = *(ModApiBase**)ud;
-
-	// New function and new function name
-	auto it = o->m_deprecated_wrappers.find(ar.name);
-
 	// Get backtrace and hash it to reduce the warning flood
 	std::string backtrace = ar.short_src;
 	backtrace.append(":").append(std::to_string(ar.currentline));
@@ -135,46 +124,16 @@ int ModApiBase::l_deprecated_function(lua_State *L)
 			== deprecated_logged.end()) {
 
 		deprecated_logged.emplace_back(hash);
-		warningstream << "Call to deprecated function '"  << ar.name << "', please use '"
-			<< it->second.name << "' at " << backtrace << std::endl;
+		warningstream << "Call to deprecated function '"  << bad << "', please use '"
+			<< good << "' at " << backtrace << std::endl;
 
-		if (m_error_deprecated_calls)
+		if (dep_mode == DeprecatedHandlingMode::Error)
 			script_error(L, LUA_ERRRUN, NULL, NULL);
 	}
 
 	u64 end_time = porting::getTimeUs();
 	g_profiler->avg("l_deprecated_function", end_time - start_time);
 
-	return it->second.func(L);
+	return func(L);
 }
 
-void ModApiBase::markAliasDeprecated(luaL_Reg *reg)
-{
-	std::string value = g_settings->get("deprecated_lua_api_handling");
-	m_error_deprecated_calls = value == "error";
-
-	if (!m_error_deprecated_calls && value != "log")
-		return;
-
-	const char *last_name = nullptr;
-	lua_CFunction last_func = nullptr;
-
-	// ! Null termination !
-	while (reg->func) {
-		if (last_func == reg->func) {
-			// Duplicate found
-			luaL_Reg original_reg;
-			// Do not inline struct. Breaks MSVC or is error-prone
-			original_reg.name = last_name;
-			original_reg.func = reg->func;
-			m_deprecated_wrappers.emplace(
-				std::pair<std::string, luaL_Reg>(reg->name, original_reg));
-			reg->func = l_deprecated_function;
-		} else {
-			last_func = reg->func;
-			last_name = reg->name;
-		}
-
-		++reg;
-	}
-}

src/script/lua_api/l_base.h

@@ -41,7 +41,6 @@ class Environment;
 class ServerInventoryManager;
 
 class ModApiBase : protected LuaHelper {
-
 public:
 	static ScriptApiBase*   getScriptApiBase(lua_State *L);
 	static Server*          getServer(lua_State *L);
@@ -75,10 +74,18 @@ public:
 			lua_CFunction func,
 			int top);
 
-	static int l_deprecated_function(lua_State *L);
-	static void markAliasDeprecated(luaL_Reg *reg);
-private:
-	// <old_name> = { <new_name>, <new_function> }
-	static std::unordered_map<std::string, luaL_Reg> m_deprecated_wrappers;
-	static bool m_error_deprecated_calls;
+	/**
+	 * A wrapper for deprecated functions.
+	 *
+	 * When called, handles the deprecation according to user settings and then calls `func`.
+	 *
+	 * @throws Lua Error if required by the user settings.
+	 *
+	 * @param L Lua state
+	 * @param good Name of good function/method
+	 * @param bad Name of deprecated function/method
+	 * @param func Actual implementation of function
+	 * @return value from `func`
+	 */
+	static int l_deprecated_function(lua_State *L, const char *good, const char *bad, lua_CFunction func);
 };

src/script/lua_api/l_internal.h

@@ -29,7 +29,16 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "common/c_internal.h"
 
 #define luamethod(class, name) {#name, class::l_##name}
-#define luamethod_aliased(class, name, alias) {#name, class::l_##name}, {#alias, class::l_##name}
+
+#define luamethod_dep(class, good, bad)                                     \
+		{#bad, [](lua_State *L) -> int {                                    \
+			return l_deprecated_function(L, #bad, #good, &class::l_##good); \
+		}}
+
+#define luamethod_aliased(class, name, alias) \
+		luamethod(class, name),               \
+		luamethod_dep(class, name, alias)
+
 #define API_FCT(name) registerFunction(L, #name, l_##name, top)
 
 // For future use

src/script/lua_api/l_noise.cpp

@@ -122,7 +122,6 @@ void LuaPerlinNoise::Register(lua_State *L)
 
 	lua_pop(L, 1);
 
-	markAliasDeprecated(methods);
 	luaL_openlib(L, 0, methods, 0);
 	lua_pop(L, 1);
 
@@ -381,7 +380,6 @@ void LuaPerlinNoiseMap::Register(lua_State *L)
 
 	lua_pop(L, 1);
 
-	markAliasDeprecated(methods);
 	luaL_openlib(L, 0, methods, 0);
 	lua_pop(L, 1);
 

src/script/lua_api/l_object.cpp

@@ -2278,7 +2278,6 @@ void ObjectRef::Register(lua_State *L)
 
 	lua_pop(L, 1);  // drop metatable
 
-	markAliasDeprecated(methods);
 	luaL_openlib(L, 0, methods, 0);  // fill methodtable
 	lua_pop(L, 1);  // drop methodtable
 }
@@ -2316,10 +2315,9 @@ luaL_Reg ObjectRef::methods[] = {
 	luamethod(ObjectRef, get_nametag_attributes),
 
 	luamethod_aliased(ObjectRef, set_velocity, setvelocity),
-	luamethod(ObjectRef, add_velocity),
-	{"add_player_velocity", ObjectRef::l_add_velocity},
+	luamethod_aliased(ObjectRef, add_velocity, add_player_velocity),
 	luamethod_aliased(ObjectRef, get_velocity, getvelocity),
-	{"get_player_velocity", ObjectRef::l_get_velocity},
+	luamethod_dep(ObjectRef, get_velocity, get_player_velocity),
 
 	// LuaEntitySAO-only
 	luamethod_aliased(ObjectRef, set_acceleration, setacceleration),