fix: eliminate TOCTOU races in ln and tac by deferring is_dir() checks (#10991)

* fix: improve error handling for file operations in tac and ln commands

* fix: eliminate TOCTOU races in ln and tac by deferring is_dir() checks

ln: call hard_link() first, check is_dir() only on failure for diagnostics
tac: remove is_dir()/metadata() pre-checks, open file directly and let OS report errors (EISDIR, ENOENT)

Closes #9450

* fix(tac): update tests for cross-platform error messages and strip_errno
- Remove `(os error 28)` from test_failed_write_is_reported expectation
  to match the intentional strip_errno() behavior

* fix(tac): improve error message formatting for file not found scenario

Author: sylvestre

src/uu/ln/src/ln.rs

@@ -430,24 +430,20 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> {
     if settings.symbolic {
         symlink(&source, dst)?;
     } else {
-        // Cannot create hard link to a directory directly
-        // We can however create hard link to a symlink that points to a directory, so long as -L is not passed
-        if src.is_dir() && (!src.is_symlink() || settings.logical) {
-            return Err(LnError::FailedToCreateHardLinkDir(source.to_path_buf()).into());
-        }
-
         let p = if settings.logical && source.is_symlink() {
-            // if we want to have an hard link,
-            // source is a symlink and -L is passed
-            // we want to resolve the symlink to create the hardlink
             fs::canonicalize(&source)
                 .map_err_context(|| translate!("ln-failed-to-access", "file" => source.quote()))?
         } else {
             source.to_path_buf()
         };
-        fs::hard_link(p, dst).map_err_context(|| {
-            translate!("ln-failed-to-create-hard-link", "source" => source.quote(), "dest" => dst.quote())
-        })?;
+        if let Err(e) = fs::hard_link(&p, dst) {
+            if p.is_dir() {
+                return Err(LnError::FailedToCreateHardLinkDir(source.to_path_buf()).into());
+            }
+            return Err(e).map_err_context(|| {
+                translate!("ln-failed-to-create-hard-link", "source" => source.quote(), "dest" => dst.quote())
+            });
+        }
     }
 
     if settings.verbose {

src/uu/tac/locales/en-US.ftl

@@ -6,7 +6,6 @@ tac-help-separator = use STRING as the separator instead of newline
 
 # Error messages
 tac-error-invalid-regex = invalid regular expression: { $error }
-tac-error-invalid-directory-argument = { $argument }: read error: Is a directory
-tac-error-file-not-found = failed to open { $filename } for reading: No such file or directory
-tac-error-read-error = failed to read from { $filename }: { $error }
+tac-error-open-error = failed to open { $filename } for reading: { $error }
+tac-error-read-error = { $filename }: read error: { $error }
 tac-error-write-error = failed to write to stdout: { $error }

src/uu/tac/locales/fr-FR.ftl

@@ -6,7 +6,6 @@ tac-help-separator = utiliser CHAÎNE comme séparateur au lieu du saut de ligne
 
 # Messages d'erreur
 tac-error-invalid-regex = expression régulière invalide : { $error }
-tac-error-file-not-found = échec de l'ouverture de { $filename } en lecture : Aucun fichier ou répertoire de ce type
-tac-error-read-error = échec de la lecture depuis { $filename } : { $error }
+tac-error-open-error = échec de l'ouverture de { $filename } en lecture : { $error }
+tac-error-read-error = { $filename } : erreur de lecture : { $error }
 tac-error-write-error = échec de l'écriture vers stdout : { $error }
-tac-error-invalid-directory-argument = { $argument } : erreur de lecture : Est un répertoire

src/uu/tac/src/error.rs

@@ -7,31 +7,31 @@
 use std::ffi::OsString;
 use thiserror::Error;
 use uucore::display::Quotable;
-use uucore::error::UError;
+use uucore::error::{UError, strip_errno};
 use uucore::translate;
 
 #[derive(Debug, Error)]
 pub enum TacError {
     /// A regular expression given by the user is invalid.
     #[error("{}", translate!("tac-error-invalid-regex", "error" => .0))]
     InvalidRegex(regex::Error),
-    /// The argument to tac is a directory.
-    #[error("{}", translate!("tac-error-invalid-directory-argument", "argument" => .0.maybe_quote()))]
-    InvalidDirectoryArgument(OsString),
-    /// The specified file is not found on the filesystem.
-    #[error("{}", translate!("tac-error-file-not-found", "filename" => .0.quote()))]
-    FileNotFound(OsString),
+    /// An error opening a file for reading.
+    ///
+    /// The parameters are the name of the file and the underlying
+    /// [`std::io::Error`] that caused this error.
+    #[error("{}", translate!("tac-error-open-error", "filename" => .0.quote(), "error" => strip_errno(.1)))]
+    OpenError(OsString, std::io::Error),
     /// An error reading the contents of a file or stdin.
     ///
     /// The parameters are the name of the file and the underlying
     /// [`std::io::Error`] that caused this error.
-    #[error("{}", translate!("tac-error-read-error", "filename" => .0.quote(), "error" => .1))]
+    #[error("{}", translate!("tac-error-read-error", "filename" => .0.maybe_quote(), "error" => strip_errno(.1)))]
     ReadError(OsString, std::io::Error),
     /// An error writing the (reversed) contents of a file or stdin.
     ///
     /// The parameter is the underlying [`std::io::Error`] that caused
     /// this error.
-    #[error("{}", translate!("tac-error-write-error", "error" => .0))]
+    #[error("{}", translate!("tac-error-write-error", "error" => strip_errno(.0)))]
     WriteError(std::io::Error),
 }
 

src/uu/tac/src/tac.rs

@@ -12,15 +12,12 @@ use memchr::memmem;
 use memmap2::Mmap;
 use std::ffi::{OsStr, OsString};
 use std::io::{BufWriter, Read, Write, stdin, stdout};
-use std::{
-    fs::{File, read},
-    io::copy,
-    path::Path,
-};
-
+use std::{fs::File, io::copy, path::Path};
+#[cfg(unix)]
+use uucore::error::UError;
+use uucore::error::UResult;
 #[cfg(unix)]
 use uucore::error::set_exit_code;
-use uucore::error::{UError, UResult};
 use uucore::{format_usage, show};
 
 use crate::error::TacError;
@@ -378,31 +375,26 @@ fn tac(filenames: &[OsString], before: bool, regex: bool, separator: &OsStr) ->
             }
         } else {
             let path = Path::new(filename);
-            if path.is_dir() {
-                let e: Box<dyn UError> =
-                    TacError::InvalidDirectoryArgument(filename.clone()).into();
-                show!(e);
-                continue;
-            }
-
-            if path.metadata().is_err() {
-                let e: Box<dyn UError> = TacError::FileNotFound(filename.clone()).into();
-                show!(e);
-                continue;
-            }
+            let mut file = match File::open(path) {
+                Ok(f) => f,
+                Err(e) => {
+                    show!(TacError::OpenError(filename.clone(), e));
+                    continue;
+                }
+            };
 
-            if let Some(mmap1) = try_mmap_path(path) {
+            if let Some(mmap1) = try_mmap_file(&file) {
                 mmap = mmap1;
                 &mmap
             } else {
-                match read(path) {
-                    Ok(buf1) => {
-                        buf = buf1;
+                let mut contents = Vec::new();
+                match file.read_to_end(&mut contents) {
+                    Ok(_) => {
+                        buf = contents;
                         &buf
                     }
                     Err(e) => {
-                        let e: Box<dyn UError> = TacError::ReadError(filename.clone(), e).into();
-                        show!(e);
+                        show!(TacError::ReadError(filename.clone(), e));
                         continue;
                     }
                 }
@@ -458,14 +450,10 @@ fn buffer_stdin() -> std::io::Result<StdinData> {
     }
 }
 
-fn try_mmap_path(path: &Path) -> Option<Mmap> {
-    let file = File::open(path).ok()?;
-
+fn try_mmap_file(file: &File) -> Option<Mmap> {
     // SAFETY: If the file is truncated while we map it, SIGBUS will be raised
     // and our process will be terminated, thus preventing access of invalid memory.
-    let mmap = unsafe { Mmap::map(&file).ok()? };
-
-    Some(mmap)
+    unsafe { Mmap::map(file).ok() }
 }
 
 #[cfg(test)]

tests/by-util/test_tac.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore axxbxx bxxaxx axxx axxxx xxaxx xxax xxxxa axyz zyax zyxa bbaaa aaabc bcdddd cddddaaabc xyzabc abcxyzabc nbbaaa
+// spell-checker:ignore axxbxx bxxaxx axxx axxxx xxaxx xxax xxxxa axyz zyax zyxa bbaaa aaabc bcdddd cddddaaabc xyzabc abcxyzabc nbbaaa EISDIR
 #[cfg(target_os = "linux")]
 use uutests::at_and_ucmd;
 use uutests::new_ucmd;
@@ -83,18 +83,23 @@ fn test_invalid_input() {
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
 
-    scene
-        .ucmd()
-        .arg("b")
-        .fails()
-        .stderr_contains("failed to open 'b' for reading: No such file or directory");
+    #[cfg(not(windows))]
+    let not_found_err = "failed to open 'b' for reading: No such file or directory";
+    #[cfg(windows)]
+    let not_found_err =
+        "failed to open 'b' for reading: The system cannot find the file specified.";
+
+    scene.ucmd().arg("b").fails().stderr_contains(not_found_err);
 
     at.mkdir("a");
-    scene
-        .ucmd()
-        .arg("a")
-        .fails()
-        .stderr_contains("a: read error: Is a directory");
+    // On Unix, File::open succeeds on directories but read_to_end fails with EISDIR.
+    // On Windows, File::open on a directory fails with "Access is denied".
+    #[cfg(not(windows))]
+    let dir_err = "a: read error: Is a directory";
+    #[cfg(windows)]
+    let dir_err = "failed to open 'a' for reading: Access is denied";
+
+    scene.ucmd().arg("a").fails().stderr_contains(dir_err);
 }
 
 #[test]
@@ -387,7 +392,7 @@ fn test_failed_write_is_reported() {
         .pipe_in("hello")
         .set_stdout(std::fs::File::create("/dev/full").unwrap())
         .fails()
-        .stderr_is("tac: failed to write to stdout: No space left on device (os error 28)\n");
+        .stderr_is("tac: failed to write to stdout: No space left on device\n");
 }
 
 #[cfg(target_os = "linux")]