From 4185df5822db6c2a2f32c5216095236a1041f4af Mon Sep 17 00:00:00 2001 From: binhvq Date: Wed, 11 Nov 2026 16:33:58 +0700 Subject: [PATCH 1/2] diff, cmp: check for the same file without opening the operands The same-file check opened both operands. With FIFOs, the writers went ahead on that open and their data was dropped with the handle, so the read that followed waited forever or saw an empty file. Compare the device and inode numbers from stat() on Unix instead. --- src/cmp.rs | 6 ++---- src/diff.rs | 6 ++---- src/utils.rs | 20 ++++++++++++++++++++ tests/by-util/test_cmp.rs | 26 ++++++++++++++++++++++++++ tests/by-util/test_diff.rs | 28 ++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 8 deletions(-) diff --git a/src/cmp.rs b/src/cmp.rs index cdd0b5b9..7c413759 100644 --- a/src/cmp.rs +++ b/src/cmp.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE-* // files that was distributed with this source code. -use crate::utils::{format_failure_to_read_input_file, is_posix_locale}; +use crate::utils::{format_failure_to_read_input_file, is_posix_locale, is_same_file}; use std::env::ArgsOs; use std::ffi::OsString; use std::io::{BufRead, BufReader, BufWriter, Read, Write}; @@ -451,9 +451,7 @@ pub fn main(opts: Peekable) -> ExitCode { } }; - if params.from == "-" && params.to == "-" - || same_file::is_same_file(¶ms.from, ¶ms.to).unwrap_or(false) - { + if params.from == "-" && params.to == "-" || is_same_file(¶ms.from, ¶ms.to) { return ExitCode::SUCCESS; } diff --git a/src/diff.rs b/src/diff.rs index 7de5f55c..e9861cb4 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -5,7 +5,7 @@ use crate::color::{colorize, ColorWhen, Palette, PaletteError}; use crate::params::{parse_params, Format, MissingFile, Params}; -use crate::utils::report_failure_to_read_input_file; +use crate::utils::{is_same_file, report_failure_to_read_input_file}; use crate::{context_diff, dir_diff, ed_diff, normal_diff, side_diff, unified_diff}; use std::env::ArgsOs; use std::ffi::OsString; @@ -80,9 +80,7 @@ pub fn compare_files(params: &Params, header: Option<&[u8]>) -> Outcome { } }; // if from and to are the same file, no need to perform any comparison - if params.from == "-" && params.to == "-" - || same_file::is_same_file(¶ms.from, ¶ms.to).unwrap_or(false) - { + if params.from == "-" && params.to == "-" || is_same_file(¶ms.from, ¶ms.to) { maybe_report_identical_files(); return Outcome::Identical; } diff --git a/src/utils.rs b/src/utils.rs index 29ab93df..d834e891 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -163,6 +163,26 @@ pub fn quote_shell_argument(arg: &[u8]) -> Cow<'_, [u8]> { Cow::Owned(quoted) } +/// Returns whether both paths lead to the same file. +/// +/// On Unix this compares what stat() reports and never opens the files: +/// opening a FIFO just to identify it would take its writer away from the +/// read that follows. +pub fn is_same_file(from: &OsStr, to: &OsStr) -> bool { + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + match (std::fs::metadata(from), std::fs::metadata(to)) { + (Ok(from), Ok(to)) => from.dev() == to.dev() && from.ino() == to.ino(), + _ => false, + } + } + #[cfg(not(unix))] + { + same_file::is_same_file(from, to).unwrap_or(false) + } +} + /// Returns `name` in lowercase, to match file names ignoring case. pub fn fold_file_name_case(name: &OsStr) -> Vec { match name.to_str() { diff --git a/tests/by-util/test_cmp.rs b/tests/by-util/test_cmp.rs index 020a941f..3bda1a74 100644 --- a/tests/by-util/test_cmp.rs +++ b/tests/by-util/test_cmp.rs @@ -72,6 +72,32 @@ fn cmp_incompatible_params() -> Result<(), Box> { Ok(()) } +// Checking whether the operands are the same file must not open them: a FIFO +// can only be read once. +#[cfg(unix)] +#[test] +fn cmp_fifo_operands_are_read_once() -> Result<(), Box> { + let tmp_dir = tempdir()?; + let mut writers = Vec::new(); + let mut fifos = Vec::new(); + for (name, content) in [("one", "a\nb\n"), ("two", "a\nc\n")] { + let fifo = tmp_dir.path().join(name); + let status = std::process::Command::new("mkfifo").arg(&fifo).status()?; + assert!(status.success()); + fifos.push(fifo.clone()); + writers.push(std::thread::spawn(move || std::fs::write(fifo, content))); + } + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("cmp").arg("-s").args(&fifos); + cmd.timeout(std::time::Duration::from_secs(10)); + cmd.assert().code(predicate::eq(1)); + for writer in writers { + writer.join().unwrap()?; + } + Ok(()) +} + #[test] fn cmp_stdin() -> Result<(), Box> { let tmp_dir = tempdir()?; diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index 10fc0a10..3de227a6 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -748,6 +748,34 @@ fn compare_directories_special_entries() -> Result<(), Box Result<(), Box> { + let tmp_dir = tempdir()?; + let mut writers = Vec::new(); + let mut fifos = Vec::new(); + for (name, content) in [("one", "a\nb\n"), ("two", "a\nc\n")] { + let fifo = tmp_dir.path().join(name); + let status = std::process::Command::new("mkfifo").arg(&fifo).status()?; + assert!(status.success()); + fifos.push(fifo.clone()); + writers.push(std::thread::spawn(move || std::fs::write(fifo, content))); + } + + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff").args(&fifos); + cmd.timeout(std::time::Duration::from_secs(10)); + cmd.assert() + .code(predicate::eq(1)) + .stdout("2c2\n< b\n---\n> c\n"); + for writer in writers { + writer.join().unwrap()?; + } + Ok(()) +} + #[test] fn directory_name_matching() -> Result<(), Box> { let tmp_dir = tempdir()?; From f85d0349a3dddc80440d25c06f299005728171f4 Mon Sep 17 00:00:00 2001 From: binhvq Date: Thu, 12 Nov 2026 04:05:19 +0700 Subject: [PATCH 2/2] tests: port the FIFO operand tests to uutests --- tests/by-util/test_cmp.rs | 24 +++++++++++------------- tests/by-util/test_diff.rs | 26 +++++++++++--------------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/tests/by-util/test_cmp.rs b/tests/by-util/test_cmp.rs index 670173f7..8c79f1ca 100644 --- a/tests/by-util/test_cmp.rs +++ b/tests/by-util/test_cmp.rs @@ -65,26 +65,24 @@ fn cmp_stdin(#[case] input: &str, #[case] code: i32, #[case] expected: &str) { // can only be read once. #[cfg(unix)] #[test] -fn cmp_fifo_operands_are_read_once() -> Result<(), Box> { - let tmp_dir = tempdir()?; +fn cmp_fifo_operands_are_read_once() { + let (at, ts) = at_and_ts!(); let mut writers = Vec::new(); - let mut fifos = Vec::new(); for (name, content) in [("one", "a\nb\n"), ("two", "a\nc\n")] { - let fifo = tmp_dir.path().join(name); - let status = std::process::Command::new("mkfifo").arg(&fifo).status()?; - assert!(status.success()); - fifos.push(fifo.clone()); + at.mkfifo(name); + let fifo = at.plus(name); writers.push(std::thread::spawn(move || std::fs::write(fifo, content))); } - let mut cmd = cargo_bin_cmd!("diffutils"); - cmd.arg("cmp").arg("-s").args(&fifos); - cmd.timeout(std::time::Duration::from_secs(10)); - cmd.assert().code(predicate::eq(1)); + ts.ucmd() + .args(&["-s", "one", "two"]) + .timeout(std::time::Duration::from_secs(10)) + .run() + .code_is(1) + .no_output(); for writer in writers { - writer.join().unwrap()?; + writer.join().unwrap().unwrap(); } - Ok(()) } #[test] diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index 83f7a7ed..a3ba48dc 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -578,28 +578,24 @@ fn recursive_loop_on_one_side_only() { // file must not open them, or the writer's data is gone before the read. #[cfg(unix)] #[test] -fn fifo_operands_are_read_once() -> Result<(), Box> { - let tmp_dir = tempdir()?; +fn fifo_operands_are_read_once() { + let (at, ts) = at_and_ts!(); let mut writers = Vec::new(); - let mut fifos = Vec::new(); for (name, content) in [("one", "a\nb\n"), ("two", "a\nc\n")] { - let fifo = tmp_dir.path().join(name); - let status = std::process::Command::new("mkfifo").arg(&fifo).status()?; - assert!(status.success()); - fifos.push(fifo.clone()); + at.mkfifo(name); + let fifo = at.plus(name); writers.push(std::thread::spawn(move || std::fs::write(fifo, content))); } - let mut cmd = cargo_bin_cmd!("diffutils"); - cmd.arg("diff").args(&fifos); - cmd.timeout(std::time::Duration::from_secs(10)); - cmd.assert() - .code(predicate::eq(1)) - .stdout("2c2\n< b\n---\n> c\n"); + ts.ucmd() + .args(&["one", "two"]) + .timeout(std::time::Duration::from_secs(10)) + .run() + .code_is(1) + .stdout_only("2c2\n< b\n---\n> c\n"); for writer in writers { - writer.join().unwrap()?; + writer.join().unwrap().unwrap(); } - Ok(()) } #[test]