fix(eventbus): Fix use-after-free crash in DispatchEvent (line 741)
ROOT CAUSE: The crash occurred when dispatching events to handlers that
had been deleted between validation and dispatch. This was triggered when
Arathi Basin battleground ended - players are removed and their BotAI
objects deleted while events are still being dispatched.
The previous validation at line 730 only checked if the GUID existed in
_subscriberPointers, but did NOT validate that the pointer was still the
same. This allowed three failure modes:
1. Bot unsubscribed (GUID removed) - correctly skipped
2. Bot deleted and REPLACED with new bot at same GUID - dispatched to WRONG object!
3. Bot deleted but GUID not yet removed from map - dispatched to FREED memory! (CRASH)
SOLUTION: Store the original BotAI* pointer alongside the handler and validate
BOTH GUID existence AND pointer match during the second validation pass.
This catches cases 2 and 3 where the underlying object has changed.
Changes:
- handlersToDispatch now stores struct with {guid, botAI*, handler*}
- Validation now checks: it->second == info.botAI (pointer match)
- Added catch(...) block to catch any remaining edge cases
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Signed-off-by: luis <[email protected]>
This commit is contained in:
committed by
luis
co-authored by
Claude Opus 4.5
parent
bc739e0c8d
commit
b60bcc4c91
@@ -671,7 +671,16 @@ private:
|
|||||||
// map, not our local copy, so iteration remains safe.
|
// map, not our local copy, so iteration remains safe.
|
||||||
|
|
||||||
// Collect handlers to dispatch while holding the lock
|
// Collect handlers to dispatch while holding the lock
|
||||||
std::vector<std::pair<ObjectGuid, IEventHandler<TEvent>*>> handlersToDispatch;
|
// CRITICAL FIX (GenericEventBus.h:741 crash): Store BOTH BotAI* and handler*
|
||||||
|
// so we can validate the EXACT pointer match during the second pass.
|
||||||
|
// Checking only GUID existence is insufficient - the BotAI object could be
|
||||||
|
// deleted and replaced with a new one at the same GUID between passes!
|
||||||
|
struct HandlerInfo {
|
||||||
|
ObjectGuid guid;
|
||||||
|
BotAI* botAI;
|
||||||
|
IEventHandler<TEvent>* handler;
|
||||||
|
};
|
||||||
|
std::vector<HandlerInfo> handlersToDispatch;
|
||||||
|
|
||||||
{
|
{
|
||||||
std::lock_guard lock(_subscriptionMutex);
|
std::lock_guard lock(_subscriptionMutex);
|
||||||
@@ -708,11 +717,11 @@ private:
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add to dispatch list
|
// Add to dispatch list with BOTH pointers for validation
|
||||||
handlersToDispatch.emplace_back(subscriberGuid, handler);
|
handlersToDispatch.push_back({subscriberGuid, botAI, handler});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Lock released here - safe to call handlers now
|
// Lock released here
|
||||||
|
|
||||||
// CRITICAL FIX: Validate ALL handlers with SINGLE lock acquisition
|
// CRITICAL FIX: Validate ALL handlers with SINGLE lock acquisition
|
||||||
// PROBLEM: Previous implementation acquired _subscriptionMutex FOR EVERY HANDLER
|
// PROBLEM: Previous implementation acquired _subscriptionMutex FOR EVERY HANDLER
|
||||||
@@ -720,32 +729,48 @@ private:
|
|||||||
// acquisitions per tick, causing SEVERE lock contention and lag.
|
// acquisitions per tick, causing SEVERE lock contention and lag.
|
||||||
//
|
//
|
||||||
// SOLUTION: Build list of valid handlers while holding lock ONCE, then dispatch
|
// SOLUTION: Build list of valid handlers while holding lock ONCE, then dispatch
|
||||||
// without any locks. Risk of dispatching to recently-unsubscribed handler
|
// without any locks.
|
||||||
// is acceptable (handler should handle gracefully).
|
//
|
||||||
std::vector<std::pair<ObjectGuid, IEventHandler<TEvent>*>> validHandlers;
|
// CRITICAL FIX (GenericEventBus.h:741 crash): Validate POINTER MATCH, not just GUID!
|
||||||
|
// The previous code only checked if GUID existed in _subscriberPointers. This fails if:
|
||||||
|
// 1. Bot is unsubscribed (GUID removed) - would skip dispatch (correct)
|
||||||
|
// 2. Bot is deleted and replaced with new bot at same GUID - would dispatch to wrong object!
|
||||||
|
// 3. Bot is deleted but GUID not yet removed - would dispatch to freed memory! (CRASH)
|
||||||
|
//
|
||||||
|
// By also checking that the BotAI* pointer matches, we catch case 2 and 3.
|
||||||
|
std::vector<HandlerInfo> validHandlers;
|
||||||
{
|
{
|
||||||
std::lock_guard lock(_subscriptionMutex);
|
std::lock_guard lock(_subscriptionMutex);
|
||||||
for (auto const& [subscriberGuid, handler] : handlersToDispatch)
|
for (auto const& info : handlersToDispatch)
|
||||||
{
|
{
|
||||||
if (_subscriberPointers.find(subscriberGuid) != _subscriberPointers.end())
|
auto it = _subscriberPointers.find(info.guid);
|
||||||
validHandlers.emplace_back(subscriberGuid, handler);
|
// CRITICAL: Check BOTH guid existence AND pointer match!
|
||||||
|
if (it != _subscriberPointers.end() && it->second == info.botAI)
|
||||||
|
validHandlers.push_back(info);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Lock released - dispatch without lock contention
|
// Lock released - dispatch without lock contention
|
||||||
|
|
||||||
// Dispatch events to validated handlers
|
// Dispatch events to validated handlers
|
||||||
for (auto const& [subscriberGuid, handler] : validHandlers)
|
for (auto const& info : validHandlers)
|
||||||
{
|
{
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
handler->HandleEvent(event);
|
info.handler->HandleEvent(event);
|
||||||
TC_LOG_TRACE("playerbot.events", "EventBus: Dispatched event to bot {}: {}",
|
TC_LOG_TRACE("playerbot.events", "EventBus: Dispatched event to bot {}: {}",
|
||||||
subscriberGuid.ToString(), event.ToString());
|
info.guid.ToString(), event.ToString());
|
||||||
}
|
}
|
||||||
catch (std::exception const& e)
|
catch (std::exception const& e)
|
||||||
{
|
{
|
||||||
TC_LOG_ERROR("playerbot.events", "EventBus: Exception in event handler for bot {}: {}",
|
TC_LOG_ERROR("playerbot.events", "EventBus: Exception in event handler for bot {}: {}",
|
||||||
subscriberGuid.ToString(), e.what());
|
info.guid.ToString(), e.what());
|
||||||
|
}
|
||||||
|
catch (...)
|
||||||
|
{
|
||||||
|
// CRITICAL: Catch ALL exceptions including access violations during dispatch
|
||||||
|
// This can happen if BotAI is deleted between validation and dispatch
|
||||||
|
TC_LOG_ERROR("playerbot.events", "EventBus: Unknown exception in event handler for bot {}",
|
||||||
|
info.guid.ToString());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user