Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isatty is not required to set errno #467

Closed
jgallagher opened this issue Nov 30, 2022 · 2 comments · Fixed by #468
Closed

isatty is not required to set errno #467

jgallagher opened this issue Nov 30, 2022 · 2 comments · Fixed by #468

Comments

@jgallagher
Copy link

Hi! We noticed that the nightly version of cargo is panicking inside rustix on illumos:

$ RUST_BACKTRACE=1 cargo +nightly --version | cat
thread 'main' panicked at 'unexpected error from isatty: 0', /cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.36.3/src/backend/libc/termios/syscalls.rs:141:20
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e0098a5cc3a87d857e597af824d0ce1ed1ad85e0/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/e0098a5cc3a87d857e597af824d0ce1ed1ad85e0/library/core/src/panicking.rs:65:14
   2: rustix::backend::termios::syscalls::isatty
   3: <cargo::core::shell::Shell>::new
   4: <cargo::util::config::Config>::default
   5: <cargo::cli::LazyConfig>::get_mut
   6: cargo::cli::main
   7: cargo::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Digging in a bit, we found that the issue is that illumos currently chooses not to set errno in isatty(), but rustix expects errno to be set to ENOTTY if isatty() returns 0, and panics otherwise. I think illumos's behavior here is not ideal (and we're also filing a bug there), but is technically allowed by the wording on the POSIX manpage (emphasis mine):

The isatty() function shall return 1 if fildes is associated with a terminal; otherwise, it shall return 0 and may set errno to indicate the error.

Would you be opposed to changing the implementation of isatty() to only look at the return value and not errno?

sunfishcode added a commit that referenced this issue Nov 30, 2022
We don't actually do anything different for different errno values,
other than panic on unknown ones, and that isn't that isn't adding
much value compared to the cost of being an extra surprise when
porting to new OS's.

Fixes #467.
@sunfishcode
Copy link
Member

Makes sense to me. I've now submitted #468 to only look at the return value, and not errno.

@jgallagher
Copy link
Author

Thanks! ❤️

sunfishcode added a commit that referenced this issue Dec 1, 2022
We don't actually do anything different for different errno values,
other than panic on unknown ones, and that isn't that isn't adding
much value compared to the cost of being an extra surprise when
porting to new OS's.

Fixes #467.
sunfishcode added a commit that referenced this issue Dec 5, 2022
We don't actually do anything different for different errno values,
other than panic on unknown ones, and that isn't that isn't adding
much value compared to the cost of being an extra surprise when
porting to new OS's.

Fixes #467.
sunfishcode added a commit that referenced this issue Dec 5, 2022
We don't actually do anything different for different errno values,
other than panic on unknown ones, and that isn't that isn't adding
much value compared to the cost of being an extra surprise when
porting to new OS's.

Fixes #467.
luqmana added a commit to luqmana/rust that referenced this issue Dec 9, 2022
Pull in fix for bytecodealliance/rustix#467 on
recent cargo nightlies.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 10, 2022
…ulacrum

Update rustix to 0.36.5

Pull in fix for bytecodealliance/rustix#467 on recent cargo nightlies.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 10, 2022
…ulacrum

Update rustix to 0.36.5

Pull in fix for bytecodealliance/rustix#467 on recent cargo nightlies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants