Server crash: GetBinding() returns nullptr by design but 52 call sites dereference it unchecked

[secoonday]

Summary

Eluna::GetBinding<T>() is written to return nullptr, but the returned pointer is dereferenced without a null check in 52 call sites (AzerothCore build). This crashed our production server.

Crash

AzerothCore 3.3.5a, rev 007b4464f5d2 (2026-07-22), ~50 concurrent players. Backtrace from a core dump:

#0  std::_Hashtable<EventKey<Hooks::ServerEvents>, ...>::empty (this=0x18)
#1  std::unordered_map<...>::empty (this=0x18)
#2  BindingMap<EventKey<Hooks::ServerEvents>>::HasBindingsFor (this=0x0, key=...)
        at src/server/game/LuaEngine/BindingMap.h:165
#3  Eluna::OnPacketSendAny (this=0x715637fc0280, player=0x7155c67f4c00, packet=..., result=@0x7156255fb297: true)
        at src/server/game/LuaEngine/hooks/PacketHooks.cpp:40
#4  Eluna::OnPacketSend (this=0x715637fc0280, session=<optimized out>, packet=...)
        at src/server/game/LuaEngine/hooks/PacketHooks.cpp:34
#5  WorldSession::SendPacket (this=0x7155a135c100, packet=0x7156255fb2e8)
        at src/server/game/Server/WorldSession.cpp:351
#6  Group::UpdatePlayerOutOfRange (this=<optimized out>, player=0x71554076d7c0)
        at src/server/game/Groups/Group.cpp:1888
#7  Player::SendUpdateToOutOfRangeGroupMembers (this=0x71554076d7c0)
        at src/server/game/Entities/Player/Player.cpp:11946
#8  Player::Update (this=0x71554076d7c0, p_time=1)
        at src/server/game/Entities/Player/PlayerUpdates.cpp:391
#9  Map::Update (this=0x715635914300, t_diff=100, s_diff=1)
        at src/server/game/Maps/Map.cpp:503

BindingMap::HasBindingsFor is called with this=0x0.

Worth noting: no Lua script on our server registers any packet event. The hook that crashed the server does nothing for us — it fires on every outgoing packet only to check a binding map that happened to be null.

Root cause

LuaEngine.h:392 — GetBinding() has two explicit return nullptr paths:

template<typename T>
BindingMap<T>* GetBinding(std::underlying_type_t<Hooks::RegisterTypes> type)
{
    if (type >= Hooks::REGTYPE_COUNT)
        return nullptr;

    auto& binding = bindingMaps[type];
    if (!binding)
        return nullptr;

    return dynamic_cast<BindingMap<T>*>(binding.get());
}

dynamic_cast can also yield nullptr on type mismatch.

Every START_HOOK* macro then does:

auto binding = GetBinding<...>(REGTYPE_...);
auto key = ...;
if (!binding->HasBindingsFor(key))   // <-- unchecked dereference
    return;

Scope

Call sites that dereference the result without a null check, in the code actually compiled for AzerothCore:

Location	Sites
hooks/ (13 files)	27
methods/AzerothCore/GlobalMethods.h	16
LuaEngine.cpp	9
Total	52

The same GlobalMethods.h pattern is duplicated per core, so methods/{Mangos,CMangos,VMangos,TrinityCore} add 16 sites each — roughly 116 sites across all supported cores.

Suggested fix

Guard at the call sites (what we did locally, 52 changes, running in production):

if (!binding || !binding->HasBindingsFor(key))
    return;

Alternatively — and probably better — have the START_HOOK* macros do the null check once, so new hooks cannot reintroduce the bug. A third option is returning a reference to a shared empty BindingMap instead of nullptr, which removes the failure mode entirely.

Reproduction

We could not reproduce on demand: it happened once in ~17 hours of uptime with ~50 players. The core dump backtrace above is the evidence.

Happy to test a patch against a live server.

[Foereaper]

Interesting, I think the suggested fix might be the wrong approach and would only mask a separate and more severe underlying issue. I would much rather the core crash, to catch those issues, than to mask it.

In essence, an Eluna state should never not have the binding maps for all register types. This is regardless if there are any scripts registering a hook to a bind store or not. We create the binding store when the Lua state opens and destroy it when the Lua state closes. This is done through CreateBindStores and DestroyBindStores.

What it seems like to me is possibly a threading issue, considering the callstack you provided, but I would have to look into it more.

[secoonday]

You're right, and I'd rather help you find the real bug than have the null checks merged. Let me give you what points at the race.

The Eluna object was alive — only its bindings were gone

That's the part of the backtrace I under-read at first:

#2  BindingMap<...>::HasBindingsFor (this=0x0, key=...)
#3  Eluna::OnPacketSendAny (this=0x715637fc0280, player=0x7155c67f4c00, ...)

