stdlib incorrectly handles O_PATH

[cyphar]

There are several bugs here, but they all stem from the same problem -- the stdlib doesn't handle O_PATH correctly in a few places.

O_PATH requires you to set an extra (unused) mode.

O_PATH causes most other flags to be ignored, so requiring a mode is a little bit weird. Really, O_PATH should probably be handled as another OpenOptions mode.

let file = OpenOptions::new().custom_flags(libc::O_PATH).open(".")?; // gives EINVAL

This one can be worked around pretty trivially (though it is a bit silly in my view), but unfortunately that's when you hit the next issue:

Rust uses ioctl(FIOCLEX) to set close-on-exec.

This doesn't work with O_PATH because O_PATH file descriptors have empty_fops which means that all ioctl(2)s on them fail (this is a security feature).

Rust proactively sets close-on-exec in many different places, which means that any method that ends up triggering an FIOCLEX gives a spurrious EBADF. The most obvious problem is with File::try_clone() but I'm sure there are plenty of other examples:

let file = OpenOptions::new().read(true).custom_flags(libc::O_PATH).open(".")?;
let new_file = file.try_clone()?; // gives EBADF

Rust really should use fcntl(F_SETFD) because ioctl(2)s are blocked on all O_PATH descriptors (while fcntl works without issue).

[sfackler]

• Rust has full control over the openat(2) arguments, so really it should just set O_CLOEXEC in the argument list. This actually allows you to avoid the well-known race condition when trying to set O_CLOEXEC explicitly.

It does: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L460. There's a fallback to support older kernels that you're probably running into: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L469-L477

[cyphar]

I figured out the reason why O_CLOEXEC was missing -- it's because of a bug in a kernel patch I'm writing (which I'm simultaneously writing a Rust library for). So the fact that the "let's try setting O_CLOEXEC explicitly" path gets tripped is totally my fault.

However, I would argue Rust still really shouldn't be using ioctl(FIOCLEX) -- because it doesn't work for O_PATH (or any file with empty_fops) while fnctl(F_SETFD) always works. This would just boil down to adding target_os = "linux" here and removing it from the FIOCLEX impl.

[cyphar]

@sfackler

I just found another problem this causes -- File::try_clone will fail with EBADF on O_PATH files because FIOCLEX doesn't work on O_PATH file descriptors:

let file = OpenOptions::new().read(true).custom_flags(libc::O_PATH).open(".")?;
let new_file = file.try_clone()?; // gives EBADF

The patch to fix this issue is literally just:

From 34a7c65154e2b49d6a58ec5883a3ee9a40289ab7 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <[email protected]>
Date: Sat, 6 Jul 2019 00:51:19 +1000
Subject: [PATCH] filedesc: don't use FIOCLEX on Linux

All ioctl(2)s will fail on O_PATH file descriptors on Linux (because
they use &empty_fops as a security measure against O_PATH descriptors
affecting the backing file).

As a result, File::try_clone() and various other methods would always
fail with -EBADF on O_PATH file descriptors. The solution is to simply
use F_SETFD (as is used on other unices) which works on O_PATH
descriptors because it operates through the fnctl(2) layer and not
through ioctl(2)s.

Fixes: rust-lang/rust#62314
Signed-off-by: Aleksa Sarai <[email protected]>
---
 src/libstd/sys/unix/fd.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/libstd/sys/unix/fd.rs b/src/libstd/sys/unix/fd.rs
index 6d23963e141a..0cecdd7ffa0b 100644
--- a/src/libstd/sys/unix/fd.rs
+++ b/src/libstd/sys/unix/fd.rs
@@ -175,6 +175,7 @@ impl FileDesc {
                   target_os = "emscripten",
                   target_os = "fuchsia",
                   target_os = "l4re",
+                  target_os = "linux",
                   target_os = "haiku")))]
     pub fn set_cloexec(&self) -> io::Result<()> {
         unsafe {
@@ -187,6 +188,7 @@ impl FileDesc {
               target_os = "emscripten",
               target_os = "fuchsia",
               target_os = "l4re",
+              target_os = "linux",
               target_os = "haiku"))]
     pub fn set_cloexec(&self) -> io::Result<()> {
         unsafe {
-- 
2.22.0

I can submit a PR for this if that would be acceptable?

[sfackler]

Sure

[cyphar]

PR is here: #62425.

[rusty-snake]

I still do not see any fix for

O_PATH requires you to set an extra (unused) mode.

Should I open an new issue for it?

[cyphar]

@rusty-snake Yeah, I forgot to write a fix for that. I can open a new issue for that...

[rusty-snake]

Are you kidding me? Yesterday I looked at the function where I needed this again after a year. And today I get a notifications here. Do you want to claim that coincidence exists?

I can open a new issue for that...

SGTM