fix(apple): Only use connlib sessions that are connected#8104
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Sentry Issue: APPLE-CLIENT-2A |
|
Sentry Issue: APPLE-CLIENT-28 |
|
Sentry Issue: APPLE-CLIENT-27 |
|
Sentry Issue: APPLE-CLIENT-1N |
|
Using the session pointer in this mimics the pattern we have in Android's TunnelService. |
| } | ||
| session?.disconnect() | ||
|
|
||
| session = nil |
There was a problem hiding this comment.
I believe this is the important fix. When we call onDisconnect, the pointer to the session is still valid. It is when you call disconnect that we free the memory.
Arguably, this is a shortcoming of swift-bridge I think where it doesn't model correctly, that we capture self entirely in the disconnect call.
There was a problem hiding this comment.
| // Immediately invalidate our session pointer to prevent workQueue items from trying to use it | ||
| session = nil |
There was a problem hiding this comment.
Just to be sure we don't leak memory, can you insert a call to disconnect here? Otherwise I think you'd have to free the memory.
There was a problem hiding this comment.
Oh, is that the case? So connlib calls our on_disconnect and then expects us to issue a final disconnect() afterward?
There was a problem hiding this comment.
We'll also need to update the Android client if so
There was a problem hiding this comment.
Oh, is that the case? So connlib calls our
on_disconnectand then expects us to issue a finaldisconnect()afterward?
I didn't think about it until now to be honest. However, I can't see how Swift would be able to correctly free the memory and we can't free it from within the callback. From Swift's perspective, all it has is a pointer and it is discarding it on the call to nil so I believe this is a memory leak.
It is only in disconnect that we take full ownership of self and therefore Rust will drop it at the end of the function.
We need to call `disconnect()` in `onDisconnect` to free the memory associated with the connlib session. Related: #8104
2 participants
In the window of time between we check
AdapterState == .tunnelStartedand we callsetDnsin the ApplepathUpdateHandler, it's possible that connlib disconnected. This window of time could potentially be non-trivial since we read system resolvers in there, which hits the disk.As such, we should always check the
sessionpointer is valid just before use.The
AdapterStateenum tracks two states:tunnelStoppedandtunnelStarted. In thetunnelStartedstate, we populate aWrappedSessionobject. This is redundant - connlib is eitherconnectedand we have aWrappedSession, or it is not. Therefore we can remove theAdapterStateabstraction completely (which was leftover from a previous developer) and directly use aWrappedSession?object to issue calls to connlib with.We set this to a valid
WrappedSessionupon connecting, and back tonilas soon as connlib eitheronDisconnects us, or the user disconnects the tunnel.Lastly, we avoid early-returning from queued workItems because we now call connlib with
session?which will no-op if there is no session, allowing whatever IPC call running at the time (such as fetchResources) to complete successfully, even though they'll see a "snapshotted" state of the Adapter/PacketTunnelProvider. In other words, we no longer enforce the session pointer to be valid for things that don't depend on its state.Fixes #7882