`cp -r` throws read permission error on unwritable destination file (mode: 440)

[WhitePeter]

When copying recursively and trying to overwrite existing files with write permission flag removed, e.g. mode 440, cp fails for the wrong reason:

coreutils --version
coreutils 0.4.0 (multi-call binary)

umask
027
mkdir a b
touch a/foo
chmod -w a/foo # mode is now 440
cd b

coreutils cp -r ../a .
# and again to try overwriting
coreutils cp -r ../a .
cp: cannot open '../a/foo' for reading: permission denied

# and now GNU coreutils
cp -r ../a .
cp: cannot create regular file './a/foo': Permission denied

# when overwriting the file directly (without -r) the error is correct, albeit the message differs
coreutils cp ../a/foo a
cp: '../a/foo' -> 'a/foo': Permission denied (os error 13) # I like the GNU message more, os error 13 means nothing to me but that the target is a regular file is not immediately obvious

Somewhere there is confusion about the cause of the error, because the cause is clearly the unwritable destination. And it must be something to do with recursive copying, because when overwriting the destination file directly the error message matches the failure condition.

Also the (last, correct) message is not quite the same as the one GNU cp emits. I don't know if this counts as one of those feature parity bugs or if it's an intended divergence, but in this case I find the GNU message just a tad more informative. As a user I have never seen os error 13 and I'd argue that users don't need to concern themselves with such details; they should be translated, i.e. "Permission denied", "os error 13" is thus redundant. The space is better used by providing additional information, such as the GNU message, that the destination is a regular file.

P.S.: I don't think it's relevant but for completeness I've added information about the umask.

[FidelSch]

Relevant piece of code:

src/uu/cp/src/platform/linux.rs

/// Use the Linux `ioctl_ficlone` API to do a copy-on-write clone.
///
/// `fallback` controls what to do if the system call fails.
#[cfg(any(target_os = "linux", target_os = "android"))]
fn clone<P>(source: P, dest: P, fallback: CloneFallback) -> std::io::Result<()>
where
    P: AsRef<Path>,
{
    let src_file = File::open(&source)?;
    let dst_file = File::create(&dest)?;
    let src_fd = src_file.as_raw_fd();
    let dst_fd = dst_file.as_raw_fd();
    let result = unsafe { libc::ioctl(dst_fd, libc::FICLONE, src_fd) };
    if result == 0 {
        return Ok(());
    }
    match fallback {
        CloneFallback::Error => Err(std::io::Error::last_os_error()),
        CloneFallback::FSCopy => std::fs::copy(source, dest).map(|_| ()),
        CloneFallback::SparseCopy => sparse_copy(source, dest),
        CloneFallback::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
    }
}

The clone function used for file copying may raise an std::io::PermissionDenied error on either being unable to open the source for reading, or to open the destination for writing, so we cannot know which failed based on the error code.

Checking file permissions after the fact seems to work fine on my build, but it feels like redundant work...

[WhitePeter]

My Rust skills are virtually non-existent so forgive me if I am misreading this code snippet. To me it looks like, in this particular case, CloneFallback::FSCopy is the path taken the first time when writing to a non-existent destination, because steps above were all done inside a tmpfs which does not support cloning (reflink copy). And on the second time it should already fail at at File::create(&dest)?, since dest does now exist but write permission is denied. Isn't that all the information necessary to correctly attribute the actual cause of the error? Or is that just one of many cases which all result in an ambiguous EPERM? Is it CloneFallback::Error which has lost that context because it's a catch all? Can it be made smarter to know the exact error path? Or just some variable that saves the action that failed?

Or is there an actual bug and it does fail at File::Open(&source)?

[FidelSch]

Exactly. When trying to open the (already existing) destination file with File::create(&dest), the operation fails with a PermissionDenied error, which is the same one raised by File::open(&source) when trying to open for reading. If you are unfamiliar with Rust, I suggest reading into the ? operator. I believe the more 'rusty' way of handling this would be by changing the return type to a custom Result type whith distinct variants for each possible error. Hope this helps.

[WhitePeter]

FYI, i think I now know the root cause. By tinkering a bit I think I have narrowed it down to CloneFallback::FSCopy => std::fs::copy(source, dest).map(|_| ()), because that is the real fallback. std::fs::copy masks the real cause because it set the same errno (EPERM) for both cases (no read permission on source, no write permission on dest). The reference manual states that internally it tries to use copy_file_range(2) (aka man 2 copy_file_range), which only uses EPERM for immutable destinations and EBADF for other read/write issues. So this looks like a deficiency of the std crate, or at least this particular fs module function (copy()).

So I think a more granular/manual approach might be needed to avoid masking the true error cause, like splitting the copy call into read/write() with immediate error handling, or whatever equivalent rusty approach. At least from my perspective there seems to be no way around it.

[WhitePeter]

FWIW, I am not too hung up on not being able to tell the root cause; if it's a read side of write side error. What irks me is the fact that the error message is incorrect and downright misleading. Given my current understanding, breaking up the std::fs::copy into read/write might incur penalties, because the data has to be handled in userspace whereas, if copy_file_range does work, it's a pure kernel space operation; one might even call it transactional in nature.

To summarize, having an error like shown in the last line of above shell output snippet, cp: '../a/foo' -> 'a/foo': Permission denied (os error 13), would be sufficient IMHO. While GNU's error handling is arguably better, it may actually just be an accident; depending on how long (an approach like) copy_file_range has been around, this could just be how it was done prior to that. So it seems reasonable not to try copying GNU to the tee here and just file away the more coarse error handling under "implementation specific quirks".