Fix use-after-free of FSocketIONative from its own game-thread callbacks - #462
Fix use-after-free of FSocketIONative from its own game-thread callbacks#462marekl11 wants to merge 1 commit into
Conversation
|
Sanity check: does this make it cache fail (atomic access behavior) on every access call? If so, I'd prefer a fix without the atomic cost if possible (might be a bigger refactor though). |
|
I checked, and no: this adds no atomic on the access path, and on UE5 it adds no atomic at all. Two things: ESPMode only selects the ref-count type in the reference controller, using RefCountType = std::conditional_t<Mode == ESPMode::ThreadSafe, std::atomic, int32>. TSharedPtr::operator->/Get() just return the cached raw Object pointer, so calls through the pointer are identical codegen either way. Atomics run only when a reference is added or dropped. Since UE5, TSharedPtr's default mode is already ESPMode::ThreadSafe (template<class ObjectType, ESPMode Mode = ESPMode::ThreadSafe> in SharedPointerFwd.h, checked on 5.6 and 5.8). So the existing unannotated TSharedPtr was already the atomic instantiation, the , ESPMode::ThreadSafe in this PR is explicit-not-different. The only new refcount traffic is one Pin() per deferred internal callback, i.e. per connect/disconnect/namespace/fail/reconnect event, never per emitted message. Worth noting the other direction too: ReleaseNativePointer copies the TSharedPtr into RunLambdaOnBackGroundThread and drops it on that thread while the game thread still holds copies in PluginNativePointers and the component, so the atomic refcount isn't optional here regardless of this PR. If you'd prefer a smaller diff, I can drop the explicit ESPMode::ThreadSafe annotations (they're redundant on UE5) and leave only the TSharedFromThis + weak-Pin() changes — that removes SocketIOClient.h, SocketIOClientComponent.h and the README from the diff entirely. I left them in mainly as self-documentation and in case the 4.27 backport branch ever picks this up, where the default is Fast. Happy to go either way. |
|
Great clarification, yeah lets drop the explicit annotations. General maintenence strategy is forward compatibility only for new versions. |
SetupInternalCallbacks() marshals each internal listener (connect/disconnect/ namespace/fail/reconnect) onto the game thread via RunShortLambdaOnGameThread, and those lambdas captured the FSocketIONative by reference. The deferred task lives on the game-thread task graph independently of the object, so if the native client is released before the task runs, the lambda dereferences freed memory. This reproduces during a UE level transition: tearing down the old world releases the client on a background thread, and LoadMap's own FlushRenderingCommands then pumps the game-thread task graph and runs the stale callback. Observed as an access violation reading 0xffffffffffffffff on the game thread, top frame being the namespace-disconnect lambda. FSocketIONative now derives from TSharedFromThis and each deferred lambda captures a TWeakPtr and Pin()s it, so it no-ops if the object is already gone. A weak (not strong) capture avoids extending the object's lifetime and moving its destruction (which joins the network thread) onto the game thread. This is the same class of fix as the component-level weak-pointer guard, one layer down at the native object. The refcount has to be atomic, since the object can be released on a background thread while the lambda Pin()s it on the game thread; on UE5 that needs no explicit mode, as TSharedPtr/TWeakPtr/TSharedFromThis already default to ESPMode::ThreadSafe. The outer asio-thread listeners intentionally keep their [&] capture: PrivateClient is the last member, so ~FSocketIONative destroys it first and the network thread is joined before the other members are torn down.
06f1c7d to
edf6871
Compare
|
Done! annotations dropped, PR is down to the two files that carry the actual fix. Rebased the description to match |
SetupInternalCallbacks() marshals each internal listener (connect/disconnect/
namespace/fail/reconnect) onto the game thread via RunShortLambdaOnGameThread,
and those lambdas captured the FSocketIONative by reference. The deferred task
lives on the game-thread task graph independently of the object, so if the native
client is released before the task runs, the lambda dereferences freed memory.
This reproduces during a UE level transition: tearing down the old world releases
the client on a background thread, and LoadMap's own FlushRenderingCommands then
pumps the game-thread task graph and runs the stale callback. Observed as an
access violation reading 0xffffffffffffffff on the game thread, top frame being
the namespace-disconnect lambda.
FSocketIONative now derives from TSharedFromThis and each deferred lambda captures
a TWeakPtr and Pin()s it, so it no-ops if the object is already gone. A weak (not
strong) capture avoids extending the object's lifetime and moving its destruction
(which joins the network thread) onto the game thread. This is the same class of
fix as the component-level weak-pointer guard, one layer down at the native object.
The refcount has to be atomic, since the object can be released on a background
thread while the lambda Pin()s it on the game thread; on UE5 that needs no explicit
mode, as TSharedPtr/TWeakPtr/TSharedFromThis already default to ESPMode::ThreadSafe.
The outer asio-thread listeners intentionally keep their [&] capture: PrivateClient
is the last member, so ~FSocketIONative destroys it first and the network thread is
joined before the other members are torn down.