Increase safety checks around ObjectRefs

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
 }

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());

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<ObjectRef>(L, -1);
+	assert(obj);
+	FATAL_ERROR_IF(obj->m_object != expect, "ObjectRef table was messed with");
 	obj->m_object = nullptr;
 }
 

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);
 

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; }

src/unittest/test_moveaction.cpp

@@ -68,6 +68,8 @@ void TestMoveAction::runTests(IGameDef *gamedef)
 	auto null_map = std::unique_ptr<ServerMap>();
 	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)