stty: do not write settings back for query-only arguments - #13950
Open
m0g3r wants to merge 3 commits into
Open
Conversation
`stty size` is a query, but any invocation carrying settings arguments ended with an unconditional `tcsetattr`. POSIX requires the kernel to raise SIGTTOU on `tcsetattr` from a background process group, and its default disposition stops the process, so `stty size` run off the foreground hangs with no diagnostic and never reaps. Only call `tcsetattr` when at least one argument actually changes the terminal settings. `Print` arguments do not. Fixes uutils#13722 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Binary size comparison: |
Contributor
|
Please also add a test in test_stty.rs |
Add a regression test for the SIGTTOU stop this change fixes. The test allocates a PTY, gives a helper its own session and controlling terminal, then runs `stty size` from a background process group with SIGTTOU restored to its default disposition and asserts it exits normally rather than being stopped. Verified against the fix: it passes with the patch, and reverting src/uu/stty/src/stty.rs to its pre-fix state makes it fail with "`stty size` was stopped by SIGTTOU in a background process group" (signal 22). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing 
Footnotes
|
`libc::ioctl`'s request parameter is `libc::Ioctl`, which is `c_ulong` on
glibc and Darwin but `c_int` on musl and Android. Casting TIOCSCTTY to
`c_ulong` unconditionally broke the test build on those targets:
error[E0308]: mismatched types
--> tests/by-util/test_stty.rs:110:35
| libc::ioctl(0, libc::TIOCSCTTY as libc::c_ulong, 0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `u64`
Let inference pick the request type from the signature instead.
Checked with `cargo check` against x86_64-unknown-linux-musl,
x86_64-unknown-linux-gnu and aarch64-apple-darwin: the old cast fails on
musl with exactly the error above, the new one compiles on all three.
`cargo test --test tests --features feat_os_unix test_stty` passes 52
tests, including test_size_from_background_process_group.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Fixes #13722.
stty sizeonly reads the terminal, but every invocation carrying settings arguments finished with an unconditionaltcsetattr. From a background process group POSIX requires the kernel to raiseSIGTTOU, whose default disposition stops the process, sostty sizeoff the foreground hangs with no diagnostic and never reaps.tcsetattris now called only when at least one parsed argument actually changes the terminal settings.ArgOptions::Printdoes not, sostty sizetakes the same path it already took for printing and then stops.Test
A unit test covers the predicate directly, since reproducing
SIGTTOUneeds a controlling terminal and a background process group, which the test harness does not provide.Checked by hand against a pty that
sizestill reports dimensions, thatrows/columnsstill apply, and that-ais unaffected:cargo test -p uu_stty27 passed, thesttyintegration suite 53 passed / 33 ignored,cargo clippy -p uu_stty --all-targets -- -D warningsclean.Note this touches the same line as #13893. If that lands first the verification it adds should move inside this guard, since there is nothing to verify when the settings were never written.
Written with AI assistance. I have reviewed the diff and verified the behaviour against a pty myself.