Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed how data pushed to lua is validated to use weak_ptr #469

Merged
merged 8 commits into from
Jul 13, 2024
Merged

Changed how data pushed to lua is validated to use weak_ptr #469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
480875c
Changed how data pushed to lua is validated to use weak_ptr based imp…
Shauren Mar 14, 2024
bd6de45
Require both aura and aura owner references to be valid for auras
Shauren Mar 14, 2024
ff9b7ca
Merge branch 'master' into use_weaks
Shauren Mar 17, 2024
39eed65
Prevent cross-map access for lua values stored by earlier hook call
Shauren Apr 1, 2024
0fae48d
Merge branch 'master' of https://github.com/ElunaLuaEngine/Eluna into…
Foereaper Jul 12, 2024
e3696b8
Wrap weak pointer implementation in ifdefs for now
Foereaper Jul 12, 2024
7ba7506
Remove type casting functions only available in TrinityCore
Shauren Jul 13, 2024
29d5388
Minimize ElunaObject differences between cores using unique_weak_ptr …
Shauren Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 90 additions & 65 deletions ElunaTemplate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ extern "C"
#include "Globals/SharedDefines.h"
#endif

#include "UniqueTrackablePtr.h"

#define TRACKABLE_PTR_NAMESPACE ::Trinity::

class ElunaGlobal
{
public:
Expand Down Expand Up @@ -99,61 +103,106 @@ class ElunaGlobal
class ElunaObject
{
public:
template<typename T>
ElunaObject(Eluna* E, T* obj, bool manageMemory);
ElunaObject(Eluna* E, char const* tname) : E(E), type_name(tname)
{
}

~ElunaObject()
virtual ~ElunaObject()
{
}

// Get wrapped object pointer
void* GetObj() const { return object; }
virtual void* GetObj() const = 0;
// Returns whether the object is valid or not
bool IsValid() const { return !callstackid || callstackid == E->GetCallstackId(); }
// Returns whether the object can be invalidated or not
bool CanInvalidate() const { return _invalidate; }
virtual bool IsValid() const = 0;
// Returns pointer to the wrapped object's type name
const char* GetTypeName() const { return type_name; }

// Sets the object pointer that is wrapped
void SetObj(void* obj)
{
ASSERT(obj);
object = obj;
SetValid(true);
}
// Sets the object pointer to valid or invalid
void SetValid(bool valid)
private:
Eluna* E;
const char* type_name;
};

TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Aura> GetWeakPtrFor(Aura const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Battleground> GetWeakPtrFor(Battleground const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Group> GetWeakPtrFor(Group const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Guild> GetWeakPtrFor(Guild const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Map> GetWeakPtrFor(Map const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Object> GetWeakPtrForObjectImpl(Object const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Quest> GetWeakPtrFor(Quest const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Spell> GetWeakPtrFor(Spell const* obj);
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Vehicle> GetWeakPtrFor(Vehicle const* obj);

template <typename T>
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<T> GetWeakPtrFor(T const* obj)
{
return TRACKABLE_PTR_NAMESPACE static_pointer_cast<T>(GetWeakPtrForObjectImpl(obj));
}

template <typename T>
class ElunaObjectImpl : public ElunaObject
{
public:
ElunaObjectImpl(Eluna* E, T* obj, char const* tname) : ElunaObject(E, tname), _obj(GetWeakPtrFor(obj))
{
ASSERT(!valid || (valid && object));
if (valid)
if (CanInvalidate())
callstackid = E->GetCallstackId();
else
callstackid = 0;
else
callstackid = 1;
}
// Sets whether the pointer will be invalidated at end of calls
void SetValidation(bool invalidate)

void* GetObj() const override { return _obj.lock().get(); }
bool IsValid() const override { return !_obj.expired(); }

private:
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<T> _obj;
};

template <>
class ElunaObjectImpl<Aura> : public ElunaObject
{
public:
ElunaObjectImpl(Eluna* E, Aura* obj, char const* tname);

void* GetObj() const override { return _obj.lock().get(); }
bool IsValid() const override
{
_invalidate = invalidate;
// aura references are not invalidated when their owner (player) changes map
// owner reference must be checked additionally to ensure scripts don't store
// and access auras of players on another map (possibly updating in another thread)
return !_obj.expired() && !_owner.expired();
}
// Invalidates the pointer if it should be invalidated
void Invalidate()

private:
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<Aura> _obj;
TRACKABLE_PTR_NAMESPACE unique_weak_ptr<WorldObject> _owner;
};

template <typename T>
class ElunaObjectValueImpl : public ElunaObject
{
public:
ElunaObjectValueImpl(Eluna* E, T* obj, char const* tname) : ElunaObject(E, tname), _obj(std::move(*obj))
{
if (CanInvalidate())
callstackid = 1;
}

void* GetObj() const override { return const_cast<T*>(&_obj); }
bool IsValid() const override { return true; }

private:
Eluna* E;
uint64 callstackid;
bool _invalidate;
void* object;
const char* type_name;
T _obj;
};

#define MAKE_ELUNA_OBJECT_VALUE_IMPL(type) \
template <> \
class ElunaObjectImpl<type> : public ElunaObjectValueImpl<type> \
{ \
public: \
using ElunaObjectValueImpl::ElunaObjectValueImpl; \
}

MAKE_ELUNA_OBJECT_VALUE_IMPL(long long);
MAKE_ELUNA_OBJECT_VALUE_IMPL(unsigned long long);
MAKE_ELUNA_OBJECT_VALUE_IMPL(ObjectGuid);
MAKE_ELUNA_OBJECT_VALUE_IMPL(WorldPacket);
MAKE_ELUNA_OBJECT_VALUE_IMPL(ElunaQuery);

template<typename T>
struct ElunaRegister
{
Expand All @@ -167,13 +216,12 @@ class ElunaTemplate
{
public:
static const char* tname;
static bool manageMemory;

// name will be used as type name
// If gc is true, lua will handle the memory management for object pushed
// gc should be used if pushing for example WorldPacket,
// that will only be needed on lua side and will not be managed by TC/mangos/<core>
static void Register(Eluna* E, const char* name, bool gc = false)
static void Register(Eluna* E, const char* name)
{
ASSERT(E);
ASSERT(name);
Expand All @@ -186,7 +234,6 @@ class ElunaTemplate
lua_pop(E->L, 1);

tname = name;
manageMemory = gc;

// create metatable for userdata of this type
luaL_newmetatable(E->L, tname);
Expand Down Expand Up @@ -265,10 +312,6 @@ class ElunaTemplate
lua_pushcfunction(E->L, GetType);
lua_setfield(E->L, metatable, "GetObjectType");

// special method to decide object invalidation at end of call
lua_pushcfunction(E->L, SetInvalidation);
lua_setfield(E->L, metatable, "SetInvalidation");

// pop metatable
lua_pop(E->L, 1);
}
Expand Down Expand Up @@ -333,15 +376,17 @@ class ElunaTemplate
return 1;
}

typedef ElunaObjectImpl<T> ElunaObjectType;

// Create new userdata
ElunaObject* elunaObject = static_cast<ElunaObject*>(lua_newuserdata(L, sizeof(ElunaObject)));
ElunaObjectType* elunaObject = static_cast<ElunaObjectType*>(lua_newuserdata(L, sizeof(ElunaObjectType)));
if (!elunaObject)
{
ELUNA_LOG_ERROR("%s could not create new userdata", tname);
lua_pushnil(L);
return 1;
}
new (elunaObject) ElunaObject(E, const_cast<T*>(obj), manageMemory);
new (elunaObject) ElunaObjectType(E, const_cast<T*>(obj), tname);

// Set metatable for it
lua_pushstring(L, tname);
Expand Down Expand Up @@ -388,17 +433,6 @@ class ElunaTemplate
return 1;
}

static int SetInvalidation(lua_State* L)
{
Eluna* E = Eluna::GetEluna(L);

ElunaObject* elunaObj = E->CHECKOBJ<ElunaObject>(1);
bool invalidate = E->CHECKVAL<bool>(2);

elunaObj->SetValidation(invalidate);
return 0;
}

static int thunk(lua_State* L)
{
ElunaRegister<T>* l = static_cast<ElunaRegister<T>*>(lua_touserdata(L, lua_upvalueindex(1)));
Expand Down Expand Up @@ -429,8 +463,6 @@ class ElunaTemplate

// Get object pointer (and check type, no error)
ElunaObject* obj = E->CHECKOBJ<ElunaObject>(1, false);
if (obj && manageMemory)
delete static_cast<T*>(obj->GetObj());
obj->~ElunaObject();
return 0;
}
Expand Down Expand Up @@ -464,13 +496,6 @@ class ElunaTemplate
static int MethodUnimpl(lua_State* L) { luaL_error(L, "attempt to call a method that is not implemented for this emulator"); return 0; }
};

template<typename T>
ElunaObject::ElunaObject(Eluna* E, T* obj, bool manageMemory) : E(E), callstackid(1), _invalidate(!manageMemory), object(obj), type_name(ElunaTemplate<T>::tname)
{
SetValid(true);
}

template<typename T> const char* ElunaTemplate<T>::tname = NULL;
template<typename T> bool ElunaTemplate<T>::manageMemory = false;

#endif
4 changes: 2 additions & 2 deletions InstanceHooks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ using namespace Hooks;
if (!MapEventBindings->HasBindingsFor(mapKey) && !InstanceEventBindings->HasBindingsFor(instanceKey))\
return;\
PushInstanceData(AI);\
HookPush(AI->instance)
HookPush<Map>(AI->instance)

#define START_HOOK_WITH_RETVAL(EVENT, AI, RETVAL) \
auto mapKey = EntryKey<InstanceEvents>(EVENT, AI->instance->GetId());\
auto instanceKey = EntryKey<InstanceEvents>(EVENT, AI->instance->GetInstanceId());\
if (!MapEventBindings->HasBindingsFor(mapKey) && !InstanceEventBindings->HasBindingsFor(instanceKey))\
return RETVAL;\
PushInstanceData(AI);\
HookPush(AI->instance)
HookPush<Map>(AI->instance)

void Eluna::OnInitialize(ElunaInstanceAI* ai)
{
Expand Down
25 changes: 8 additions & 17 deletions LuaEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,16 +313,6 @@ void Eluna::RunScripts()
OnLuaStateOpen();
}

void Eluna::InvalidateObjects()
{
++callstackid;
#ifdef TRINITY
ASSERT(callstackid, "Callstackid overflow");
#else
ASSERT(callstackid && "Callstackid overflow");
#endif
}

void Eluna::Report(lua_State* _L)
{
const char* msg = lua_tostring(_L, -1);
Expand Down Expand Up @@ -423,11 +413,13 @@ void Eluna::Push()
}
void Eluna::Push(const long long l)
{
ElunaTemplate<long long>::Push(this, new long long(l));
// pushing pointer to local is fine, a copy of value will be stored, not pointer itself
ElunaTemplate<long long>::Push(this, &l);
}
void Eluna::Push(const unsigned long long l)
{
ElunaTemplate<unsigned long long>::Push(this, new unsigned long long(l));
// pushing pointer to local is fine, a copy of value will be stored, not pointer itself
ElunaTemplate<unsigned long long>::Push(this, &l);
}
void Eluna::Push(const long l)
{
Expand Down Expand Up @@ -544,7 +536,8 @@ void Eluna::Push(Object const* obj)
}
void Eluna::Push(ObjectGuid const guid)
{
ElunaTemplate<unsigned long long>::Push(this, new unsigned long long(guid.GetRawValue()));
// pushing pointer to local is fine, a copy of value will be stored, not pointer itself
ElunaTemplate<ObjectGuid>::Push(this, &guid);
}

static int CheckIntegerRange(lua_State* luastate, int narg, int min, int max)
Expand Down Expand Up @@ -650,7 +643,8 @@ template<> unsigned long Eluna::CHECKVAL<unsigned long>(int narg)
}
template<> ObjectGuid Eluna::CHECKVAL<ObjectGuid>(int narg)
{
return ObjectGuid(uint64((CHECKVAL<unsigned long long>(narg))));
ObjectGuid* guid = CHECKOBJ<ObjectGuid>(narg, true);
return guid ? *guid : ObjectGuid();
}

template<> Object* Eluna::CHECKOBJ<Object>(int narg, bool error)
Expand Down Expand Up @@ -1008,9 +1002,6 @@ void Eluna::CleanUpStack(int number_of_arguments)

lua_pop(L, number_of_arguments + 1); // Add 1 because the caller doesn't know about `event_id`.
// Stack: (empty)

if (event_level == 0)
InvalidateObjects();
Copy link
Member

@Rochet2 Rochet2 Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may still want to have the old system to invalidate pushed objects at end of an event stack.
Or at some specific event, like at start of map updates starting.

One of the issues is that if you have an aura for example aura = player:GetAura(spellid), then that aura will be valid until the aura object is destroyed.
But as I understand, aura can be transferred to other maps with a player.

If we dont use some specific event to always invalidate objects, then it can happen that an aura or other object is transferred to another map and can be accessed from two lua states at the same time during map update events when we run multi-state mode eluna.

Any thoughts on this?

Copy link
Contributor Author

@Shauren Shauren Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you don't see this in the PR but aura handle is not invalidated when its destructor runs (which is delayed a bit, first it gets added to m_removedAuras list) but immediately when it is unapplied from unit (see TrinityCore/TrinityCore@a79b42b#diff-f3fb604c220161af7953902aecc9586825b8193413ab94fdeea4de5665d7a393R622)

But you are right about moving to another map - I will need to think about it since I was planning to use this outside of lua too

Copy link
Contributor Author

@Shauren Shauren Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luckily aura already has another reference it can additionally check, its WorldObject owner - so this is what I came up with e676a56

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also Loot functions proposed here.
I wonder how cases like this should be handled where there is a new type exposed that does not necessarily have the unique_trackable_ptr.

I guess introducing new type would always require modifying the cores first to introduce the tracking and then the type can be exposed after.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to think its would be better to only allow preserving references for longer for some types, not all of them (using the old callbackid for the ones we want to exclude)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the primary objects where storing the GUID and fetching the object is the norm is; players, creatures (units) and gameobjects. I'm guessing we can start with those at the very least and expand where it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be addressed by 39eed65

}

/*
Expand Down
7 changes: 0 additions & 7 deletions LuaEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,6 @@ class ELUNA_GAME_API Eluna
// Indicates that the lua state should be reloaded
bool reload = false;

// A counter for lua event stacks that occur (see event_level).
// This is used to determine whether an object belongs to the current call stack or not.
// 0 is reserved for always belonging to the call stack
// 1 is reserved for a non valid callstackid
uint64 callstackid = 2;
// A counter for the amount of nested events. When the event_level
// reaches 0 we are about to return back to C++. At this point the
// objects used during the event stack are invalidated.
Expand All @@ -196,7 +191,6 @@ class ELUNA_GAME_API Eluna
void CloseLua();
void DestroyBindStores();
void CreateBindStores();
void InvalidateObjects();

// Use ReloadEluna() to make eluna reload
// This is called on world update to reload eluna
Expand Down Expand Up @@ -341,7 +335,6 @@ class ELUNA_GAME_API Eluna

void RunScripts();
bool HasLuaState() const { return L != NULL; }
uint64 GetCallstackId() const { return callstackid; }
int Register(uint8 reg, uint32 entry, ObjectGuid guid, uint32 instanceId, uint32 event_id, int functionRef, uint32 shots);
void UpdateEluna(uint32 diff);

Expand Down
Loading
Loading