Fix tui resize on linux#12
Merged
Merged
Conversation
Contributor
|
Thanks for the fix |
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.
Summary
On Linux, an inner TUI launched through the built-in sandbox (e.g. opencode under
omac start) renders once at the correct size but never reflows when the surroundingterminal is resized. This PR removes
--new-sessionfrom thebwrapinvocation, whichrestores
SIGWINCHresize delivery to the inner process.The underlying problem is already partially solved in bubblewrap but was never merged: containers/bubblewrap#586
Problem
A terminal program learns its window size two ways:
TIOCGWINSZioctl on its tty fd. This works regardless ofcontrolling-terminal status — which is why the initial render is correctly sized.
SIGWINCH. The kernel only deliversSIGWINCHtothe foreground process group of the controlling terminal.
We invoked
bwrapwith--new-session, which callssetsid(2). That places the entireinner process tree (
bwrap → omac sandbox stage2 → opencode) in a new session with nocontrolling terminal. With no controlling terminal, the kernel never delivers
SIGWINCHon resize, so the TUI never learns it should reflow. omac forwards only
SIGINT/SIGTERM/SIGHUP/SIGQUITto the child group — notSIGWINCH— so nothing bridgedthe gap either.
Fix
Remove
--new-sessionfromBuildBwrapArgv. The inner tree keeps the real controllingterminal, the kernel delivers
SIGWINCHon resize, and the TUI reflows correctly.What
--new-sessionwas forIts sole purpose is to block
TIOCSTIterminal-input injection: a confined processsharing the real terminal fd can call
ioctl(fd, TIOCSTI, …)to push characters into theterminal's input queue, which the outer shell then executes after omac exits — a
deferred sandbox escape into the host shell. This is the only capability we give up, and on
modern Linux it is already closed at the kernel level (see risk assessment).
Risk assessment
Scope: narrow.
--new-sessionaffects only session/controlling-terminal semantics. Ithas no bearing on the mechanisms that confine the sandbox from the outside world, all
of which are unchanged:
--ro-bind/--bindmounts + Landlock path rules--unshare-pid/--unshare-ipc/--unshare-uts--die-with-parent--new-sessionResidual risk — TIOCSTI injection:
TIOCSTIis gated behind thedev.tty.legacy_tiocstisysctl, which defaults to
0(disabled). With it disabled,ioctl(TIOCSTI)returnsEIOregardless of--new-session. On these systems dropping the flag loses nothing— the kernel already closes the vector. (Verified on the dev machine: kernel 7.0.11,
dev.tty.legacy_tiocsti = 0.)dev.tty.legacy_tiocsti = 1: the vector reopens. Aconfined process could queue input into the user's shell that executes unconfined after
omac exits. Requires an interactive shell that reads from the shared terminal after the
session ends.
Severity: Low for the project's threat model (confining the inner process from reaching
host filesystem/network/processes), which is fully preserved. The regression applies only
to the terminal-injection vector, only on pre-6.2 kernels or non-default sysctl configs,
and only against an interactive parent shell.
Alternatives considered:
--new-sessiononly whendev.tty.legacy_tiocsti == 0, keep it otherwise. Portable and safe on old kernels, atthe cost of no resize in that rare case.
--new-sessionand give the innerprocess a PTY as its controlling terminal, forwarding window-size changes to the PTY
master. Restores resize and closes TIOCSTI more completely (injection can only reach
omac's own PTY master, never the real shell). Heavier (a PTY-forwarding loop); deferred
as a follow-up.
This PR takes the direct removal: simplest change, full resize support, and zero isolation
loss on modern default-configured Linux.
Testing
bwrap_test.gothat fails if--new-sessioneverreappears in the argv.
go test ./internal/sandboxrun/passes;gofmtclean.omac start.Note
The historical spec docs under
openspec/changes/native-sandbox/still list--new-sessionas required. Left untouched as a record of a completed change — if theproject treats them as living spec, I can update those three files to match.