fs::copy: Cannot copy special/device files (ex: /dev/null) into a file

[sylvestre]

I tried this code:

use std::fs;

fn main() {
    match fs::copy("/dev/null", "bar.txt") {
        Ok(_n) => (),
        Err(e) => {
            println!("{}", e);
            ()
        }
    }
}

I expected to see this happen:
I agree that the use case isn't super useful. But I would expect this to work and create an empty file.

Instead, this happened:

the source path is not an existing regular file

Don't hesitate to wontfix this bug...

[jonas-schievink]

Well, this is documented behavior:

This function will return an error in the following situations, but is not limited to just these cases:

• The from path is not a file.

[the8472]

In the light of "everything is a file" perhaps the documentation could be improved to say "regular file" .

[pickfire]

@sylvestre Not sure if #79399 is the kind of fix you are looking for, it fixes it from a documentation perspective.

I wonder if it is possible to allow copying from /dev/null, /dev/full and /dev/zero, they seemed to be useful in some cases. Should we just allow them?

[sylvestre]

@pickfire At least, the doc should be improved but yeah, I think this function should allow copy.

There are cases when /dev/null is used as a regular file like:
https://sources.debian.org/src/apparmor/2.13.5-1/debian/debhelper/postinst-apparmor/?hl=9#L9
https://sources.debian.org/src/mysql-8.0/8.0.21-1/packaging/rpm-common/mysqlrouter.init/?hl=56#L56

[the8472]

Should we just allow them?

It would be good to know why the check explicitly rejecting it has been added in the first place. It has been there since the module was introduced in 2015

And understanding the use-case would be good too. If you want to create an empty file (copying /dev/null) that can be achieved via File::with_options. Copying from /dev/zero would fill up your drive unless you dump it into something not backed by storage.

[sylvestre]

@the8472 It would be good to know why the check explicitly rejecting it has been added in the first place.

