From f65c4ee4e1ae591f740e86fb2db6590c0c33725f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20B=C3=A1lint=20Misius?= Date: Tue, 6 May 2025 18:13:31 +0200 Subject: [PATCH] Make tools (and thus elements) read-only in some contexts Namely, in Select and Click tool callbacks, which are otherwise interface contexts and allow changing tools. This may cause use-after-free but it's much more likely to cause stack overflows. Importantly, the setter usage of ui.activeTool is considered to change tool state because it invokes Select callbacks. --- src/gui/game/GameControllerEvents.h | 1 + src/lua/LuaElements.cpp | 10 +++++----- src/lua/LuaInterface.cpp | 1 + src/lua/LuaScriptInterface.cpp | 10 ++++++++++ src/lua/LuaScriptInterface.h | 1 + src/lua/LuaTools.cpp | 10 +++++----- 6 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/gui/game/GameControllerEvents.h b/src/gui/game/GameControllerEvents.h index ee6a39bac..3d530f553 100644 --- a/src/gui/game/GameControllerEvents.h +++ b/src/gui/game/GameControllerEvents.h @@ -12,6 +12,7 @@ enum EventTraits : uint32_t eventTraitInterface = UINT32_C(0x00000008), eventTraitInterfaceGraphics = UINT32_C(0x00000010), eventTraitConstSim = UINT32_C(0x00000020), + eventTraitConstTools = UINT32_C(0x00000040), }; constexpr EventTraits operator |(EventTraits lhs, EventTraits rhs) { diff --git a/src/lua/LuaElements.cpp b/src/lua/LuaElements.cpp index ca57d4d6a..c0578ec03 100644 --- a/src/lua/LuaElements.cpp +++ b/src/lua/LuaElements.cpp @@ -324,7 +324,7 @@ static bool luaCtypeDrawWrapper(CTYPEDRAW_FUNC_ARGS) static int allocate(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); luaL_checktype(L, 1, LUA_TSTRING); luaL_checktype(L, 2, LUA_TSTRING); auto group = tpt_lua_toByteString(L, 1).ToUpper(); @@ -422,7 +422,7 @@ static int element(lua_State *L) if (lua_gettop(L) > 1) { - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); { auto &sd = SimulationData::Ref(); std::unique_lock lk(sd.elementGraphicsMx); @@ -573,7 +573,7 @@ static int property(lua_State *L) if (lua_gettop(L) > 2) { - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); auto &sd = SimulationData::Ref(); std::unique_lock lk(sd.elementGraphicsMx); auto &elements = sd.elements; @@ -727,7 +727,7 @@ static int property(lua_State *L) static int ffree(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); int id = luaL_checkinteger(L, 1); ByteString identifier; @@ -773,7 +773,7 @@ static int exists(lua_State *L) static int loadDefault(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); auto &sd = SimulationData::Ref(); std::unique_lock lk(sd.elementGraphicsMx); auto &elements = sd.elements; diff --git a/src/lua/LuaInterface.cpp b/src/lua/LuaInterface.cpp index 312dd9319..b8e7c0690 100644 --- a/src/lua/LuaInterface.cpp +++ b/src/lua/LuaInterface.cpp @@ -256,6 +256,7 @@ static int activeTool(lua_State *L) tpt_lua_pushByteString(L, lsi->gameModel->GetActiveTool(index)->Identifier); return 1; } + lsi->AssertMutableToolsEvent(); auto identifier = tpt_lua_checkByteString(L, 2); auto *tool = lsi->gameModel->GetToolFromIdentifier(identifier); if (!tool) diff --git a/src/lua/LuaScriptInterface.cpp b/src/lua/LuaScriptInterface.cpp index 582ea078a..d922c01bc 100644 --- a/src/lua/LuaScriptInterface.cpp +++ b/src/lua/LuaScriptInterface.cpp @@ -900,6 +900,7 @@ void LuaScriptInterface::AssertInterfaceEvent() luaL_error(L, "this functionality is restricted to interface events"); } } + void LuaScriptInterface::AssertMutableSimEvent() { if (eventTraits & eventTraitConstSim) @@ -907,3 +908,12 @@ void LuaScriptInterface::AssertMutableSimEvent() luaL_error(L, "this functionality is restricted to mutable simulation events"); } } + +void LuaScriptInterface::AssertMutableToolsEvent() +{ + AssertInterfaceEvent(); + if (eventTraits & eventTraitConstTools) + { + luaL_error(L, "this functionality is restricted to mutable tool events"); + } +} diff --git a/src/lua/LuaScriptInterface.h b/src/lua/LuaScriptInterface.h index adfa94681..ce11bcb1e 100644 --- a/src/lua/LuaScriptInterface.h +++ b/src/lua/LuaScriptInterface.h @@ -153,6 +153,7 @@ public: void AssertInterfaceEvent(); void AssertMutableSimEvent(); + void AssertMutableToolsEvent(); friend class CommandInterface; }; diff --git a/src/lua/LuaTools.cpp b/src/lua/LuaTools.cpp index 74bb890e2..09b328870 100644 --- a/src/lua/LuaTools.cpp +++ b/src/lua/LuaTools.cpp @@ -7,7 +7,7 @@ static int allocate(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); luaL_checktype(L, 1, LUA_TSTRING); luaL_checktype(L, 2, LUA_TSTRING); auto group = tpt_lua_toByteString(L, 1).ToUpper(); @@ -51,7 +51,7 @@ static bool IsCustom(int index) static int ffree(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); int index = luaL_checkinteger(L, 1); auto *tool = lsi->gameModel->GetToolByIndex(index); if (!tool) @@ -127,7 +127,7 @@ static void luaClickWrapper(SimTool *tool, Simulation *sim, const Brush &brush, lua_pushboolean(L, tool->shiftBehaviour); lua_pushboolean(L, tool->ctrlBehaviour); lua_pushboolean(L, tool->altBehaviour); - if (tpt_lua_pcall(L, 7, 0, 0, eventTraitInterface)) + if (tpt_lua_pcall(L, 7, 0, 0, eventTraitInterface | eventTraitConstTools)) { lsi->Log(CommandInterface::LogError, "In click func: " + LuaGetError()); lua_pop(L, 1); @@ -271,7 +271,7 @@ static void luaSelectWrapper(SimTool *tool, int toolSelection) { lua_rawgeti(L, LUA_REGISTRYINDEX, customTools[index].select); lua_pushinteger(L, toolSelection); - if (tpt_lua_pcall(L, 1, 0, 0, eventTraitInterface)) + if (tpt_lua_pcall(L, 1, 0, 0, eventTraitInterface | eventTraitConstTools)) { lsi->Log(CommandInterface::LogError, "In select func: " + LuaGetError()); lua_pop(L, 1); @@ -287,7 +287,7 @@ struct DependentFalse : std::false_type static int property(lua_State *L) { auto *lsi = GetLSI(); - lsi->AssertInterfaceEvent(); + lsi->AssertMutableToolsEvent(); int index = luaL_checkinteger(L, 1); auto *tool = lsi->gameModel->GetToolByIndex(index); if (!tool)