rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.89k stars 1.56k forks source link

Metadata of raw fd #3025

Open mahkoh opened 3 years ago

mahkoh commented 3 years ago

Afaik the shortest way to get the metadata of a raw fd is

    let f = unsafe { File::from_raw_fd(fd) };
    let metadata = f.metadata();
    mem::forget(f);

There should be some way to do this without temporarily creating a File. E.g. std::os::unix::io::metadata_of_raw_fd.

skade commented 3 years ago

I'm back and forth around this.

I'm against creating more and more unix-specific API of this kind that can easily be built outside of libstd.

A reason for it is that:

let f = unsafe { File::from_raw_fd(fd) };

Might not be a valid operation, as it assumes fd to be owned - something which may or may not be true, making a naïve implementation of this error prone.

Still, given that this function only makes sense on certain kinds of file descriptors, would it make sense to introduce a RawFile, which doesn't assume ownership, but exposes file-like operations, (debug)_asserting that fd is NotNull? I think this will yield better API than operating on RawFd, which can be anything.

mahkoh commented 3 years ago

Might not be a valid operation, as it assumes fd to be owned - something which may or may not be true, making a naïve implementation of this error prone.

Metadata cannot be constructed outside std. Therefore the only ways to construct it from a file descriptor is via File.

Still, given that this function only makes sense on certain kinds of file descriptors,

fstat can be executed for all file descriptors.

skade commented 3 years ago

Might not be a valid operation, as it assumes fd to be owned - something which may or may not be true, making a naïve implementation of this error prone.

Metadata cannot be constructed outside std. Therefore the only ways to construct it from a file descriptor is via File.

That is correct, but a reimplementation is easily possible. My point was about the ownership problem of File much more, we might encourage breach of contract.

Still, given that this function only makes sense on certain kinds of file descriptors,

fstat can be executed for all file descriptors.

Yes, it can, but that isn't exposed through the stdlib API, not even through the raw one. Metadata is part of the fs module only. Changing that is a far bigger debate.

While everything in Unix is a file, it may be a std::fs::File and the metadata returned a std::fs::Metadata.

It isn't stdlibs goal to implement everything that is possible with unix API, but a minimal useful set. For the full implementation, reaching to things line nix is a better.

mahkoh commented 3 years ago

That is correct, but a reimplementation is easily possible.

Of course crates that create a different metadata type are readily available: https://docs.rs/uapi/0.1.3/uapi/fn.fstat.html. That's not the point of this issue.

skade commented 3 years ago

That is correct, but a reimplementation is easily possible.

Of course crates that create a different metadata type are readily available: https://docs.rs/uapi/0.1.3/uapi/fn.fstat.html. That's not the point of this issue.

Yes. I know this is not the point of the issue and you did remove the second sentence when quoting that strongly supports your case.

My points was exactly that your example points to a problem, but that I think the argument you lay out for it does not point to a strong reason.

the8472 commented 3 years ago

ManuallyDrop::new(unsafe { File::from_raw_fd(fd) }).metadata() is more concise and already used in various places.

mahkoh commented 3 years ago

That is nicer.

and already used in various places.

If that is so then it's high time that a safe alternative is added to std.

the8472 commented 3 years ago

I'm not sure if it can be made safe since RawFd has no ownership or lifetime. So something doing a close/open in the meantime may reallocate the FD to a different file which might lead to races where you think you're fetching the metadata of something but actually start operating on something else. If you own a file there's no TOCTOU, but if you only have a fd then there is.

mahkoh commented 3 years ago

There is nothing unsafe about calling fstat on any integer at any time.

the8472 commented 3 years ago

The fstat itself not, but imagine some rube goldberg machine involving memory backed by memory-mapped files where mutable access is guarded by a metadata check on the inode number. This only works if there are no races, which the programmer has to assert.

At least I assume that something like that is the reason that FromRawFd is unsafe in general. It's not like any of the syscalls are unsafe individually.

mahkoh commented 3 years ago

https://github.com/rust-lang/rust/issues/72175#issuecomment-663681255

the8472 commented 3 years ago

Yeah, pretty much. I think that needs to be settled before this can be made safe.

mahkoh commented 3 years ago

No it does not. The stdlib does not have to call from_raw_fd to create a File from an fd. Therefore any questions about from_raw_fd do not apply.

joshtriplett commented 3 years ago

Could you elaborate on why you're looking to create a std::fs::Metadata in particular, rather than, say, a libc::stat? If you're already dealing in raw file descriptors, it's easy to call stat directly.

mahkoh commented 3 years ago

I have no particular need for std::fs::Metadata. The issue is that the one-liner posted above is

If Metadata didn't have Unix and even Linux-specific extensions, then I would agree that this kind of thing should be delegated to external crates. But it does have such traits and they are not deprecated even though more powerful external crates have been available for a long time.