Skip to content

Fix use-after-free of FSocketIONative from its own game-thread callbacks - #462

Open
marekl11 wants to merge 1 commit into
getnamo:mainfrom
marekl11:fix/native-deferred-callback-uaf
Open

Fix use-after-free of FSocketIONative from its own game-thread callbacks#462
marekl11 wants to merge 1 commit into
getnamo:mainfrom
marekl11:fix/native-deferred-callback-uaf

Conversation

@marekl11

@marekl11 marekl11 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

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.

@getnamo

getnamo commented Jul 9, 2026

Copy link
Copy Markdown
Owner

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).

@marekl11

Copy link
Copy Markdown
Contributor Author

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.

@getnamo

getnamo commented Aug 13, 2026

Copy link
Copy Markdown
Owner

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.
@marekl11
marekl11 force-pushed the fix/native-deferred-callback-uaf branch from 06f1c7d to edf6871 Compare August 13, 2026 21:08
@marekl11

Copy link
Copy Markdown
Contributor Author

Done! annotations dropped, PR is down to the two files that carry the actual fix. Rebased the description to match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants