From c8dc9c2b8ddccf03dc5ccd8de8a0778d27ac35a2 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Mon, 7 Oct 2024 21:02:32 +0200 Subject: [PATCH] Increase safety checks around ObjectRefs --- src/script/common/c_content.cpp | 2 ++ src/script/cpp_api/s_base.cpp | 21 +++++++++++++-------- src/script/lua_api/l_object.cpp | 4 +++- src/script/lua_api/l_object.h | 6 ++++-- src/unittest/mock_serveractiveobject.h | 2 +- src/unittest/test_moveaction.cpp | 4 ++++ 6 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/script/common/c_content.cpp b/src/script/common/c_content.cpp index 2e5873bbf..9ad067742 100644 --- a/src/script/common/c_content.cpp +++ b/src/script/common/c_content.cpp @@ -2236,12 +2236,14 @@ void push_pointed_thing(lua_State *L, const PointedThing &pointed, bool csm, void push_objectRef(lua_State *L, const u16 id) { + assert(id != 0); // Get core.object_refs[i] lua_getglobal(L, "core"); lua_getfield(L, -1, "object_refs"); luaL_checktype(L, -1, LUA_TTABLE); lua_pushinteger(L, id); lua_gettable(L, -2); + assert(!lua_isnoneornil(L, -1)); lua_remove(L, -2); // object_refs lua_remove(L, -2); // core } diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp index bdd2514e6..cd74b7cfd 100644 --- a/src/script/cpp_api/s_base.cpp +++ b/src/script/cpp_api/s_base.cpp @@ -474,7 +474,7 @@ void ScriptApiBase::addObjectReference(ServerActiveObject *cobj) int objectstable = lua_gettop(L); // object_refs[id] = object - lua_pushnumber(L, cobj->getId()); // Push id + lua_pushinteger(L, cobj->getId()); // Push id lua_pushvalue(L, object); // Copy object to top of stack lua_settable(L, objectstable); } @@ -491,24 +491,29 @@ void ScriptApiBase::removeObjectReference(ServerActiveObject *cobj) int objectstable = lua_gettop(L); // Get object_refs[id] - lua_pushnumber(L, cobj->getId()); // Push id + lua_pushinteger(L, cobj->getId()); // Push id lua_gettable(L, objectstable); // Set object reference to NULL - ObjectRef::set_null(L); + ObjectRef::set_null(L, cobj); lua_pop(L, 1); // pop object // Set object_refs[id] = nil - lua_pushnumber(L, cobj->getId()); // Push id + lua_pushinteger(L, cobj->getId()); // Push id lua_pushnil(L); lua_settable(L, objectstable); } -// Creates a new anonymous reference if cobj=NULL or id=0 -void ScriptApiBase::objectrefGetOrCreate(lua_State *L, - ServerActiveObject *cobj) +void ScriptApiBase::objectrefGetOrCreate(lua_State *L, ServerActiveObject *cobj) { assert(getType() == ScriptingType::Server); - if (cobj == NULL || cobj->getId() == 0) { + if (!cobj) { + ObjectRef::create(L, nullptr); // dummy reference + } else if (cobj->getId() == 0) { + // TODO after 5.10.0: convert this to a FATAL_ERROR + errorstream << "ScriptApiBase::objectrefGetOrCreate(): " + << "Pushing orphan ObjectRef. Please open a bug report for this." + << std::endl; + assert(0); ObjectRef::create(L, cobj); } else { push_objectRef(L, cobj->getId()); diff --git a/src/script/lua_api/l_object.cpp b/src/script/lua_api/l_object.cpp index ae863502f..7a199e458 100644 --- a/src/script/lua_api/l_object.cpp +++ b/src/script/lua_api/l_object.cpp @@ -2776,9 +2776,11 @@ void ObjectRef::create(lua_State *L, ServerActiveObject *object) lua_setmetatable(L, -2); } -void ObjectRef::set_null(lua_State *L) +void ObjectRef::set_null(lua_State *L, void *expect) { ObjectRef *obj = checkObject(L, -1); + assert(obj); + FATAL_ERROR_IF(obj->m_object != expect, "ObjectRef table was messed with"); obj->m_object = nullptr; } diff --git a/src/script/lua_api/l_object.h b/src/script/lua_api/l_object.h index 8225aa470..bc131f4f2 100644 --- a/src/script/lua_api/l_object.h +++ b/src/script/lua_api/l_object.h @@ -38,10 +38,12 @@ public: ~ObjectRef() = default; // Creates an ObjectRef and leaves it on top of stack - // Not callable from Lua; all references are created on the C side. + // NOTE: do not call this, use `ScriptApiBase::objectrefGetOrCreate()`! static void create(lua_State *L, ServerActiveObject *object); - static void set_null(lua_State *L); + // Clear the pointer in the ObjectRef (at -1). + // Throws an fatal error if the object pointer wasn't `expect`. + static void set_null(lua_State *L, void *expect); static void Register(lua_State *L); diff --git a/src/unittest/mock_serveractiveobject.h b/src/unittest/mock_serveractiveobject.h index 78e3df8af..995a27e55 100644 --- a/src/unittest/mock_serveractiveobject.h +++ b/src/unittest/mock_serveractiveobject.h @@ -22,7 +22,7 @@ with this program; if not, write to the Free Software Foundation, Inc., class MockServerActiveObject : public ServerActiveObject { public: - MockServerActiveObject(ServerEnvironment *env = nullptr, const v3f &p = v3f()) : + MockServerActiveObject(ServerEnvironment *env = nullptr, v3f p = v3f()) : ServerActiveObject(env, p) {} virtual ActiveObjectType getType() const { return ACTIVEOBJECT_TYPE_TEST; } diff --git a/src/unittest/test_moveaction.cpp b/src/unittest/test_moveaction.cpp index 966cea8c7..b3e49341e 100644 --- a/src/unittest/test_moveaction.cpp +++ b/src/unittest/test_moveaction.cpp @@ -68,6 +68,8 @@ void TestMoveAction::runTests(IGameDef *gamedef) auto null_map = std::unique_ptr(); ServerEnvironment server_env(std::move(null_map), &server, &mb); MockServerActiveObject obj(&server_env); + obj.setId(1); + server.getScriptIface()->addObjectReference(&obj); TEST(testMove, &obj, gamedef); TEST(testMoveFillStack, &obj, gamedef); @@ -82,6 +84,8 @@ void TestMoveAction::runTests(IGameDef *gamedef) TEST(testCallbacks, &obj, &server); TEST(testCallbacksSwap, &obj, &server); + + server.getScriptIface()->removeObjectReference(&obj); } static ItemStack parse_itemstack(const char *s)