fix(capture): isolate input with EVIOCGRAB instead of xinput disable - #33
Merged
Conversation
Device isolation used `xinput disable`, which is global X11 state whose lifetime is not tied to the process. Undoing it was procedural, so any path that skipped `enableXinput` stranded the machine with no mouse and no keyboard: the remote never handing the cursor back, a crash, a SIGKILL, a wedged connection. Nothing in the system would restore them, and recovering meant replugging hardware — which could crash Xorg and drop the session. Replace it with EVIOCGRAB, where ownership is the file descriptor. The kernel drops the grab when the fd closes, including on crash, SIGKILL and OOM, so restoring local input is structural rather than something a cleanup path can miss. This also removes the X device-hierarchy churn that `xinput` produced, and with it the ~1-2s compositor stall on return. An earlier EVIOCGRAB attempt was reverted in d407e88. It kept xinput as a fallback, so the churn and its compositor stall remained and the cursor warp on return took ~750ms with the grab held across it. With xinput gone the warp measures 3-4ms. The synchronous warp-then-release ordering is kept deliberately; releasing first and warping asynchronously is what caused the rubberband regression in 22f85ce. Classify devices by capability bitmask, mirroring udev's input_id, instead of matching `razer|wooting` in the name. Laptop touchpads, touchscreens and tablets are now isolated; power and sleep buttons, lid switches and audio-jack detection are deliberately left alone so the physical power button stays available as a recovery path. Also fixes three defects found while verifying this: - Stop() could hang forever. Devices were opened blocking, so their fds were not registered with Go's poller and Close() did not interrupt a parked read(2); a monitor goroutine for a silent device never returned. Open with O_NONBLOCK. - The tracked device set was captured once at startup, so a wireless receiver re-enumerating on wake was never isolated and leaked motion to the local display. grabInput refreshes the set first, and vanished devices are dropped. - Never grab virtual (uinput) devices. Grabbing mwb's own mwb-mouse and mwb-keyboard would swallow the events it injects, silently breaking remote typing and clicking. Other tools' output devices are skipped for the same reason. xinput is no longer a runtime dependency; drop it from the deb, the goreleaser package and the installer.
`mwb update` fetches the release artifact for the running architecture, verifies it against the SHA-256 published in that release's checksums.txt, and replaces the binary atomically, so an interrupted update leaves either the old binary or the new one and never a truncated file. It refuses unverified downloads, non-HTTPS URLs and any host that is not GitHub. Installs from the .deb are left to dpkg rather than overwritten behind the package manager's back, and an unwritable destination reports the sudo requirement instead of a bare permission error. The running service keeps the old binary until restarted, so the restart command is printed. `mwb version` prints the installed version. The release pipeline already passed -X main.version, but no such variable existed, so the ldflag was silently doing nothing and every release build was unversioned.
…he caller
Outbound switches run on pollCursorEdge; returns run on the network handler via
SetActive. With each caller stating its own intent, the release belonging to a
finished switch-back could land after the grab for the next switch-out and
un-grab devices while the cursor was on the remote machine, leaving local input
live — a window of dual-cursor movement.
Observed live:
44.372 remote edge hit — switching back to Ubuntu
44.607 screen edge hit, switching to remote
44.608 WARN grab input device event20 "device or resource busy"
44.613 grabbed local input devices count=11 of=12
44.629 released local input devices count=12 of=12 <- stale
applyIsolation now serializes on grabMu and re-reads c.active inside it, so a
late caller converges on the true state instead of overriding a newer one.
setIsolation also skips devices already in the target state. Re-grabbing a
device this process already holds returns EBUSY, which was logged as a failure
and produced the misleading count=11 of=12 above.
Adds test seams for the grab ioctl and device discovery so the state machine is
covered without taking a real user's input away or depending on the test
machine's hardware.
The inbound allowlist matched the source address against the single address resolved from `host`. That is structurally unsatisfiable for a real peer: the configured peer is a machine, not an address, and a machine's other addresses cannot be derived from the one that was configured. A dual-stack Windows host opens the connection from an IPv6 link-local address that no lookup of its IPv4 will ever return, so every inbound attempt was refused and the link flapped between failed accepts and outbound retries. Measured on a normal session: 4 rejections, 5 resets and 5 reconnect cycles in 20 minutes. DHCP renewal, a second NIC, Wi-Fi/Ethernet failover and VPN routes have the same effect. Refuse globally routable sources before they can consume handshake work, so the internet still cannot reach the handshake, and let local-network sources (RFC1918/ULA private space, IPv6 link-local) attempt it. Loopback stays refused because the peer is another machine, and an empty allowlist still fails closed when resolution failed. The shared key remains the authentication control. This widens who may attempt authentication, never who passes it. Also drops grabTargetFiles, left unused by the applyIsolation refactor.
dpkg-query and systemctl are invoked to describe how mwb was installed. Neither had a deadline, so a wedged package database or a stalled systemd user bus could hang `mwb update` indefinitely with no output. Both now run under a 5s context, matching the repo rule that every exec.Command carries one.
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
xinput disableleft the machine with no mouse and no keyboard whenever the matchingenable never ran. Remote never hands the cursor back, mwb crashes, gets SIGKILLed, the
connection wedges: any of those and nothing in the system restores your input. The only
way out is replugging hardware, which is how I lost an Xorg session on 30 Jul.
Switched to EVIOCGRAB. The grab belongs to the file descriptor, so the kernel drops it
when the process dies by any means. Killing mwb now gives you your mouse back, which it
did not before. Devices are classified by capability bitmask instead of matching
razer|wootingin the name, so laptop touchpads and touchscreens are covered and powerbuttons are left alone.
xinputis no longer a dependency.EVIOCGRAB was tried before and reverted in d407e88. That version kept xinput as a
fallback, so the X device churn stayed, the compositor still stalled, and the cursor
warp on return took ~750ms with the grab held across it. With xinput fully gone the
warp measures 3-4ms and grab causes no device churn at all (0 vs 1
device removed).Warp still happens before release, deliberately. Releasing first is what caused the
rubberband in 22f85ce.
Four more things fell out of testing it:
Stop()could hang forever. Devices were opened blocking, so their fds never reachedGo's poller and
Close()did not interrupt a parkedread(2). A silent device'sgoroutine never returned. Opened
O_NONBLOCKnow.never grabbed and leaked motion to the local screen.
the next switch-out's grab, so local input was live while the cursor was on the
remote. Showed up in the log as
count=11 of=12followed byreleased count=12.Dual-stack peers connect from an IPv6 link-local address that no lookup of their IPv4
will return, so the link flapped: 4 rejections and 5 reconnects in 20 minutes. Now
refuses globally routable sources and lets local ones attempt the handshake. The
shared key still does the authenticating.
Also adds
mwb updateandmwb version. The release pipeline was passing-X main.versionat a variable that did not exist, so every release build wasunversioned.
Tested
make build,go test -race ./...andgolangci-lint runall clean. Switched back andforth on real hardware,
count=12 of=12every time in both directions, and the countsstayed right through a fast bounce that used to trip the race.
Not tested
reason to replug is gone and grab causes no X churn, but the Xorg fault itself is
untouched and unproven.
kill -9restoring input was proven on a throwaway uinput device, not against thereal devices.
outbound since, so nothing has exercised it outside the unit tests.
Worth a second look
isAllowedPeerwidens who may attempt the handshake to any local-network source. Thatis a trust boundary change, not a bug fix, and it deserves a real read rather than a
tick.
setxkbmap,localectl,xrandrandloginctlstill shell out without a contexttimeout. Out of scope here.