From 211d9613c4c82223560c250e64526fce18f6fc8a Mon Sep 17 00:00:00 2001 From: binhvq Date: Wed, 11 Nov 2026 18:04:47 +0700 Subject: [PATCH 1/2] diff: look for NUL bytes in the first st_blksize bytes only --- src/diff.rs | 75 +++++++++++++++++++++++++++++--------- tests/by-util/test_diff.rs | 37 +++++++++++++++++++ 2 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 7de5f55c..9f931d81 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -87,51 +87,56 @@ pub fn compare_files(params: &Params, header: Option<&[u8]>) -> Outcome { return Outcome::Identical; } - // read files - fn read_file_contents(filepath: &OsString) -> io::Result> { + // read files, along with how many leading bytes decide if they are binary + fn read_file_contents(filepath: &OsString) -> io::Result<(Vec, usize)> { + let mut content = Vec::new(); if filepath == "-" { - let mut content = Vec::new(); - io::stdin().read_to_end(&mut content).and(Ok(content)) + io::stdin().read_to_end(&mut content)?; + Ok((content, stdin_binary_probe_len())) } else { - fs::read(filepath) + let mut file = fs::File::open(filepath)?; + file.read_to_end(&mut content)?; + Ok((content, binary_probe_len(&file))) } } let mut io_error = false; let mut from_missing = false; let mut to_missing = false; - let from_content = match read_file_contents(¶ms.from) { + let (from_content, from_probe_len) = match read_file_contents(¶ms.from) { Ok(from_content) => from_content, Err(e) if e.kind() == io::ErrorKind::NotFound && params.missing_file != MissingFile::Error => { from_missing = true; - vec![] + (vec![], 0) } Err(e) => { report_failure_to_read_input_file(¶ms.executable, ¶ms.from, &e); io_error = true; - vec![] + (vec![], 0) } }; - let to_content = match read_file_contents(¶ms.to) { + let (to_content, to_probe_len) = match read_file_contents(¶ms.to) { Ok(to_content) => to_content, Err(e) if e.kind() == io::ErrorKind::NotFound && params.missing_file == MissingFile::Empty => { to_missing = true; - vec![] + (vec![], 0) } Err(e) => { report_failure_to_read_input_file(¶ms.executable, ¶ms.to, &e); io_error = true; - vec![] + (vec![], 0) } }; if io_error { return Outcome::Trouble; } - if !params.text && (is_binary(&from_content) || is_binary(&to_content)) { + if !params.text + && (is_binary(&from_content, from_probe_len) || is_binary(&to_content, to_probe_len)) + { if from_content == to_content { maybe_report_identical_files(); return Outcome::Identical; @@ -248,12 +253,48 @@ fn output_palette(params: &Params) -> Option { } } -/// Number of leading bytes inspected when deciding whether an input is binary. -const BINARY_PROBE_LEN: usize = 16 * 1024; +/// Number of leading bytes inspected when deciding whether an input is binary, +/// where the file system does not tell its preferred I/O size. +const DEFAULT_BINARY_PROBE_LEN: usize = 16 * 1024; + +/// An input is treated as binary when a NUL byte occurs in its first +/// `probe_len` bytes. +fn is_binary(content: &[u8], probe_len: usize) -> bool { + content[..content.len().min(probe_len)].contains(&0) +} + +/// Like GNU diff, only the first buffer read from a file decides whether it +/// is binary, and that buffer is as large as the preferred I/O size of the +/// file (`st_blksize`). +#[cfg(unix)] +fn binary_probe_len(file: &fs::File) -> usize { + use std::os::unix::fs::MetadataExt; + file.metadata() + .ok() + .and_then(|metadata| usize::try_from(metadata.blksize()).ok()) + .filter(|&len| len > 0) + .unwrap_or(DEFAULT_BINARY_PROBE_LEN) +} + +#[cfg(not(unix))] +fn binary_probe_len(_file: &fs::File) -> usize { + DEFAULT_BINARY_PROBE_LEN +} + +#[cfg(unix)] +fn stdin_binary_probe_len() -> usize { + use std::os::fd::AsFd; + io::stdin() + .as_fd() + .try_clone_to_owned() + .map_or(DEFAULT_BINARY_PROBE_LEN, |fd| { + binary_probe_len(&fs::File::from(fd)) + }) +} -/// An input is treated as binary when a NUL byte occurs near its start. -fn is_binary(content: &[u8]) -> bool { - content[..content.len().min(BINARY_PROBE_LEN)].contains(&0) +#[cfg(not(unix))] +fn stdin_binary_probe_len() -> usize { + DEFAULT_BINARY_PROBE_LEN } /// Print the one-line summary used instead of the differences, `kind` naming diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index 10fc0a10..575c9733 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -364,6 +364,43 @@ fn missing_file_as_empty_keeps_stdin_timestamp() -> Result<(), Box Result<(), Box> { + use std::os::unix::fs::MetadataExt; + + let tmp_dir = tempdir()?; + let late_nul = tmp_dir.path().join("late_nul"); + let early_nul = tmp_dir.path().join("early_nul"); + let text = tmp_dir.path().join("text"); + let mut data = b"line\n".repeat(2000); + std::fs::write(&text, &data)?; + let block_size = usize::try_from(std::fs::metadata(&text)?.blksize())?; + if !(1..data.len()).contains(&block_size) { + return Ok(()); + } + data[block_size] = 0; + std::fs::write(&late_nul, &data)?; + data[block_size - 1] = 0; + std::fs::write(&early_nul, &data)?; + + for (from, to) in [(&late_nul, &text), (&text, &late_nul)] { + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff").arg(from).arg(to); + cmd.assert() + .code(predicate::eq(1)) + .stdout(predicate::str::is_match(r"^\d+c\d+\n")?); + } + let mut cmd = cargo_bin_cmd!("diffutils"); + cmd.arg("diff").arg(&early_nul).arg(&text); + cmd.assert() + .code(predicate::eq(1)) + .stdout(predicate::str::starts_with("Binary files")); + Ok(()) +} + #[test] fn binary_files() -> Result<(), Box> { let tmp_dir = tempdir()?; From 8923fc74e146d9aa131965037bce2f160d11a8cf Mon Sep 17 00:00:00 2001 From: binhvq Date: Thu, 12 Nov 2026 03:33:41 +0700 Subject: [PATCH 2/2] tests: port the one-sided recursive loop test to uutests --- tests/by-util/test_diff.rs | 55 +++++++++++++++----------------------- 1 file changed, 21 insertions(+), 34 deletions(-) diff --git a/tests/by-util/test_diff.rs b/tests/by-util/test_diff.rs index ca3d74f1..c6d41404 100644 --- a/tests/by-util/test_diff.rs +++ b/tests/by-util/test_diff.rs @@ -540,51 +540,38 @@ fn compare_directories_special_entries() { // has nothing there: a real directory on the other side ends the recursion. #[cfg(unix)] #[test] -fn recursive_loop_on_one_side_only() -> Result<(), Box> { - use std::os::unix::fs::symlink; - - let tmp_dir = tempdir()?; - let dir = tmp_dir.path(); - std::fs::create_dir_all(dir.join("one/sub"))?; - std::fs::create_dir_all(dir.join("two/sub/loop"))?; - symlink("..", dir.join("one/sub/loop"))?; +fn recursive_loop_on_one_side_only() { + let (at, ts) = at_and_ts!(); + at.mkdir_all("one/sub"); + at.mkdir_all("two/sub/loop"); + at.relative_symlink_dir("..", "one/sub/loop"); for (from, to) in [("one", "two"), ("two", "one")] { - let mut cmd = cargo_bin_cmd!("diffutils"); - cmd.current_dir(dir).arg("diff").arg("-r").arg(from).arg(to); - cmd.assert() - .code(predicate::eq(1)) - .stdout("Only in one/sub/loop: sub\n") - .stderr(""); + ts.ucmd() + .args(&["-r", from, to]) + .fails_with_code(1) + .stdout_only("Only in one/sub/loop: sub\n"); } // With -N a missing directory is compared as an empty one, which would // never end the recursion. - std::fs::remove_dir(dir.join("two/sub/loop"))?; + std::fs::remove_dir(at.plus("two/sub/loop")).unwrap(); for (from, to) in [("one", "two"), ("two", "one")] { - let mut cmd = cargo_bin_cmd!("diffutils"); - cmd.current_dir(dir).arg("diff").arg("-r").arg("-N"); - cmd.arg(from).arg(to); - cmd.assert() - .code(predicate::eq(2)) - .stderr("diff: one/sub/loop: recursive directory loop\n"); + ts.ucmd() + .args(&["-r", "-N", from, to]) + .fails_with_code(2) + .stderr_is("diff: one/sub/loop: recursive directory loop\n"); } // Each side is checked against its own ancestors: here the links lead to // the top of the other side, and only loop one level further down. - std::fs::remove_file(dir.join("one/sub/loop"))?; - symlink("../../two", dir.join("one/sub/loop"))?; - symlink("../../one", dir.join("two/sub/loop"))?; - let mut cmd = cargo_bin_cmd!("diffutils"); - cmd.current_dir(dir) - .arg("diff") - .arg("-r") - .arg("one") - .arg("two"); - cmd.assert() - .code(predicate::eq(2)) - .stderr("diff: one/sub/loop/sub/loop: recursive directory loop\n"); - Ok(()) + std::fs::remove_file(at.plus("one/sub/loop")).unwrap(); + at.relative_symlink_dir("../../two", "one/sub/loop"); + at.relative_symlink_dir("../../one", "two/sub/loop"); + ts.ucmd() + .args(&["-r", "one", "two"]) + .fails_with_code(2) + .stderr_is("diff: one/sub/loop/sub/loop: recursive directory loop\n"); } #[test]