[android] Add support for android's file descriptor ownership tagging to libstd.

[jmgao]

Android has started investigating using Rust for system components, and one of the biggest historical sources of bugs we've had in cross-language interop has been caused by confusion around file descriptor ownership, since you're forced to pass file descriptors across language boundaries as a bare int without any type information about whether ownership is being transferred. The bugs this results in are horrible to debug, because they're nondeterministic and manifest as impossible behavior occurring on different threads. (e.g. [1], [2])

We've added some facilities to our libc to detect/prevent this class of bugs. Basically, user code can tell libc that a file descriptor can only be closed with a specified token, and to abort/log an error if someone tries to close the fd without the token (the behavior is process-wide and configurable at runtime, but the default on Android R (API level 30) and beyond is to abort). We've found dozens of bugs in Android with this, and hopefully prevented many more from ever being checked in.

This PR makes libstd use this on android when available. It potentially changes behavior, but only for code that's either broken, or very weird (e.g. File::from_raw_fd(fd).into_raw_fd() sequences which construct a File that owns the fd, and then removes it from the File before drop), and also only for unsafe code, I believe.

(also, as another commit in this PR, I improved the error handling on non-android close. Let me know if I should split this out to a separate PR) (whoops, didn't actually build this commit: I'll do this as a followup)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nikomatsakis]

I left a comment about this on Zulip -- I'm not sure the proper procedure here, but I'm inclined to land this PR. Probably it should be tagged with relnotes though.

[nikomatsakis]

Thinking a bit about it, since it is a user-visible change, I think an FCP is probably appropriate. @jmgao I'm curious if you could give some examples of what kinds of bugs you expect to find with this change -- e.g., would it help to detect broken code from users? Only if they are using the from_raw_fd API?

[nikomatsakis]

Also, cc @rust-lang/libs -- this seems to lie somewhere on the boundary of "implementation detail" and "public facing API change", so maybe y'all want to weigh in?

[Amanieu]

Should into_raw_fd reset the tag to 0? I think it should since you're explicitly saying that the fd is no longer managed by Rust.

[jmgao]

Thinking a bit about it, since it is a user-visible change, I think an FCP is probably appropriate. @jmgao I'm curious if you could give some examples of what kinds of bugs you expect to find with this change -- e.g., would it help to detect broken code from users? Only if they are using the from_raw_fd API?

Yeah, it'll detect when user code incorrectly transfers ownership, either in (via from_raw_fd) or out (via as_raw_fd, either closing it manually in Rust, or more likely, IMO, returning it across an FFI boundary and calling close on the other side).

Here's an example of a bug in Android that's the latter scenario: an fd is owned by a Java ParcelFIleDescriptor, and some C++ code grabbed it and passed it directly to some other code that expected to own the fd.

Should into_raw_fd reset the tag to 0? I think it should since you're explicitly saying that the fd is no longer managed by Rust.

Oops, yes, I'll fix this shortly. Before merging this PR in general, I need to find somewhere to add some tests, but I'm not entirely clear on where they should live, since they're Android-only (and behavior will also depend on what version of Android it's running on). Any suggestions?

[Amanieu]

The documentation for as_raw_fd should be updated to say that behavior is undefined if the returned fd is closed even if mem::forget is used. It should mention Android's ownership tagging as an example and recommend the use of into_raw_fd instead.

The documentation for from_raw_fd should also state that the fd must not currently be owned by any other Rust fd wrapper (e.g. File). Again Android's fd tagging should be mentioned as an example of how this can cause issues in practice.

[pnkfelix]

discussed at today's T-compiler triage meeting.

The T-compiler members present at the meeting didn't have any strong opinions here. We agreed with @nikomatsakis that this warrants an FCP of some sort.

There was general consensus that this is, at its heart, a question of what the publicly specified API for Rust's File should be; the actual implementation code (which T-compiler maintains) is pretty trivial.

So we collectively decided at the meeting that while this should require an FCP, it should really solely require an FCP of the members of T-libs; T-compiler is going to opt out of participating in the FCP.

Thus, I am removing the T-compiler label, keeping the T-libs label, and I will go see if I can find a T-libs member to start up the FCP process for this PR.

[Amanieu]

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @KodrAus
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[jmgao]

The documentation for as_raw_fd should be updated to say that behavior is undefined if the returned fd is closed even if mem::forget is used. It should mention Android's ownership tagging as an example and recommend the use of into_raw_fd instead.

The documentation for from_raw_fd should also state that the fd must not currently be owned by any other Rust fd wrapper (e.g. File). Again Android's fd tagging should be mentioned as an example of how this can cause issues in practice.

Somewhat of a nitpicky question: is "undefined behavior" the proper phrase to use for this? It's definitely incorrect, but it can't lead to the compiler using the fact that you did that to make assumptions that transitively breaks code elsewhere. The phrasing I'm thinking of now is something along the lines of:

AsRawFd:

 This method does not pass ownership of the raw file descriptor to the caller. The descriptor is only guaranteed to be valid while the original object has not yet been destroyed.