this=0x715637fc0280 in frame #3 is a valid Eluna*. So this isn't a destroyed state being used — it's a live state whose bindingMaps[REGTYPE_SERVER] had been reset. Which is exactly the situation you say should never happen.

Two windows where that is true

CloseLua() nulls the stores while the object is still very much alive, and before the lua state is even closed:

void Eluna::CloseLua()
{
    OnLuaStateClose();
    DestroyBindStores();      // <-- all bindingMaps reset here
    if (L)
        lua_close(L);         // <-- object still alive, still reachable
    L = NULL;
    ...
}

And CreateBindStores() opens the same window on the way up, because it resets everything before recreating:

void Eluna::CreateBindStores()
{
    DestroyBindStores();                                        // <-- all null
    CreateBinding<EventKey<Hooks::ServerEvents>>(REGTYPE_SERVER);   // <-- then one
    CreateBinding<EventKey<Hooks::PlayerEvents>>(REGTYPE_PLAYER);   //     by one
    ...
}

In both windows GetBinding() legitimately returns nullptr for a state that is otherwise perfectly usable. Every hook that fires in that interval hits it.

Why this matches our crash

The crash came off MapUpdater::WorkerThread → Map::Update → Player::Update, i.e. a map update thread, not the world thread:

#6  Group::UpdatePlayerOutOfRange
#7  Player::SendUpdateToOutOfRangeGroupMembers
#8  Player::Update
#9  Map::Update
#10 MapUpdateRequest::call        MapUpdater.cpp:46
#11 MapUpdater::WorkerThread      MapUpdater.cpp:186

We run multistate, so each map owns its Eluna. No reload eluna had been issued anywhere near that time — the server had been up 17 hours untouched — so whatever tore down or brought up a state was map lifecycle, concurrent with a map update worker.

So the guards I proposed would indeed paper over exactly the thing you'd want to see.

Where that leaves us

To be transparent about what we're running: we do have the 52 guards applied locally, because the alternative is a hard crash with ~50 players online and we can't hold that open while this is investigated. We're treating it as a tourniquet, not a fix, and we'll drop it the moment there's a proper one — it's recorded that way in our own notes.

Happy to help from the production side:

• The 5 GB core dump is still on disk and the binary has symbols (RelWithDebInfo), so I can pull thread apply all bt and tell you what the other threads were doing at that instant — say the word and I'll paste it.
• Or if you want a diagnostic build, I can run one that logs on every GetBinding() returning null (state pointer, regtype, thread id) and let it sit under real load until it fires again.

Whichever is more useful to you.

[Foereaper]

I looked into it, it is definitely a case where either an Eluna object has been created or is closing and the BindStores do not exist, and a separate thread from a different map tries to fire off a hook in the owning maps' event handler.

I would assume this probably happened when creating or destroying a dungeon map, which would make sense with multistate and the packet update being sent to a group member that did not exist on player 1's map.

The proper fix in this case is to implement an Eluna atomic lifetime state machine, and only ever accept events if Eluna is in a READY state. When we create the Eluna object, we default it to NOT_READY, then switch to INITIALIZING whenever we load the scripts, create the bind stores, register methods etc. Only when all of that is done we switch to READY. When we reload, or tear down Eluna, we switch to DRAINING, and allow all the currently running hooks to finish before we move to CLOSING. We can keep track of the currently running hooks with an atomic counter.

I'll look into implementing something like this over the next few days/weekend, unless someone beats me to it. The nullptr check is fine enough for now, but you should revert that whenever the proper fix is in place.

Note: The proposed state machine would only fix issues where events are attempted to be fired when the state is not ready to deal with them, it would not fix the issue that we have a concurrency problem where Map A's thread is accessing and executing events on Map B's Eluna object. Maybe @Shauren has some thoughts on this matter, since it can also affect other C++ scripts through ScriptMgr on the same path?

[secoonday]

That matches what we saw, and the lifetime state machine with an in-flight counter sounds like the right shape — a DRAINING phase that lets running hooks finish is the part a plain guard can't give you.

Understood on reverting. The 52 guards are recorded in our notes as a tourniquet with your name on the condition, so they come out as soon as the state machine is in.

Offer, since it may be the awkward part to test: we can put a build under real concurrency. ~50 players at peak, multistate, instances being created and torn down continuously, and the crash reproduced twice in three days on its own without anyone trying. If you want, once you have something, I'll run it unpatched (guards reverted, so nothing masks a regression) and report back — either it holds for a few days under load, or the autopsy hands you a backtrace with symbols. We keep core dumps and build RelWithDebInfo.

Also happy to run an instrumented build first if you'd rather confirm the map create/destroy path before designing around it — e.g. logging state pointer, regtype, thread id and map id on every hook that finds a null store. Whichever helps more.

No rush from our side; the guards hold us fine in the meantime.

[Shauren]

Dig out the value of Eluna::bindingMaps from your core dump