From fe13f9dfd12c0a7f08355b83e34e7dec1bfdd86d Mon Sep 17 00:00:00 2001 From: Jude Melton-Houghton Date: Sun, 11 Sep 2022 13:28:37 -0400 Subject: [PATCH] Fix potential use-after-free with item metadata (#12729) This fixes a use-after-free bug in the case where itemstack metadata is accessed after the itemstack has been garbage-collected. --- src/script/lua_api/l_item.cpp | 13 ++----------- src/script/lua_api/l_item.h | 13 ++++++++----- src/script/lua_api/l_itemstackmeta.cpp | 16 +++++++++++++--- src/script/lua_api/l_itemstackmeta.h | 17 ++++++++++------- src/util/pointer.h | 14 ++++++++++++++ 5 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/script/lua_api/l_item.cpp b/src/script/lua_api/l_item.cpp index cf0fcce71..bf73e78c1 100644 --- a/src/script/lua_api/l_item.cpp +++ b/src/script/lua_api/l_item.cpp @@ -34,7 +34,7 @@ with this program; if not, write to the Free Software Foundation, Inc., int LuaItemStack::gc_object(lua_State *L) { LuaItemStack *o = *(LuaItemStack **)(lua_touserdata(L, 1)); - delete o; + o->drop(); return 0; } @@ -152,7 +152,7 @@ int LuaItemStack::l_get_meta(lua_State *L) { NO_MAP_LOCK_REQUIRED; LuaItemStack *o = checkobject(L, 1); - ItemStackMetaRef::create(L, &o->m_stack); + ItemStackMetaRef::create(L, o); return 1; } @@ -438,15 +438,6 @@ LuaItemStack::LuaItemStack(const ItemStack &item): { } -const ItemStack& LuaItemStack::getItem() const -{ - return m_stack; -} -ItemStack& LuaItemStack::getItem() -{ - return m_stack; -} - // LuaItemStack(itemstack or itemstring or table or nil) // Creates an LuaItemStack and leaves it on top of stack int LuaItemStack::create_object(lua_State *L) diff --git a/src/script/lua_api/l_item.h b/src/script/lua_api/l_item.h index a392555d2..72b1922dd 100644 --- a/src/script/lua_api/l_item.h +++ b/src/script/lua_api/l_item.h @@ -21,11 +21,15 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "lua_api/l_base.h" #include "inventory.h" // ItemStack +#include "util/pointer.h" -class LuaItemStack : public ModApiBase { +class LuaItemStack : public ModApiBase, public IntrusiveReferenceCounted { private: ItemStack m_stack; + LuaItemStack(const ItemStack &item); + ~LuaItemStack() = default; + static const char className[]; static const luaL_Reg methods[]; @@ -138,11 +142,10 @@ private: static int l_peek_item(lua_State *L); public: - LuaItemStack(const ItemStack &item); - ~LuaItemStack() = default; + DISABLE_CLASS_COPY(LuaItemStack) - const ItemStack& getItem() const; - ItemStack& getItem(); + inline const ItemStack& getItem() const { return m_stack; } + inline ItemStack& getItem() { return m_stack; } // LuaItemStack(itemstack or itemstring or table or nil) // Creates an LuaItemStack and leaves it on top of stack diff --git a/src/script/lua_api/l_itemstackmeta.cpp b/src/script/lua_api/l_itemstackmeta.cpp index 739fb9221..c17bb8995 100644 --- a/src/script/lua_api/l_itemstackmeta.cpp +++ b/src/script/lua_api/l_itemstackmeta.cpp @@ -38,12 +38,12 @@ ItemStackMetaRef* ItemStackMetaRef::checkobject(lua_State *L, int narg) Metadata* ItemStackMetaRef::getmeta(bool auto_create) { - return &istack->metadata; + return &istack->getItem().metadata; } void ItemStackMetaRef::clearMeta() { - istack->metadata.clear(); + istack->getItem().metadata.clear(); } void ItemStackMetaRef::reportMetadataChange(const std::string *name) @@ -67,6 +67,16 @@ int ItemStackMetaRef::l_set_tool_capabilities(lua_State *L) return 0; } +ItemStackMetaRef::ItemStackMetaRef(LuaItemStack *istack): istack(istack) +{ + istack->grab(); +} + +ItemStackMetaRef::~ItemStackMetaRef() +{ + istack->drop(); +} + // garbage collector int ItemStackMetaRef::gc_object(lua_State *L) { ItemStackMetaRef *o = *(ItemStackMetaRef **)(lua_touserdata(L, 1)); @@ -76,7 +86,7 @@ int ItemStackMetaRef::gc_object(lua_State *L) { // Creates an NodeMetaRef and leaves it on top of stack // Not callable from Lua; all references are created on the C side. -void ItemStackMetaRef::create(lua_State *L, ItemStack *istack) +void ItemStackMetaRef::create(lua_State *L, LuaItemStack *istack) { ItemStackMetaRef *o = new ItemStackMetaRef(istack); //infostream<<"NodeMetaRef::create: o="<metadata.setToolCapabilities(caps); + istack->getItem().metadata.setToolCapabilities(caps); } void clearToolCapabilities() { - istack->metadata.clearToolCapabilities(); + istack->getItem().metadata.clearToolCapabilities(); } // Exported functions @@ -58,12 +58,15 @@ private: // garbage collector static int gc_object(lua_State *L); public: - ItemStackMetaRef(ItemStack *istack): istack(istack) {} - ~ItemStackMetaRef() = default; + // takes a reference + ItemStackMetaRef(LuaItemStack *istack); + ~ItemStackMetaRef(); + + DISABLE_CLASS_COPY(ItemStackMetaRef) // Creates an ItemStackMetaRef and leaves it on top of stack // Not callable from Lua; all references are created on the C side. - static void create(lua_State *L, ItemStack *istack); + static void create(lua_State *L, LuaItemStack *istack); static void Register(lua_State *L); }; diff --git a/src/util/pointer.h b/src/util/pointer.h index b659cea0e..f4b70f822 100644 --- a/src/util/pointer.h +++ b/src/util/pointer.h @@ -257,3 +257,17 @@ private: unsigned int *refcount; }; +// This class is not thread-safe! +class IntrusiveReferenceCounted { +public: + IntrusiveReferenceCounted() = default; + virtual ~IntrusiveReferenceCounted() = default; + void grab() noexcept { ++m_refcount; } + void drop() noexcept { if (--m_refcount == 0) delete this; } + + // Preserve own reference count. + IntrusiveReferenceCounted(const IntrusiveReferenceCounted &) {} + IntrusiveReferenceCounted &operator=(const IntrusiveReferenceCounted &) { return *this; } +private: + u32 m_refcount = 1; +};