Merge pull request #2610 from miDeb/cp/abuse

cp: handle edge cases when dest is a symlink

.vscode/cspell.dictionaries/jargon.wordlist.txt

@@ -59,6 +59,7 @@ kibibytes
 libacl
 lcase
 lossily
+lstat
 mebi
 mebibytes
 mergeable

Cargo.lock

@@ -3212,6 +3212,7 @@ dependencies = [
  "walkdir",
  "wild",
  "winapi 0.3.9",
+ "winapi-util",
  "z85",
 ]
 

src/uu/cat/src/cat.rs

@@ -20,6 +20,7 @@ use std::io::{self, Read, Write};
 use thiserror::Error;
 use uucore::display::Quotable;
 use uucore::error::UResult;
+use uucore::fs::FileInformation;
 
 #[cfg(unix)]
 use std::os::unix::io::AsRawFd;
@@ -317,8 +318,7 @@ fn cat_path(
     path: &str,
     options: &OutputOptions,
     state: &mut OutputState,
-    #[cfg(unix)] out_info: &nix::sys::stat::FileStat,
+    out_info: Option<&FileInformation>,
-    #[cfg(windows)] out_info: &winapi_util::file::Information,
 ) -> CatResult<()> {
     if path == "-" {
         let stdin = io::stdin();
@@ -342,10 +342,15 @@ fn cat_path(
         }
         _ => {
             let file = File::open(path)?;
-            #[cfg(any(windows, unix))]
+
-            if same_file(out_info, &file) {
+            if let Some(out_info) = out_info {
+                if out_info.file_size() != 0
+                    && FileInformation::from_file(&file).as_ref() == Some(out_info)
+                {
                     return Err(CatError::OutputIsInput);
                 }
+            }
+
             let mut handle = InputHandle {
                 reader: file,
                 is_interactive: false,
@@ -355,25 +360,8 @@ fn cat_path(
     }
 }
 
-#[cfg(unix)]
-fn same_file(a_info: &nix::sys::stat::FileStat, b: &File) -> bool {
-    let b_info = nix::sys::stat::fstat(b.as_raw_fd()).unwrap();
-    b_info.st_size != 0 && b_info.st_dev == a_info.st_dev && b_info.st_ino == a_info.st_ino
-}
-
-#[cfg(windows)]
-fn same_file(a_info: &winapi_util::file::Information, b: &File) -> bool {
-    let b_info = winapi_util::file::information(b).unwrap();
-    b_info.file_size() != 0
-        && b_info.volume_serial_number() == a_info.volume_serial_number()
-        && b_info.file_index() == a_info.file_index()
-}
-
 fn cat_files(files: Vec<String>, options: &OutputOptions) -> UResult<()> {
-    #[cfg(windows)]
+    let out_info = FileInformation::from_file(&std::io::stdout());
-    let out_info = winapi_util::file::information(&std::io::stdout()).unwrap();
-    #[cfg(unix)]
-    let out_info = nix::sys::stat::fstat(std::io::stdout().as_raw_fd()).unwrap();
 
     let mut state = OutputState {
         line_number: 1,
@@ -384,7 +372,7 @@ fn cat_files(files: Vec<String>, options: &OutputOptions) -> UResult<()> {
     let mut error_messages: Vec<String> = Vec::new();
 
     for path in &files {
-        if let Err(err) = cat_path(path, options, &mut state, &out_info) {
+        if let Err(err) = cat_path(path, options, &mut state, out_info.as_ref()) {
             error_messages.push(format!("{}: {}", path.maybe_quote(), err));
         }
     }

src/uu/cp/src/cp.rs

@@ -8,7 +8,7 @@
 // For the full copyright and license information, please view the LICENSE file
 // that was distributed with this source code.
 
-// spell-checker:ignore (ToDO) ficlone linkgs lstat nlink nlinks pathbuf reflink strs xattrs
+// spell-checker:ignore (ToDO) ficlone linkgs lstat nlink nlinks pathbuf reflink strs xattrs symlinked
 
 #[cfg(target_os = "linux")]
 #[macro_use]
@@ -19,6 +19,7 @@ extern crate quick_error;
 extern crate uucore;
 
 use uucore::display::Quotable;
+use uucore::fs::FileInformation;
 #[cfg(windows)]
 use winapi::um::fileapi::CreateFileW;
 #[cfg(windows)]
@@ -838,8 +839,10 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu
 
     let mut non_fatal_errors = false;
     let mut seen_sources = HashSet::with_capacity(sources.len());
+    let mut symlinked_files = HashSet::new();
     for source in sources {
         if seen_sources.contains(source) {
+            // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases)
             show_warning!("source {} specified more than once", source.quote());
         } else {
             let mut found_hard_link = false;
@@ -848,7 +851,9 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu
                 preserve_hardlinks(&mut hard_links, source, dest, &mut found_hard_link).unwrap();
             }
             if !found_hard_link {
-                if let Err(error) = copy_source(source, target, &target_type, options) {
+                if let Err(error) =
+                    copy_source(source, target, &target_type, options, &mut symlinked_files)
+                {
                     match error {
                         // When using --no-clobber, we don't want to show
                         // an error message
@@ -909,15 +914,16 @@ fn copy_source(
     target: &TargetSlice,
     target_type: &TargetType,
     options: &Options,
+    symlinked_files: &mut HashSet<FileInformation>,
 ) -> CopyResult<()> {
     let source_path = Path::new(&source);
     if source_path.is_dir() {
         // Copy as directory
-        copy_directory(source, target, options)
+        copy_directory(source, target, options, symlinked_files)
     } else {
         // Copy as file
         let dest = construct_dest_path(source_path, target, target_type, options)?;
-        copy_file(source_path, dest.as_path(), options)
+        copy_file(source_path, dest.as_path(), options, symlinked_files)
     }
 }
 
@@ -947,14 +953,19 @@ fn adjust_canonicalization(p: &Path) -> Cow<Path> {
 ///
 /// Any errors encountered copying files in the tree will be logged but
 /// will not cause a short-circuit.
-fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyResult<()> {
+fn copy_directory(
+    root: &Path,
+    target: &TargetSlice,
+    options: &Options,
+    symlinked_files: &mut HashSet<FileInformation>,
+) -> CopyResult<()> {
     if !options.recursive {
         return Err(format!("omitting directory {}", root.quote()).into());
     }
 
     // if no-dereference is enabled and this is a symlink, copy it as a file
     if !options.dereference && fs::symlink_metadata(root).unwrap().file_type().is_symlink() {
-        return copy_file(root, target, options);
+        return copy_file(root, target, options, symlinked_files);
     }
 
     let current_dir =
@@ -1011,7 +1022,7 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR
 
         let local_to_target = target.join(&local_to_root_parent);
         if is_symlink && !options.dereference {
-            copy_link(&path, &local_to_target)?;
+            copy_link(&path, &local_to_target, symlinked_files)?;
         } else if path.is_dir() && !local_to_target.exists() {
             or_continue!(fs::create_dir_all(local_to_target));
         } else if !path.is_dir() {
@@ -1021,7 +1032,12 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR
                 let dest = local_to_target.as_path().to_path_buf();
                 preserve_hardlinks(&mut hard_links, &source, dest, &mut found_hard_link).unwrap();
                 if !found_hard_link {
-                    match copy_file(path.as_path(), local_to_target.as_path(), options) {
+                    match copy_file(
+                        path.as_path(),
+                        local_to_target.as_path(),
+                        options,
+                        symlinked_files,
+                    ) {
                         Ok(_) => Ok(()),
                         Err(err) => {
                             if fs::symlink_metadata(&source)?.file_type().is_symlink() {
@@ -1036,7 +1052,12 @@ fn copy_directory(root: &Path, target: &TargetSlice, options: &Options) -> CopyR
                     }?;
                 }
             } else {
-                copy_file(path.as_path(), local_to_target.as_path(), options)?;
+                copy_file(
+                    path.as_path(),
+                    local_to_target.as_path(),
+                    options,
+                    symlinked_files,
+                )?;
             }
         }
     }
@@ -1145,18 +1166,24 @@ fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResu
     Ok(())
 }
 
-#[cfg(not(windows))]
+fn symlink_file(
-#[allow(clippy::unnecessary_wraps)] // needed for windows version
+    source: &Path,
-fn symlink_file(source: &Path, dest: &Path, context: &str) -> CopyResult<()> {
+    dest: &Path,
-    match std::os::unix::fs::symlink(source, dest).context(context) {
+    context: &str,
-        Ok(_) => Ok(()),
+    symlinked_files: &mut HashSet<FileInformation>,
-        Err(_) => Ok(()),
+) -> CopyResult<()> {
+    #[cfg(not(windows))]
+    {
+        std::os::unix::fs::symlink(source, dest).context(context)?;
     }
-}
+    #[cfg(windows)]
-
+    {
-#[cfg(windows)]
+        std::os::windows::fs::symlink_file(source, dest).context(context)?;
-fn symlink_file(source: &Path, dest: &Path, context: &str) -> CopyResult<()> {
+    }
-    Ok(std::os::windows::fs::symlink_file(source, dest).context(context)?)
+    if let Some(file_info) = FileInformation::from_path(dest, false) {
+        symlinked_files.insert(file_info);
+    }
+    Ok(())
 }
 
 fn context_for(src: &Path, dest: &Path) -> String {
@@ -1183,6 +1210,7 @@ fn handle_existing_dest(source: &Path, dest: &Path, options: &Options) -> CopyRe
     }
 
     match options.overwrite {
+        // FIXME: print that the file was removed if --verbose is enabled
         OverwriteMode::Clobber(ClobberMode::Force) => {
             if fs::metadata(dest)?.permissions().readonly() {
                 fs::remove_file(dest)?;
@@ -1206,11 +1234,39 @@ fn handle_existing_dest(source: &Path, dest: &Path, options: &Options) -> CopyRe
 ///
 /// The original permissions of `source` will be copied to `dest`
 /// after a successful copy.
-fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> {
+fn copy_file(
+    source: &Path,
+    dest: &Path,
+    options: &Options,
+    symlinked_files: &mut HashSet<FileInformation>,
+) -> CopyResult<()> {
     if dest.exists() {
         handle_existing_dest(source, dest, options)?;
     }
 
+    // Fail if dest is a dangling symlink or a symlink this program created previously
+    if fs::symlink_metadata(dest)
+        .map(|m| m.file_type().is_symlink())
+        .unwrap_or(false)
+    {
+        if FileInformation::from_path(dest, false)
+            .map(|info| symlinked_files.contains(&info))
+            .unwrap_or(false)
+        {
+            return Err(Error::Error(format!(
+                "will not copy '{}' through just-created symlink '{}'",
+                source.display(),
+                dest.display()
+            )));
+        }
+        if !dest.exists() {
+            return Err(Error::Error(format!(
+                "not writing through dangling symlink '{}'",
+                dest.display()
+            )));
+        }
+    }
+
     if options.verbose {
         println!("{}", context_for(source, dest));
     }
@@ -1255,10 +1311,10 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> {
             fs::hard_link(&source, &dest).context(context)?;
         }
         CopyMode::Copy => {
-            copy_helper(&source, &dest, options, context)?;
+            copy_helper(&source, &dest, options, context, symlinked_files)?;
         }
         CopyMode::SymLink => {
-            symlink_file(&source, &dest, context)?;
+            symlink_file(&source, &dest, context, symlinked_files)?;
         }
         CopyMode::Sparse => return Err(Error::NotImplemented(options::SPARSE.to_string())),
         CopyMode::Update => {
@@ -1271,10 +1327,10 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> {
                 if src_time <= dest_time {
                     return Ok(());
                 } else {
-                    copy_helper(&source, &dest, options, context)?;
+                    copy_helper(&source, &dest, options, context, symlinked_files)?;
                 }
             } else {
-                copy_helper(&source, &dest, options, context)?;
+                copy_helper(&source, &dest, options, context, symlinked_files)?;
             }
         }
         CopyMode::AttrOnly => {
@@ -1308,7 +1364,13 @@ fn copy_file(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> {
 
 /// Copy the file from `source` to `dest` either using the normal `fs::copy` or a
 /// copy-on-write scheme if --reflink is specified and the filesystem supports it.
-fn copy_helper(source: &Path, dest: &Path, options: &Options, context: &str) -> CopyResult<()> {
+fn copy_helper(
+    source: &Path,
+    dest: &Path,
+    options: &Options,
+    context: &str,
+    symlinked_files: &mut HashSet<FileInformation>,
+) -> CopyResult<()> {
     if options.parents {
         let parent = dest.parent().unwrap_or(dest);
         fs::create_dir_all(parent)?;
@@ -1320,7 +1382,7 @@ fn copy_helper(source: &Path, dest: &Path, options: &Options, context: &str) ->
          */
         File::create(dest).context(dest.display().to_string())?;
     } else if is_symlink {
-        copy_link(source, dest)?;
+        copy_link(source, dest, symlinked_files)?;
     } else if options.reflink_mode != ReflinkMode::Never {
         #[cfg(not(any(target_os = "linux", target_os = "macos")))]
         return Err("--reflink is only supported on linux and macOS"
@@ -1338,7 +1400,11 @@ fn copy_helper(source: &Path, dest: &Path, options: &Options, context: &str) ->
     Ok(())
 }
 
-fn copy_link(source: &Path, dest: &Path) -> CopyResult<()> {
+fn copy_link(
+    source: &Path,
+    dest: &Path,
+    symlinked_files: &mut HashSet<FileInformation>,
+) -> CopyResult<()> {
     // Here, we will copy the symlink itself (actually, just recreate it)
     let link = fs::read_link(&source)?;
     let dest: Cow<'_, Path> = if dest.is_dir() {
@@ -1358,7 +1424,7 @@ fn copy_link(source: &Path, dest: &Path) -> CopyResult<()> {
         }
         dest.into()
     };
-    symlink_file(&link, &dest, &*context_for(&link, &dest))
+    symlink_file(&link, &dest, &*context_for(&link, &dest), symlinked_files)
 }
 
 /// Copies `source` to `dest` using copy-on-write if possible.

src/uucore/Cargo.toml

@@ -41,6 +41,7 @@ lazy_static = "1.3"
 
 [target.'cfg(target_os = "windows")'.dependencies]
 winapi = { version = "0.3", features = ["errhandlingapi", "fileapi", "handleapi", "winerror"] }
+winapi-util = { version= "0.1.5", optional=true }
 
 [target.'cfg(target_os = "redox")'.dependencies]
 termion = "1.5"
@@ -50,7 +51,7 @@ default = []
 # * non-default features
 encoding = ["data-encoding", "data-encoding-macro", "z85", "thiserror"]
 entries = ["libc"]
-fs = ["libc"]
+fs = ["libc", "nix", "winapi-util"]
 fsext = ["libc", "time"]
 mode = ["libc"]
 perms = ["libc", "walkdir"]

src/uucore/src/lib/features/fs.rs

@@ -17,12 +17,15 @@ use libc::{
 use std::borrow::Cow;
 use std::env;
 use std::fs;
+use std::hash::Hash;
 use std::io::Error as IOError;
 use std::io::Result as IOResult;
 use std::io::{Error, ErrorKind};
-#[cfg(any(unix, target_os = "redox"))]
+#[cfg(unix)]
-use std::os::unix::fs::MetadataExt;
+use std::os::unix::{fs::MetadataExt, io::AsRawFd};
 use std::path::{Component, Path, PathBuf};
+#[cfg(target_os = "windows")]
+use winapi_util::AsHandleRef;
 
 #[cfg(unix)]
 #[macro_export]
@@ -32,6 +35,114 @@ macro_rules! has {
     };
 }
 
+/// Information to uniquely identify a file
+pub struct FileInformation(
+    #[cfg(unix)] nix::sys::stat::FileStat,
+    #[cfg(windows)] winapi_util::file::Information,
+);
+
+impl FileInformation {
+    /// Get information from a currently open file
+    #[cfg(unix)]
+    pub fn from_file(file: &impl AsRawFd) -> Option<Self> {
+        if let Ok(x) = nix::sys::stat::fstat(file.as_raw_fd()) {
+            Some(Self(x))
+        } else {
+            None
+        }
+    }
+
+    /// Get information from a currently open file
+    #[cfg(target_os = "windows")]
+    pub fn from_file(file: &impl AsHandleRef) -> Option<Self> {
+        if let Ok(x) = winapi_util::file::information(file.as_handle_ref()) {
+            Some(Self(x))
+        } else {
+            None
+        }
+    }
+
+    /// Get information for a given path.
+    ///
+    /// If `path` points to a symlink and `dereference` is true, information about
+    /// the link's target will be returned.
+    pub fn from_path(path: impl AsRef<Path>, dereference: bool) -> Option<Self> {
+        #[cfg(unix)]
+        {
+            let stat = if dereference {
+                nix::sys::stat::stat(path.as_ref())
+            } else {
+                nix::sys::stat::lstat(path.as_ref())
+            };
+            if let Ok(stat) = stat {
+                Some(Self(stat))
+            } else {
+                None
+            }
+        }
+        #[cfg(target_os = "windows")]
+        {
+            use std::fs::OpenOptions;
+            use std::os::windows::prelude::*;
+            let mut open_options = OpenOptions::new();
+            if !dereference {
+                open_options.custom_flags(winapi::um::winbase::FILE_FLAG_OPEN_REPARSE_POINT);
+            }
+            open_options
+                .read(true)
+                .open(path.as_ref())
+                .ok()
+                .and_then(|file| Self::from_file(&file))
+        }
+    }
+
+    pub fn file_size(&self) -> u64 {
+        #[cfg(unix)]
+        {
+            use std::convert::TryInto;
+
+            assert!(self.0.st_size >= 0, "File size is negative");
+            self.0.st_size.try_into().unwrap()
+        }
+        #[cfg(target_os = "windows")]
+        {
+            self.0.file_size()
+        }
+    }
+}
+
+#[cfg(unix)]
+impl PartialEq for FileInformation {
+    fn eq(&self, other: &Self) -> bool {
+        self.0.st_dev == other.0.st_dev && self.0.st_ino == other.0.st_ino
+    }
+}
+
+#[cfg(target_os = "windows")]
+impl PartialEq for FileInformation {
+    fn eq(&self, other: &Self) -> bool {
+        self.0.volume_serial_number() == other.0.volume_serial_number()
+            && self.0.file_index() == other.0.file_index()
+    }
+}
+
+impl Eq for FileInformation {}
+
+impl Hash for FileInformation {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        #[cfg(unix)]
+        {
+            self.0.st_dev.hash(state);
+            self.0.st_ino.hash(state);
+        }
+        #[cfg(target_os = "windows")]
+        {
+            self.0.volume_serial_number().hash(state);
+            self.0.file_index().hash(state);
+        }
+    }
+}
+
 pub fn resolve_relative_path(path: &Path) -> Cow<Path> {
     if path.components().all(|e| e != Component::ParentDir) {
         return path.into();

tests/by-util/test_cp.rs

@@ -1379,3 +1379,42 @@ fn test_canonicalize_symlink() {
         .no_stderr()
         .no_stdout();
 }
+
+#[test]
+fn test_copy_through_just_created_symlink() {
+    for &create_t in &[true, false] {
+        let (at, mut ucmd) = at_and_ucmd!();
+        at.mkdir("a");
+        at.mkdir("b");
+        at.mkdir("c");
+        #[cfg(unix)]
+        fs::symlink("../t", at.plus("a/1")).unwrap();
+        #[cfg(target_os = "windows")]
+        symlink_file("../t", at.plus("a/1")).unwrap();
+        at.touch("b/1");
+        if create_t {
+            at.touch("t");
+        }
+        ucmd.arg("--no-dereference")
+            .arg("a/1")
+            .arg("b/1")
+            .arg("c")
+            .fails()
+            .stderr_only(if cfg!(not(target_os = "windows")) {
+                "cp: will not copy 'b/1' through just-created symlink 'c/1'"
+            } else {
+                "cp: will not copy 'b/1' through just-created symlink 'c\\1'"
+            });
+    }
+}
+
+#[test]
+fn test_copy_through_dangling_symlink() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.touch("file");
+    at.symlink_file("nonexistent", "target");
+    ucmd.arg("file")
+        .arg("target")
+        .fails()
+        .stderr_only("cp: not writing through dangling symlink 'target'");
+}