Skip to content

Fix isatty in WASI. #3696

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.

Sign up for GitHub

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

Merged
sunfishcode merged 3 commits into from
Jan 24, 2022
Merged

Fix isatty in WASI. #3696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c64ef3
Fix `isatty` in WASI.
sunfishcode Jan 18, 2022
b4e3327
Use `unix` instead of `not(windows)` for the `isatty` calls.
sunfishcode Jan 19, 2022
a0aaf9f
Use `not(unix)` instead of `windows` for `atty` usage.
sunfishcode Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/wasi-common/cap-std-sync/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ rustix = "0.31.0"
[target.'cfg(windows)'.dependencies]
winapi = "0.3"
lazy_static = "1.4"
atty = "0.2.14"

[dev-dependencies]
tempfile = "3.1.0"
10 changes: 10 additions & 0 deletions crates/wasi-common/cap-std-sync/src/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ impl WasiFile for File {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(self.0.num_ready_bytes()?)
}
fn isatty(&self) -> bool {
#[cfg(unix)]
{
rustix::io::isatty(&self.0)
}
#[cfg(windows)]
{
false
}
Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be cfg(unix) and cfg(not(unix)) instead?

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the first to cfg(unix), but I think cfg(windows) makes sense for the second. If we port to a non-Unix non-Windows platform in the future, we should figure out what isatty means for that platform.

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The atty crate would figure that out, right?

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in its current form, because the atty crate currently has no interface for passing in a handle. It just supports literal stdin, stdout, and stderr. Possibly that will change in the future, and then we could simplify code like this.

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case yes, but in stdio.rs you are using atty on stdin only for windows. There the #[cfg(windows)] can be turned into #[cfg(not(unix))] without issues.

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. I've now made this change.

}
async fn readable(&self) -> Result<(), Error> {
Err(Error::badf())
}
Expand Down
38 changes: 33 additions & 5 deletions crates/wasi-common/cap-std-sync/src/stdio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ impl WasiFile for Stdin {
Ok(())
}
async fn get_filetype(&self) -> Result<FileType, Error> {
Ok(FileType::Unknown)
if self.isatty() {
Ok(FileType::CharacterDevice)
} else {
Ok(FileType::Unknown)
}
}
async fn get_fdflags(&self) -> Result<FdFlags, Error> {
Ok(FdFlags::empty())
Expand Down Expand Up @@ -104,6 +108,16 @@ impl WasiFile for Stdin {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(self.0.num_ready_bytes()?)
}
fn isatty(&self) -> bool {
#[cfg(unix)]
{
rustix::io::isatty(&self.0)
}
#[cfg(not(unix))]
{
atty::is(atty::Stream::Stdin)
}
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could atty be used on all platforms? (on Unix does atty differ from rustix?)

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux rustix can use inline asm, while atty always uses libc.

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason for using rustix here was that cap-std-sync already depends on rustix for other things, so this avoids adding a new dependency on atty on unix-family platforms.

Rustix's isatty uses the same ioctl as musl, which is a different ioctl from what glibc uses, but the difference shouldn't matter in practice.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok, personally I'm not a fan of having a lot of "if linux else everyone else this" because it seems like it'll just cause tons of Linux-specific or not-Linux-specific issues. This is pretty minor though.

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual change uses cfg(unix), so it isn't Linux-specific.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure but it's the same thing with Windows. Crates like atty which provide a platform-agnostic interface at least theoretically deal with the portability within them (and afaik atty is widely used and well vetted). If we specifically don't use that then we're subject to all the bugs that atty would get except we'll get them instead (theoretically) at some later point.

But again this is personal preference and this is minor. I don't like to pervasively use platform-specific crates, instead using platform-specific crates in very specific situations and otherwise favoring platform-agnostic crates for heavy usage.

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now factored this to use a platform-agnostic crate, so that Wasmtime's own code avoids the cfgs here.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify I'm fine either way, mostly just stating my own personal preference. But also, this looks the same as before, so unsure if this is intended to stay as-is or whether a commit was forgotten? (this PR is fine to merge whenever imo, I don't feel any need to debate fine detatils like this)

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one now has #[cfg(unix)] and #[cfg(not(unix))]. The other is left as #[cfg(unix)] and #[cfg(windows)] without fallback for non-unix, non-windows platforms.

Copy link
Copy Markdown
Member Author

@sunfishcode sunfishcode Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's consistent with impl AsFd for File under cfg(unix) which is already there. Ideally we should support non-windows-non-unix, but that's a bigger change.

And, I'm working on a change which will help; I plan to replace all these isatty implementations with a new is-terminal crate which supports windows, unix, and (to the extent possible) non-windows-non-unix with a single API. I'll do it in a separate PR because I need to make a few other changes to avoid cargo deny flagging it for duplicate dependency versions.

}
async fn readable(&self) -> Result<(), Error> {
Err(Error::badf())
}
Expand All @@ -125,7 +139,7 @@ impl AsFd for Stdin {
}

macro_rules! wasi_file_write_impl {
($ty:ty) => {
($ty:ty, $ident:ident) => {
#[async_trait::async_trait]
impl WasiFile for $ty {
fn as_any(&self) -> &dyn Any {
Expand All @@ -138,7 +152,11 @@ macro_rules! wasi_file_write_impl {
Ok(())
}
async fn get_filetype(&self) -> Result<FileType, Error> {
Ok(FileType::Unknown)
if self.isatty() {
Ok(FileType::CharacterDevice)
} else {
Ok(FileType::Unknown)
}
}
async fn get_fdflags(&self) -> Result<FdFlags, Error> {
Ok(FdFlags::APPEND)
Expand Down Expand Up @@ -210,6 +228,16 @@ macro_rules! wasi_file_write_impl {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(0)
}
fn isatty(&self) -> bool {
#[cfg(unix)]
{
rustix::io::isatty(&self.0)
}
#[cfg(not(unix))]
{
atty::is(atty::Stream::$ident)
}
}
async fn readable(&self) -> Result<(), Error> {
Err(Error::badf())
}
Expand Down Expand Up @@ -237,11 +265,11 @@ pub struct Stdout(std::io::Stdout);
pub fn stdout() -> Stdout {
Stdout(std::io::stdout())
}
wasi_file_write_impl!(Stdout);
wasi_file_write_impl!(Stdout, Stdout);

pub struct Stderr(std::io::Stderr);

pub fn stderr() -> Stderr {
Stderr(std::io::stderr())
}
wasi_file_write_impl!(Stderr);
wasi_file_write_impl!(Stderr, Stderr);
22 changes: 19 additions & 3 deletions crates/wasi-common/src/ctx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,31 @@ impl WasiCtx {
}

pub fn set_stdin(&mut self, f: Box<dyn WasiFile>) {
self.insert_file(0, f, FileCaps::all());
let rights = Self::stdio_rights(&*f);
self.insert_file(0, f, rights);
}

pub fn set_stdout(&mut self, f: Box<dyn WasiFile>) {
self.insert_file(1, f, FileCaps::all());
let rights = Self::stdio_rights(&*f);
self.insert_file(1, f, rights);
}

pub fn set_stderr(&mut self, f: Box<dyn WasiFile>) {
self.insert_file(2, f, FileCaps::all());
let rights = Self::stdio_rights(&*f);
self.insert_file(2, f, rights);
}

fn stdio_rights(f: &dyn WasiFile) -> FileCaps {
let mut rights = FileCaps::all();

// If `f` is a tty, restrict the `tell` and `seek` capabilities, so
// that wasi-libc's `isatty` correctly detects the file descriptor
// as a tty.
if f.isatty() {
rights &= !(FileCaps::TELL | FileCaps::SEEK);
}

rights
}

pub fn push_preopened_dir(
Expand Down
1 change: 1 addition & 0 deletions crates/wasi-common/src/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub trait WasiFile: Send + Sync {
async fn seek(&self, pos: std::io::SeekFrom) -> Result<u64, Error>; // file op that generates a new stream from a file will supercede this
async fn peek(&self, buf: &mut [u8]) -> Result<u64, Error>; // read op
async fn num_ready_bytes(&self) -> Result<u64, Error>; // read op
fn isatty(&self) -> bool;

async fn readable(&self) -> Result<(), Error>;
async fn writable(&self) -> Result<(), Error>;
Expand Down
6 changes: 6 additions & 0 deletions crates/wasi-common/src/pipe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ impl<R: Read + Any + Send + Sync> WasiFile for ReadPipe<R> {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(0)
}
fn isatty(&self) -> bool {
false
}
async fn readable(&self) -> Result<(), Error> {
Err(Error::badf())
}
Expand Down Expand Up @@ -336,6 +339,9 @@ impl<W: Write + Any + Send + Sync> WasiFile for WritePipe<W> {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(0)
}
fn isatty(&self) -> bool {
false
}
async fn readable(&self) -> Result<(), Error> {
Err(Error::badf())
}
Expand Down
3 changes: 3 additions & 0 deletions crates/wasi-common/tokio/src/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ macro_rules! wasi_file_impl {
async fn num_ready_bytes(&self) -> Result<u64, Error> {
block_on_dummy_executor(|| self.0.num_ready_bytes())
}
fn isatty(&self) -> bool {
self.0.isatty()
}

#[cfg(not(windows))]
async fn readable(&self) -> Result<(), Error> {
Expand Down