Skip to content

Conversation

@JohnoKing
Copy link
Contributor

This pull request fixes a regression introduced in 3e4221a that causes tr to fail when used with ksh93 sockets (previous bug report: #7658).
The test used for determining if a file descriptor tested with fstat points to a directory is traditionally done by using the S_IFMT mask12, 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 tr to break when used with all sockets created with socketpair, including ksh93 pipes3. 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");
     }
}

Changes:
- is_stdin_directory(): Fix the regression introduced in 3e4221a by replacing the bogus check with one equivalent to S_ISDIR().
- test_tr.rs: Add a regression test for this bug.

Fixes #7658

Footnotes

  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

@github-actions
Copy link

github-actions bot commented Jun 6, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Jul 4, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

JohnoKing added 3 commits July 9, 2025 14:59
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 3e4221a
  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 uutils#7658
Also fix cargo clippy lint.
(In case it isn't obvious, I'm fairly new to Rust)
@github-actions
Copy link

github-actions bot commented Jul 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@cakebaker cakebaker merged commit 6d30b91 into uutils:main Jul 9, 2025
116 of 117 checks passed
@cakebaker
Copy link
Contributor

Thanks for your PR!

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 this pull request may close these issues.

The tr utility fails with read error under ksh93

3 participants