I guess the folks who implemented it didn't know that it wasn't uncommon to do
install /dev/null (for example: https://sources.debian.org/src/apparmor/2.13.5-1/debian/debhelper/postinst-apparmor/#L9 used in Debian & Ubuntu for evince, cups, libreoffice, passwd, etc)
or
cp /dev/null (for example: https://sources.debian.org/src/texlive-bin/2020.20200327.54578-5/texk/texlive/linked_scripts/texlive/mktexlsr/?hl=85#L187

[BartMassey]

Check should be removed in my opinion. Don't understand what it's checking against. As noted here, it is common in UNIX to copy from all kinds of special files. In addition to /dev/null and FIFOs, both mentioned already, there's the common case of copying from a disk or disk partition like /dev/sda1.

Yes, you have to be careful what you copy from and to. That's true in any case.

[ijackson]

Hrm, I just read the docs for fs::copy and it is supposed to copy the permissions. Maybe we need fs::copy_contents.

[the8472]

If you need custom behavior deviating from fs::copy you could open/create files as desired and then io::copy instead. Of course that option should be carefully conisdered since there are some things one can get wrong when rolling your own file creation in a security-sensitive context.

[BartMassey]

Hrm, I just read the docs for fs::copy and it is supposed to copy the permissions. Maybe we need fs::copy_contents.

I don't see why it can't copy the permissions from a device file? I think that may have been a poor design choice, but I don't think it gets in the way of fixing this issue.

[BartMassey]

If you need custom behavior deviating from fs::copy you could open/create files as desired and then io::copy instead.

I don't think the requested behavior deviates from the specification for fs::copy(). The spec promises to return an error when "the from path is not a file". As you noted earlier, in UNIX everything is a file. I think programs that rely on the current behavior to exclude special files are arguably buggy. I am of the school that when the spec and the implementation disagree, you fix the implementation unless there's an obvious bug in the spec. I see none here.

Curious if anyone can give an example where allowing fs::copy() from a device special file could cause unexpected bad behavior from an existing Rust program? Is there a backup program floating around that uses this to avoid backing up special files, for example? Wouldn't such a backup program also need to check the file stats for other reasons?

[the8472]

I was responding to the post directly above mine, not the general topic as I already covered that earlier.

[ijackson]

I don't think the requested behavior deviates from the specification for fs::copy(). The spec promises to return an error when "the from path is not a file". As you noted earlier, in UNIX everything is a file. I think programs that rely on the current behavior to exclude special files are arguably buggy. I am of the school that when the spec and the implementation disagree, you fix the implementation unless there's an obvious bug in the spec. I see none here.

I think this is rather stretched nterpretation of the text "the from path is not a file". A reader familiar with Unix will read that as "not a plain file". But I think it's fine to change the spec here. The issue with the permissions is more awkward (see below).

Curious if anyone can give an example where allowing fs::copy() from a device special file could cause unexpected bad behavior from an existing Rust program? Is there a backup program floating around that uses this to avoid backing up special files, for example? Wouldn't such a backup program also need to check the file stats for other reasons?

A backup program would definitely need to check the metadata for other reasons. But some kind of rudimentary version control system might not need to. Note that such a system would probably do something unhelpful with symlinks. (I haven't checked but I assume that fs::copy dereferences symlinks, both for source and destination.)

Hrm, I just read the docs for fs::copy and it is supposed to copy the permissions. Maybe we need fs::copy_contents.

I don't see why it can't copy the permissions from a device file? I think that may have been a poor design choice, but I don't think it gets in the way of fixing this issue.

Copying the permissions from a device file is definitely wrong. For example, /dev/null is usually 666 but on a plain file it is probably a serious security bug.

Stepping back, ISTM that we should (i) decide what the best behaviour would be (ii) consider whether that behaviour would be a breaking change (iii) make sure to document the behaviour unambiguously.

So

best behaviour for a basic file-copying function

On Unix, copying the execute bit is sensible. Copying read restrictions is sensible. I think copying the other permissions is not usually sensible for a simple file copying function.

Or to put it another way the behaviour should be as if we used open(2) with a suitable mode derived from the source mode, modified by the umask as usual. Having thought about it for a few minutes I think that the file should be rw where the umask doesn't contain w, and should otherwise not be w and inherit r and x from the source file subject to the umask. It should probably not inherit acls.

Symlinks should be dereferenced at both ends. If the source is not a plain file, the destination mode should be 666 modified by the umask (ie, we should call open(,,666).

IMO if the destination file does not exist it should not have its permissions changed. Ie, we fs::copy should compute somehow a value to pass as the third argument to open(,O_CREAT|O_TRUNC,).

is this a breaking change

Well, it clearly this would change the behaviour. Would it be a breaking change?

I think though that programs which need particular behaviour for the file mode will already need to have taken control of that themselves. For programs that didn't do anything special it's quite possible that if we change this behaviour we may fix bugs in those programs.

The existing spec says it copies permissions and it is not clear whether that includes acls or the immutable bit. A reader familiar with Unix would probably assume tht it meant just the file mode, but a reader not familiar with Unix terminology would perhaps think all permission-like things are included. I don't imagine the current implementation copies acls (I haven't checked) because that is very complicated (and not always even possible).

The unix mode permission bits are interpreted in the context of the file's ownership. What copying the group permissions means depends on what the group ownership of the new file is. On systems with universal personal groups and a umask of 002, copying the group-writeability of a file is generally a good idea. On other systems it may be a very bad idea; it might end up with a destination file group-writeable by a group like users.

So arguably this is a bugfix. I think this is not clear-cut.

[BartMassey]

@ijackson Great comment! I agree with pretty much all of it.

The only quibble I might have is with "A reader familiar with Unix will read that as 'not a plain file'." I just assumed they were talking about directories, which would be a perfectly normal restriction for a file copy program.

The permissions thing is, as you say, a big bag of worms. If I had spec'ed this, I probably would have said that new target files would be created 0777 modified by the umask (which is pretty much the normal behavior for UNIX things) and existing target files would keep their existing permissions. The situation is made worse when you start thinking about what should happen under Windows and other non-standard OS targets.

Again, I'm not sure there's anything "special" about device special files here. It's probably a mistake to preserve the permissions on any world-writeable file when creating a new copy: the umask should always be applied by default. This would definitely be a change in the specification, but I'm for it.

[joshtriplett]

I think part of the question here is whether we expect fs::copy to work like cp -a (which would copy device/special files as device/special files), or like cp (which copies the contents of files, even if they're device/special files).

I think it's worth noting that on Windows, this calls CopyFileEx, and on macOS, this calls fclonefileat or fcopyfile. It looks like those functions have substantial support for handling special files. This seems like a reasonable indication of what this function is attempting to do.

I think it may have made sense for this function to copy file contents even if the file is a device file or other special file, and then we may want a different function that does the equivalent of cp -a (which is tricky to get right, and it'd be nice to have a function for that).

That said, since not handling device/special files is a specifically documented property of this function, I'd be somewhat concerned that people wrote code that relies on this behavior. (That doesn't mean we can't change it, as it may fall under platform-specific behavior, but it may be imprudent for us to change it.)

Given that, the safest option might be to add a copy_contents function (name subject to bikeshedding) that omits the check for device/special files.

[BartMassey]

@joshtriplett Yeah, it seems like we're well into the "explore behavior in the wild" stage. Does anybody use this function in non-toy programs at all? Would the best course to be to deprecate and kill it, offering no replacement? It seems like this innocent-looking thing can lead to quite a hornet's nest of considerations.

[cheriimoya]

Just to throw another option in (i am actually just at the beginning of my rust journey), can't there be an fs::CopyOptions like the fs::OpenOptions for "special" things and behaviors? There you could do stuff like setting the umask, allowing different types of files to be copied or copying a symlink as is.

[BartMassey]

@cheriimoya Welcome to Rust! Yes, there absolutely could be an alternate API, which is one of the options being considered.

Sadly, you wouldn't want to use global state for setting options, for a whole bunch of really excellent reasons, so it really would have to be an entirely new API, exposing new functions. The danger here is that, as the old saying goes, "now you have two problems." It would be much better to preserve the existing API if we can agree on how to make it safe and palatable for everyone involved.

It's all especially frustrating because the simplest and arguably most common use case, just copying a plain file to another plain file, is almost right. Even there, though, the permissions spec might need to be changed to avoid really surprising behaviors — I had no idea until this thread that fs::copy() is spec-ed to ignore the target umask on UNIX systems. That's a security bug waiting to happen, in my opinion.

[cheriimoya]

thanks @BartMassey! oh, yeah, i thought that fs::copy was like fs::File::open but after reading the src it seems to be way more complicated than i initially thought:D

even after skimming through the code i still don't fully get it.. would it somehow be possible to move fs::copy to fs::File::copy and keeping the current api? Or is this not possible?

[BartMassey]

Any change to the existing API is breaking, and will probably never happen. Part of what makes the discussion of fs::copy() interesting is that it's loosely documented, so it's not clear what constitutes breakage.