Skip to content

Commit

Permalink
cp: show mode if target does not have S_IWUSR (PR uutils#6696) (uutil…
Browse files Browse the repository at this point in the history 
…s#6700)

* cp: show mode if target does not have S_IWUSR

If the target exists, and does not have the user write bit (S_IWUSR)
set, additional information should be added to the overwrite
confirmation prompt.

This should get the "i-2" test to pass. See
uutils#6658.

* cp: with -i, delete destination if needed

---------

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>
  • Loading branch information
@andrewliebenow @sylvestre
andrewliebenow and sylvestre committed Sep 18, 2024
1 parent 564dd47 commit b8150f5
Show file tree
Hide file tree
Showing 2 changed files with 165 additions and 39 deletions.
169 changes: 130 additions & 39 deletions src/uu/cp/src/cp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,59 @@ fn copy_source(
}
}

/// If `path` does not have `S_IWUSR` set, returns a tuple of the file's
/// mode in octal (index 0) and human-readable (index 1) formats.
///
/// If the destination of a copy operation is a file that is not writeable to
/// the owner (bit `S_IWUSR`), extra information needs to be added to the
/// interactive mode prompt: the mode (permissions) of the file in octal and
/// human-readable format.
// TODO
// The destination metadata can be read multiple times in the course of a single execution of `cp`.
// This fix adds yet another metadata read.
// Should this metadata be read once and then reused throughout the execution?
// https://github.com/uutils/coreutils/issues/6658
fn file_mode_for_interactive_overwrite(
#[cfg_attr(not(unix), allow(unused_variables))] path: &Path,
) -> Option<(String, String)> {
// Retain outer braces to ensure only one branch is included
{
#[cfg(unix)]
{
use libc::{mode_t, S_IWUSR};
use std::os::unix::prelude::MetadataExt;

match path.metadata() {
Ok(me) => {
// Cast is necessary on some platforms
let mode: mode_t = me.mode() as mode_t;

// It looks like this extra information is added to the prompt iff the file's user write bit is 0
// write permission, owner
if uucore::has!(mode, S_IWUSR) {
None
} else {
// Discard leading digits
let mode_without_leading_digits = mode & 0o7777;

Some((
format!("{mode_without_leading_digits:04o}"),
uucore::fs::display_permissions_unix(mode, false),
))
}
}
// TODO: How should failure to read the metadata be handled? Ignoring for now.
Err(_) => None,
}
}

#[cfg(not(unix))]
{
None
}
}
}

