From aa4303fc385fd35fa63997290153835c9c7d2cf8 Mon Sep 17 00:00:00 2001 From: uramer Date: Mon, 12 Feb 2024 22:25:12 +0100 Subject: [PATCH 1/4] Fix crash when throwing in index meta methods --- CMakeLists.txt | 1 + components/lua/luastate.hpp | 24 ++++++++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7b1b18e41e..3a69f28c75 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -477,6 +477,7 @@ if (NOT WIN32) endif() # C++ library binding to Lua +add_compile_definitions(SOL_ALL_SAFETIES_ON) set(SOL_INCLUDE_DIR ${OpenMW_SOURCE_DIR}/extern/sol3) set(SOL_CONFIG_DIR ${OpenMW_SOURCE_DIR}/extern/sol_config) diff --git a/components/lua/luastate.hpp b/components/lua/luastate.hpp index aea1e32590..f14fc44867 100644 --- a/components/lua/luastate.hpp +++ b/components/lua/luastate.hpp @@ -232,16 +232,36 @@ namespace LuaUtil } } + // work around for a (likely) sol3 bug + // when the index meta method throws, simply calling table.get crashes instead of re-throwing the error + template + sol::object safe_get(const sol::table& table, const Key& key) + { + auto index = table.traverse_raw_get>( + sol::metatable_key, sol::meta_function::index); + if (index) + { + sol::protected_function_result result = index.value()(table, key); + if (result.valid()) + return result.get(); + else + throw result.get(); + } + else + return table.raw_get(key); + } + // getFieldOrNil(table, "a", "b", "c") returns table["a"]["b"]["c"] or nil if some of the fields doesn't exist. template sol::object getFieldOrNil(const sol::object& table, std::string_view first, const Str&... str) { if (!table.is()) return sol::nil; + sol::object value = safe_get(table.as(), first); if constexpr (sizeof...(str) == 0) - return table.as()[first]; + return value; else - return getFieldOrNil(table.as()[first], str...); + return getFieldOrNil(value, str...); } // String representation of a Lua object. Should be used for debugging/logging purposes only. From 550659c2d936cb658c676f461effa55f32934e89 Mon Sep 17 00:00:00 2001 From: uramer Date: Mon, 12 Feb 2024 22:40:07 +0100 Subject: [PATCH 2/4] Fix loadVFS error handling --- components/lua/luastate.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/lua/luastate.cpp b/components/lua/luastate.cpp index 13e2208b68..3cf6378f2f 100644 --- a/components/lua/luastate.cpp +++ b/components/lua/luastate.cpp @@ -376,7 +376,7 @@ namespace LuaUtil sol::protected_function_result LuaState::throwIfError(sol::protected_function_result&& res) { - if (!res.valid() && static_cast(res.get_type()) == LUA_TSTRING) + if (!res.valid()) throw std::runtime_error(std::string("Lua error: ") += res.get().what()); else return std::move(res); @@ -397,7 +397,7 @@ namespace LuaUtil std::string fileContent(std::istreambuf_iterator(*mVFS->get(path)), {}); sol::load_result res = mSol.load(fileContent, path, sol::load_mode::text); if (!res.valid()) - throw std::runtime_error("Lua error: " + res.get()); + throw std::runtime_error(std::string("Lua error: ") += res.get().what()); return res; } From 08b7ee8a44f40fc50621495ad29ed51f46928954 Mon Sep 17 00:00:00 2001 From: uramer Date: Mon, 12 Feb 2024 22:54:35 +0100 Subject: [PATCH 3/4] Test LuaUtil::safeGet preventing crash --- apps/openmw_test_suite/lua/test_lua.cpp | 12 +++++++++++- components/lua/luastate.hpp | 4 ++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/openmw_test_suite/lua/test_lua.cpp b/apps/openmw_test_suite/lua/test_lua.cpp index 90c987522d..a669a3c670 100644 --- a/apps/openmw_test_suite/lua/test_lua.cpp +++ b/apps/openmw_test_suite/lua/test_lua.cpp @@ -51,6 +51,9 @@ return { } )X"); + TestingOpenMW::VFSTestFile metaIndexErrorFile( + "return setmetatable({}, { __index = function(t, key) error('meta index error') end })"); + std::string genBigScript() { std::stringstream buf; @@ -70,7 +73,7 @@ return { { std::unique_ptr mVFS = TestingOpenMW::createTestVFS({ { "aaa/counter.lua", &counterFile }, { "bbb/tests.lua", &testsFile }, { "invalid.lua", &invalidScriptFile }, { "big.lua", &bigScriptFile }, - { "requireBig.lua", &requireBigScriptFile } }); + { "requireBig.lua", &requireBigScriptFile }, { "metaIndexError.lua", &metaIndexErrorFile } }); LuaUtil::ScriptsConfiguration mCfg; LuaUtil::LuaState mLua{ mVFS.get(), &mCfg }; @@ -223,4 +226,11 @@ return { // At this moment all instances of the script should be garbage-collected. EXPECT_LT(memWithoutScript, memWithScript); } + + TEST_F(LuaStateTest, SafeIndexMetamethod) + { + sol::table t = mLua.runInNewSandbox("metaIndexError.lua"); + // without safe get we crash here + EXPECT_ERROR(LuaUtil::safeGet(t, "any key"), "meta index error"); + } } diff --git a/components/lua/luastate.hpp b/components/lua/luastate.hpp index f14fc44867..509b5e16e1 100644 --- a/components/lua/luastate.hpp +++ b/components/lua/luastate.hpp @@ -235,7 +235,7 @@ namespace LuaUtil // work around for a (likely) sol3 bug // when the index meta method throws, simply calling table.get crashes instead of re-throwing the error template - sol::object safe_get(const sol::table& table, const Key& key) + sol::object safeGet(const sol::table& table, const Key& key) { auto index = table.traverse_raw_get>( sol::metatable_key, sol::meta_function::index); @@ -257,7 +257,7 @@ namespace LuaUtil { if (!table.is()) return sol::nil; - sol::object value = safe_get(table.as(), first); + sol::object value = safeGet(table.as(), first); if constexpr (sizeof...(str) == 0) return value; else From d6f112aef295734913de09eca12f0a66147d4c8d Mon Sep 17 00:00:00 2001 From: Anton Uramer Date: Tue, 13 Feb 2024 12:35:28 +0100 Subject: [PATCH 4/4] Revert Lua sol safeties flag --- CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3a69f28c75..7b1b18e41e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -477,7 +477,6 @@ if (NOT WIN32) endif() # C++ library binding to Lua -add_compile_definitions(SOL_ALL_SAFETIES_ON) set(SOL_INCLUDE_DIR ${OpenMW_SOURCE_DIR}/extern/sol3) set(SOL_CONFIG_DIR ${OpenMW_SOURCE_DIR}/extern/sol_config)