From 6d30b9110a772b6168fe571802ea5cc5a4354692 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Wed, 9 Jul 2025 06:47:16 -0700 Subject: [PATCH] tr: Fix regression causing read error with sockets (#8083) * tr: Fix regression causing read error with sockets The test used for determining if a file descriptor tested with fstat points to a directory is traditionally done by using the S_IFMT mask[^1][^2], e.g.: #define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR) In Rust, this translates to 'm & libc::S_IFMT == libc::S_IFDIR'. is_stdin_directory() uses 'has!(mode, S_IFDIR)', which is **not** equivalent and causes non-directory sockets to be incorrectly recognized as directories. This causes uu-tr to break when used with all sockets created with socketpair, including ksh93 pipes[^3]. Below is an example Rust program demonstrating why the current check is bogus: fn main() { use nix::sys::socket::{socketpair, SockFlag, AddressFamily, SockType}; use nix::sys::stat::fstat; use libc::{S_IFDIR, S_IFMT, mode_t}; let (_fd1, fd2) = socketpair(AddressFamily::Unix, SockType::Stream, None, SockFlag::empty()).unwrap(); let mode = fstat(&fd2).unwrap().st_mode as mode_t; if mode & S_IFMT == S_IFDIR { // Equivalent to S_ISDIR() println!("Not reached"); // Not a dir, so unreachable } if mode & S_IFDIR == S_IFDIR { // Bogus check for S_IFDIR println!("BAD"); } } - is_stdin_directory(): Fix the regression introduced in 3e4221a4 by replacing the bogus check with one equivalent to S_ISDIR(). - test_tr.rs: Add a regression test for this bug. [^1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=io/sys/stat.h;h=4bea9e9a#l123 [^2]: https://git.musl-libc.org/cgit/musl/tree/include/sys/stat.h?id=047a1639#n51 [^3]: https://github.com/ksh93/ksh/blob/cc5e0692/src/cmd/ksh93/sh/io.c#L98-L102 Fixes https://github.com/uutils/coreutils/issues/7658 * Add comment explaining rationale Also fix cargo clippy lint. (In case it isn't obvious, I'm fairly new to Rust) * Fix more lint --- .../workspace.wordlist.txt | 2 ++ Cargo.lock | 10 +++++++++ Cargo.toml | 8 ++++++- src/uucore/src/lib/features/fs.rs | 4 +++- tests/by-util/test_tr.rs | 21 +++++++++++++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index d4a7b52fb..e42f24a94 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -154,6 +154,7 @@ IFSOCK IRGRP IROTH IRUSR +ISDIR ISGID ISUID ISVTX @@ -205,6 +206,7 @@ setgid setgroups settime setuid +socketpair socktype statfs statp diff --git a/Cargo.lock b/Cargo.lock index d3a59a95d..99cf61458 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1692,6 +1692,15 @@ dependencies = [ "libc", ] +[[package]] +name = "memoffset" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "488016bfae457b036d996092f6cb448677611ce4449e970ceaf42695203f218a" +dependencies = [ + "autocfg", +] + [[package]] name = "minimal-lexical" version = "0.2.1" @@ -1729,6 +1738,7 @@ dependencies = [ "cfg-if", "cfg_aliases", "libc", + "memoffset", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e75f4be59..d4ae1eb00 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -537,7 +537,13 @@ hex-literal = "1.0.0" rstest.workspace = true [target.'cfg(unix)'.dev-dependencies] -nix = { workspace = true, features = ["process", "signal", "user", "term"] } +nix = { workspace = true, features = [ + "process", + "signal", + "socket", + "user", + "term", +] } rlimit = "0.10.1" xattr.workspace = true diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 5c228376d..b98886540 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -720,7 +720,9 @@ pub fn is_stdin_directory(stdin: &Stdin) -> bool { { use nix::sys::stat::fstat; let mode = fstat(stdin.as_fd()).unwrap().st_mode as mode_t; - has!(mode, S_IFDIR) + // We use the S_IFMT mask ala S_ISDIR() to avoid mistaking + // sockets for directories. + mode & S_IFMT == S_IFDIR } #[cfg(windows)] diff --git a/tests/by-util/test_tr.rs b/tests/by-util/test_tr.rs index 0121f87aa..491fd64ee 100644 --- a/tests/by-util/test_tr.rs +++ b/tests/by-util/test_tr.rs @@ -1563,3 +1563,24 @@ fn test_broken_pipe_no_error() { .run_stdout_starts_with(b"") .fails_silently(); } + +#[cfg(not(windows))] +#[test] +fn test_stdin_is_socket() { + use nix::sys::socket::{AddressFamily, SockFlag, SockType, socketpair}; + use nix::unistd::write; + + let (fd1, fd2) = socketpair( + AddressFamily::Unix, + SockType::Stream, + None, + SockFlag::empty(), + ) + .unwrap(); + write(fd1, b"::").unwrap(); + new_ucmd!() + .args(&[":", ";"]) + .set_stdin(fd2) + .succeeds() + .stdout_is(";;"); +}