Fix use-after-free in the pre-hop part of the socket callbacks - #466
Open
marekl11 wants to merge 2 commits into
Open
Fix use-after-free in the pre-hop part of the socket callbacks#466marekl11 wants to merge 2 commits into
marekl11 wants to merge 2 commits into
Conversation
getnamo#462 gave the game-thread hops inside the internal listeners a weak self to pin, but the listener bodies themselves still ran off a captured `this`. Those bodies execute on the asio network thread, and sio holds the listeners for as long as the client lives — so everything before the hop (reading VerboseLog, writing SessionId/bIsConnected, testing which callback is bound) was still reading and writing a FSocketIONative that may already be gone. Same shape as the bug that PR fixed, one level up. SetupInternalCallbacks now takes one weak ref and each listener pins it first, which covers the body and the hop together — the hops reuse the same weak ref instead of calling AsShared() again from inside. Both callers run well after construction, so AsShared() at registration time is fine. Three more with the same problem: - Connect's background lambda touches PrivateClient, MaxReconnectionAttempts and ReconnectionDelay on a pool thread. connect() can sit there for a TCP timeout, which is plenty of time for a level transition to release the socket underneath it. - EmitRaw's ack callback and OnRawBinaryEvent's listener both read bCallbackOnGameThread on the asio thread before deciding where to dispatch. And on the component side, the three bindings SetupCallbacks' weak-this treatment never got extended to: BindEventToGenericEvent broadcasts a member delegate, BindEventToFunction and EmitWithCallBack call member functions. No behaviour change when the object is alive, which is every normal case.
getnamo
self-requested a review
September 1, 2026 19:59
SetupInternalCallbacks() takes a weak reference to the client so each listener body can check that the object is still alive before touching a member. That reference comes from AsShared(), which requires the object to already be owned by a TSharedPtr. The constructor calls ClearAllCallbacks(), which calls SetupInternalCallbacks() — at that point MakeShareable() has not yet run, so AsShared() trips the DoesSharedInstanceExist() assertion and the process dies as soon as anything creates a client. Skip the listener install on that pass and repeat ClearAllCallbacks() in NewValidNativePointer() once the shared pointer exists. Callers after construction are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #462. That one gave the game-thread hops inside the internal listeners a weak self to pin, but the listener bodies themselves still ran off a captured
this— and those bodies execute on the asio network thread, for as long as sio holds the listener. So everything before the hop (readingVerboseLog, writingSessionId/bIsConnected, testing which callback is bound) was still touching anFSocketIONativethat may already be gone. Same bug, one level up.SetupInternalCallbacksnow takes one weak ref and each listener pins it first, which covers the body and the hop together — the hops reuse that ref instead of callingAsShared()again from inside. Both callers (ClearAllCallbacks,RebindCurrentEventMap) run well after construction, soAsShared()at registration time is safe.Three more with the same shape:
Connect's background lambda touchesPrivateClient,MaxReconnectionAttemptsandReconnectionDelayon a pool thread.connect()can sit there for the length of a TCP timeout, which is plenty of room for a level transition to release the socket underneath it.EmitRaw's ack callback andOnRawBinaryEvent's listener both readbCallbackOnGameThreadon the asio thread before deciding where to dispatch.And on the component side, the three bindings that
SetupCallbacks' weak-this treatment never got extended to:BindEventToGenericEventbroadcasts a member delegate,BindEventToFunctionandEmitWithCallBackcall member functions.No behaviour change while the object is alive, which is every normal case.
Note on overlap: #465 touches some of the same lambdas in
SocketIONative.cpp, but only the ones whose capture lists were already dead ([&]alongside explicit captures of everything the body used). This branch deliberately leaves those alone and only changes the ones that genuinely reach for a member, so the two shouldn't fight. Happy to rebase whichever lands second.