Harden ShareProxy: validate ports, avoid blocking the main actor, fix a shutdown race - #2
Open
AadhilFarhan wants to merge 2 commits into
Open
Harden ShareProxy: validate ports, avoid blocking the main actor, fix a shutdown race#2AadhilFarhan wants to merge 2 commits into
AadhilFarhan wants to merge 2 commits into
Conversation
… a shutdown race
- ShareProxy.init force-unwrapped an NWEndpoint.Port built from an
unvalidated Int, trapping on any out-of-range port. Now validates
1...65535 and throws ShareProxyError instead, matching the project's
degrade-don't-crash convention. Reachable today via the CLI's
--share flag with no prior validation.
- ShareProxy.init can block its calling thread briefly waiting for the
listener to come up. AppModel.startSharing called it synchronously
on the main actor from QRPanel's onAppear, so tapping the QR button
could freeze the UI. startSharing is now async and dispatches the
construction the same way PortScanner.scan() already is; re-checks
shares[port] after resuming in case another call raced it.
- stop() could lose a race with an in-flight accept: a connection
already progressing through relay() when cancel() ran could still
land in the connections dict after stop() had already drained it,
leaking an un-cancelled relay. relay() now checks a stopped flag
under the same lock stop() uses.
- Added deinit { stop() } as a safety net if a caller drops the last
reference without calling stop() explicitly.
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
1 participant
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.
What
Three related safety gaps in the LAN-sharing relay, bundled since they're all "make ShareProxy's lifecycle safe":
NWEndpoint.Port(rawValue: UInt16(targetPort))!trapped on any port outside0...65535. Reachable today viaPortside --share 99999with no prior validation.initnow validates and throwsShareProxyError.invalidPortinstead.ShareProxy.initblocks briefly (up to 2s) waiting for its listener to come up, via a semaphore wait.AppModel.startSharingcalled it synchronously on the main actor, straight fromQRPanel.onAppear— so tapping the QR button could freeze the UI, the same class of bug the project already guards against forPortScanner.scan().startSharingis nowasyncand dispatches construction the same wayPortScanner.scan()already is, with a re-check ofshares[port]after resuming in case another call raced it while suspended.relay()whenstop()runs could still land in theconnectionsdict afterstop()had already drained it — leaking a relay that's never cancelled.relay()now checks astoppedflag under the same lockstop()uses. Addeddeinit { stop() }as a safety net for callers that drop the last reference without callingstop().Test plan
ShareProxyTests: port0, negative, and70000all throwShareProxyError.invalidPortwithout touching the network (validation runs before the listener is ever created, so these are fast and non-flaky).Portside --share 99999now prints a clean error and exits 1 instead of crashing.Portside --share, curled the LAN address — HTTP 200, confirming the async/dispatch change didn't break the legitimate path.