fix(web): re-check screen-mirror ownership after EnterRPC to avoid orphaning the Flipper#162
Merged
Merged
Conversation
…phaning the Flipper
handleScreenAcquire tentatively claims the mirror (screenHolder = c,
mirrorActive = true) and then runs the slow EnterRPC serial round-trip OUTSIDE
screenMu, but stored the resulting cancel/release/rpcClient without re-checking
that it is still the holder. If the holding WebSocket connection drops while
EnterRPC is in flight, handleWebSocket's defer calls releaseScreen, which sees
a still-nil screenCancel/screenRelease and can only nil the holder (it can't
tear down an RPC session it doesn't yet know about). handleScreenAcquire then
resumed and blindly stored the now-orphaned release func and started the frame
stream. Consequences:
- release() was never called -> Flipper left wedged in RPC mode;
- streamFrames/heartbeat run on a live ctx -> goroutine leak;
- mirrorActive ended up false while RPC was actually live, so
refuseIfMirrorActive wrongly admitted /api/fs, /api/device, and input
requests whose serial commands then collided with the active RPC session.
Fix: after EnterRPC, re-acquire screenMu and verify screenHolder == c. If the
holder changed during the gap, hand the device back (cancel() + release()) and
bail instead of storing orphaned state. The Marauder acquire path is not
affected — it has no slow device-open inside the holder window.
Found via the internal audit sweep.
Test: TestScreen_AcquireReleasedDuringEnterRPC_HandsDeviceBack uses a gated
EnterRPC to inject a holder-disconnect during the gap and asserts release() IS
called and no orphaned screen state remains. Proven to FAIL on the pre-fix code
("Flipper left wedged in RPC mode"). The existing concurrent-acquire invariant
test still passes.
Verified: task ci green (lint 0 / vet / build / test:full 0 fail / govulncheck
clean); task eval 17/17.
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.
Summary
A HIGH-severity acquire/disconnect TOCTOU in the web screen-mirror path that leaves the Flipper wedged + leaks a goroutine (category A/C).
handleScreenAcquiretentatively claims the mirror (screenHolder = c,mirrorActive = true) and then runs the slowEnterRPCserial round-trip outsidescreenMu, storing the resultingcancel/release/rpcClientwithout re-checking ownership. If the holding WebSocket connection drops whileEnterRPCis in flight,handleWebSocket's defer callsreleaseScreen, which sees a still-nilscreenCancel/screenReleaseand can only nil the holder.handleScreenAcquirethen resumes and stores the now-orphanedreleasefunc and starts the frame stream:release()is never called → Flipper left wedged in RPC mode;streamFrames/heartbeat run on a live ctx → goroutine leak;mirrorActiveends up false while RPC is live, sorefuseIfMirrorActivewrongly admits/api/fs,/api/device, and input requests whose serial commands then collide with the active RPC session.Fix
After
EnterRPC, re-acquirescreenMuand verifyscreenHolder == c. If the holder changed during the gap, hand the device back (cancel()+release()) and bail instead of storing orphaned state. (The Marauder acquire path is unaffected — no slow device-open inside its holder window.)Found via the internal audit sweep.
Verification
TestScreen_AcquireReleasedDuringEnterRPC_HandsDeviceBackuses a gatedEnterRPCto inject a holder-disconnect during the gap and assertsrelease()IS called and no orphaned state remains. Proven to FAIL on the pre-fix code ("Flipper left wedged in RPC mode"). The existing concurrent-acquire invariant test still passes.task cigreen (lint 0 / vet / build /test:full0 fail / govulncheck clean);task eval17/17.Bug fix — anchors v0.780.0.