fix(mobile): close duplicated TUN fd after engine.Stop to prevent fd leak - #18
Merged
Conversation
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
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.
Problem
StartTunandStartTunBridgecalldupFd(int(fd))before handing theduplicated fd to
engine.Insert(key). The dup is necessary to avoid aSIGSEGV race with Android's
ParcelFileDescriptor.close()(commit41c3eef).However, the dup'd fd is never closed by the Go side.
engine.Stop()closesthe fd it registered — but only on the happy path where
_defaultDeviceisnon-nil. If
engine.Stop()panics (therecover()atmobile.go:282/328swallows it) or
engine.Start()never completed, the fd leaks. On Android,where users hot-connect profiles many times per session, this leaks one fd per
cycle until the kernel refuses (
EMFILE— "VPN interface could not beestablished").
Fix
tunOwnedFd int32 = -1(guarded byengineMu) to track the mostrecently dup'd fd inserted into the engine.
StartTun/StartTunBridge: defensively close any pre-existing tracked fdbefore re-inserting, then record the new
safeFd.StopTun/StopTunBridge: explicitsyscall.Close(tunOwnedFd)+ reset to-1after therecover()-wrappedengine.Stop().recover()(commit41c3eef) is preserved in both Stopfunctions.
StopTunBridge'sengineMu.Unlock()→tun.StopFakeDNSProxy()ordering ispreserved.
On the happy path (tun2socks closes the fd via
_defaultDevice.Close()), theexplicit close is a harmless double-close (
EBADFis ignored). The benefit ison the unhappy paths where tun2socks does NOT close the fd.
Files changed
mobile/mobile.go— +25/-1Verification
go vet ./mobile/...passes (on Linux target; Windows reports false-positiveon
syscall.Dupwhich is unix-only)gofmt -l mobile/mobile.go— clean for this patch (residual pre-existingissues in surrounding code are out of scope)
gomobile bind+gradlew assembleDebugverified via CITest plan
No new unit tests. Runtime verification: connect/disconnect a VPN profile ~30
times on a real device and confirm no
EMFILE/ "VPN interface could not beestablished" error.
Closes plan 004.