impl OverwriteMode {
fn verify(&self, path: &Path, debug: bool) -> CopyResult<()> {
match *self {
Expand All @@ -1410,7 +1463,18 @@ impl OverwriteMode {
Err(Error::Skipped(false))
}
Self::Interactive(_) => {
if prompt_yes!("overwrite {}?", path.quote()) {
let prompt_yes_result = if let Some((octal, human_readable)) =
file_mode_for_interactive_overwrite(path)
{
prompt_yes!(
"replace {}, overriding mode {octal} ({human_readable})?",
path.quote()
)
} else {
prompt_yes!("overwrite {}?", path.quote())
};

if prompt_yes_result {
Ok(())
} else {
Err(Error::Skipped(true))
Expand Down Expand Up @@ -1651,7 +1715,7 @@ fn handle_existing_dest(
dest: &Path,
options: &Options,
source_in_command_line: bool,
copied_files: &mut HashMap<FileInformation, PathBuf>,
copied_files: &HashMap<FileInformation, PathBuf>,
) -> CopyResult<()> {
// Disallow copying a file to itself, unless `--force` and
// `--backup` are both specified.
Expand Down Expand Up @@ -1679,46 +1743,73 @@ fn handle_existing_dest(
}
}
if !is_dest_removed {
match options.overwrite {
// FIXME: print that the file was removed if --verbose is enabled
OverwriteMode::Clobber(ClobberMode::Force) => {
if is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly() {
fs::remove_file(dest)?;
delete_dest_if_needed_and_allowed(
source,
dest,
options,
source_in_command_line,
copied_files,
)?;
}

Ok(())
}

/// Checks if:
/// * `dest` needs to be deleted before the copy operation can proceed
/// * the provided options allow this deletion
///
/// If so, deletes `dest`.
fn delete_dest_if_needed_and_allowed(
source: &Path,
dest: &Path,
options: &Options,
source_in_command_line: bool,
copied_files: &HashMap<FileInformation, PathBuf>,
) -> CopyResult<()> {
let delete_dest = match options.overwrite {
OverwriteMode::Clobber(cl) | OverwriteMode::Interactive(cl) => {
match cl {
// FIXME: print that the file was removed if --verbose is enabled
ClobberMode::Force => {
// TODO
// Using `readonly` here to check if `dest` needs to be deleted is not correct:
// "On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file's assigned group. It also does not check ACLs. Therefore the return value of this function cannot be relied upon to predict whether attempts to read or write the file will actually succeed."
// This results in some copy operations failing, because this necessary deletion is being skipped.
is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly()
}
}
OverwriteMode::Clobber(ClobberMode::RemoveDestination) => {
fs::remove_file(dest)?;
}
OverwriteMode::Clobber(ClobberMode::Standard) => {
// Consider the following files:
//
// * `src/f` - a regular file
// * `src/link` - a hard link to `src/f`
// * `dest/src/f` - a different regular file
//
// In this scenario, if we do `cp -a src/ dest/`, it is
// possible that the order of traversal causes `src/link`
// to get copied first (to `dest/src/link`). In that case,
// in order to make sure `dest/src/link` is a hard link to
// `dest/src/f` and `dest/src/f` has the contents of
// `src/f`, we delete the existing file to allow the hard
// linking.

if options.preserve_hard_links()
// only try to remove dest file only if the current source
// is hardlink to a file that is already copied
&& copied_files.contains_key(
&FileInformation::from_path(
source,
options.dereference(source_in_command_line),
)
.context(format!("cannot stat {}", source.quote()))?,
) {
fs::remove_file(dest)?;
ClobberMode::RemoveDestination => true,
ClobberMode::Standard => {
// Consider the following files:
//
// * `src/f` - a regular file
// * `src/link` - a hard link to `src/f`
// * `dest/src/f` - a different regular file
//
// In this scenario, if we do `cp -a src/ dest/`, it is
// possible that the order of traversal causes `src/link`
// to get copied first (to `dest/src/link`). In that case,
// in order to make sure `dest/src/link` is a hard link to
// `dest/src/f` and `dest/src/f` has the contents of
// `src/f`, we delete the existing file to allow the hard
// linking.
options.preserve_hard_links() &&
// only try to remove dest file only if the current source
// is hardlink to a file that is already copied
copied_files.contains_key(
&FileInformation::from_path(
source,
options.dereference(source_in_command_line)
).context(format!("cannot stat {}", source.quote()))?
)
}
}
_ => (),
};
}
OverwriteMode::NoClobber => false,
};

if delete_dest {
fs::remove_file(dest)?;
}

Ok(())
Expand Down
35 changes: 35 additions & 0 deletions tests/by-util/test_cp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,41 @@ fn test_cp_arg_interactive_verbose_clobber() {
.stdout_contains("skipped 'b'");
}

#[test]
#[cfg(unix)]
fn test_cp_f_i_verbose_non_writeable_destination_y() {
let (at, mut ucmd) = at_and_ucmd!();

at.touch("a");
at.touch("b");

// Non-writeable file
at.set_mode("b", 0o0000);

ucmd.args(&["-f", "-i", "--verbose", "a", "b"])
.pipe_in("y")
.succeeds()
.stderr_is("cp: replace 'b', overriding mode 0000 (---------)? ")
.stdout_is("'a' -> 'b'\n");
}

#[test]
#[cfg(unix)]
fn test_cp_f_i_verbose_non_writeable_destination_empty() {
let (at, mut ucmd) = at_and_ucmd!();

at.touch("a");
at.touch("b");

// Non-writeable file
at.set_mode("b", 0o0000);

ucmd.args(&["-f", "-i", "--verbose", "a", "b"])
.pipe_in("")
.fails()
.stderr_only("cp: replace 'b', overriding mode 0000 (---------)? ");
}

#[test]
#[cfg(target_os = "linux")]
fn test_cp_arg_link() {
Expand Down

0 comments on commit b8150f5

Please sign in to comment.