+Do not attempt to take ownership of this file descriptor (e.g. by using `FromRawFd`). On Android API level 30 and beyond, file descriptor ownership is enforced, which will lead to the process aborting if this occurs.

FromRawFd:

 This function consumes ownership of the specified file descriptor. The returned object will take responsibility for closing it when the object goes out of scope.
+The file descriptor consumed must not be already owned by another object. On Android API level 30 and beyond, file descriptor ownership is enforced, which will lead to the process aborting if this occurs.

[Amanieu]

Undefined behavior doesn't have to be linked to the compiler, it can come from the library as well: in this case it means that behavior may change in the future and may lead to unsoundness via unsafe assumptions (i.e. writing to the wrong FD).

I think that undefined behavior is appropriate in this case: Android happens to catch these cases, but other platforms do not.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[nikomatsakis]

Hey @rust-lang/libs, I see that the FCP has completed, but I think this concern raised by @the8472 merits an official response:

That would conflict with temporarily constructing a wrapper to use standard IO APIs from another wrapper type which doesn't support those APIs.

My impression is that we don't have a workaround or alternative for this pattern, so I'm wondering whether you all consider this a blocking concern to be resolved.

[m-ou-se]

Thanks for the ping @nikomatsakis. I've added it to the agenda for the libs meeting, which starts in a few hours.

[m-ou-se]

A few of the places that use ManuallyDrop<File> or ManuallyDrop<FileDesc> or ManuallyDrop<TcpStream> (etc.) as a stand in for a 'borrowed file descriptor':

In std:

• rust/library/std/src/sys/unix/stdio.rs

  Line 17 in 7d747db

  ManuallyDrop::new(FileDesc::new(libc::STDIN_FILENO)).read(buf)

  (And five more in that file.)

• rust/library/std/src/sys/unix/kernel_copy.rs

  Line 423 in 7d747db

  let file: ManuallyDrop<File> = ManuallyDrop::new(unsafe { File::from_raw_fd(fd) });

In wasmtime:

• https://github.com/bytecodealliance/wasmtime/blob/3d606a01e50517de50adab7d1161c4057fb20aed/crates/wasi-common/src/sys/unix/mod.rs#L46-L51

In other crates:

• https://github.com/CleverCloud/amq-protocol/blob/a055e71bf51efd0f417308863bd5817fcdcae268/tcp/src/lib.rs#L64-L65
• https://github.com/siiptuo/pio/blob/28be2ae2857dc11b45a8292a81d84aa78f64f436/src/output.rs#L74
• https://github.com/YorickPeterse/inko/blob/267d09e38b1e5064b9c4638f1d0e639220e31c51/vm/src/closable.rs#L105

And a few places that use File::from_raw_fd for file descriptors that are not strictly 'owned':

• In deno: https://github.com/denoland/deno/blob/26639b3bac463768c65f7fc40a1c53317549e1eb/cli/ops/io.rs#L46
• In alacritty: https://github.com/alacritty/alacritty/blob/9724418d350df881afe8453bde6eb88643e7211e/alacritty_terminal/src/tty/unix.rs#L182-L184
• In capnproto-rust: https://github.com/capnproto/capnproto-rust/blob/aafaac6b368fd186ecc32f21ddccd0b651138a31/benchmark/benchmark.rs#L248-L249

There's probably more, but it's a bit hard to search for.

[m-ou-se]

It seems to me that because of

rust/library/std/src/sys/unix/stdio.rs

Lines 36 to 39 in 7d747db

	impl io::Write for Stdout {
	fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
	ManuallyDrop::new(FileDesc::new(libc::STDOUT_FILENO)).write(buf)
	}

even a simple println!("hello"); println!("world"); would already break, as it calls write twice, which would end up tagging stdout twice. Am I missing something?

[jmgao]

My impression is that we don't have a workaround or alternative for this pattern, so I'm wondering whether you all consider this a blocking concern to be resolved.

IMO this is definitely a blocking concern. I think the behavior proposed by @the8472 might be the best option:

Perhaps from_raw_fd should be changed to only set a tag if none is present and otherweise store tag: 0 in FileDesc and then panic on drop if it is 0. That would force the use of ManuallyDrop for such temporary conversions.

[KodrAus]

@rfcbot cancel

[rfcbot]

@KodrAus proposal cancelled.

[the8472]

This API is also new. Since CI is setup to only test against a fairly old minimum API level this change won't be exercised in CI and thus it won't catch broken uses.

[crlf0710]

@jmgao Ping from triage! What's the next steps here?

[crlf0710]

@jmgao Ping from triage, i'm closing this due to inactivity. Feel free to reopen or create a new PR when you have time to work on this again. Thanks!

[workingjubilee]

With the recent IO safety concept being introduced into std, perhaps this has a reasonable route forward again?

[apiraino]

@rustbot label -to-announce

[apiraino]

@rustbot label +to-announce