use openat when encountering ENAMETOOLONG #88731

the8472 · 2021-09-07T19:22:29Z

This adds a fallback to various fs methods that handles ENAMETOOLONG by splitting the path into relative segments and then opening each segment with openat(dirfd, segment, O_PATH).

Limitations:

not all fs methods are covered. the primary motivation is to get remove_dir_all working to delete a deep directory structure created by accident
currently linux-only. this could be extended to platforms which have either O_PATH or O_EXEC but there's no CI coverage for the BSDs, so I don't want to foist it on them.

fixes #53339

rust-highfive · 2021-09-07T19:22:32Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

library/std/src/sys/unix/fs.rs

lukaslueg · 2021-09-07T20:55:20Z

IMHO this is very close to unexpected magic behaviour. Essentially ENAMETOLONG is filtered, which may provide comfort, but is unexpected. There is also no way Ask-For-Forgiveness-Not-Permission, expecting and handling ENAMETOLONG in the caller. Maybe this should get a separate entry point.

the8472 · 2021-09-07T21:09:59Z

File names which are too long (>255 bytes on many filesystems) would still result in ENAMETOOLONG, i.e. the error is not filtered. This fallback only handles paths exceeding PATH_MAX (4096 bytes on linux).

Having a separate deep version for all the std::fs functions would require them to be platform-specific extension because some platforms lack the -at syscalls or O_PATH/O_EXEC. That would be quite unergonomic, especially for the convenience methods like fs::write.

m-ou-se · 2021-10-03T15:18:14Z

@joshtriplett, @cuviper, what's your opinion on this change?

joshtriplett · 2021-10-04T02:22:21Z

Why does this need O_PATH, rather than O_DIRECTORY? The latter is supported by every kernel version we support, and I think it'll work here.

the8472 · 2021-10-04T18:38:04Z

Already answered on zulip, but repeating it here for the record:

O_PATH is necessary on linux because open requires permission flags to be passed (this uses O_RDONLY) but that would fail when traversing exec/search-only directories. O_PATH circumvents that because it ignores the permission flags. Some other platforms have O_EXEC that could be used instead.

@cuviper josh suggested I check with you whether RHEL happened to backport O_PATH support since the minimum kernel version the only thing keeping us from using it by default for remove_dir_all.

cuviper · 2021-10-04T23:03:20Z

@cuviper josh suggested I check with you whether RHEL happened to backport O_PATH support since the minimum kernel version the only thing keeping us from using it by default for remove_dir_all.

No, that wasn't backported.

However, Linux doesn't check for invalid/unknown flags in open/openat -- this is how we used to get away with passing O_CLOEXEC even before we updated to a sufficient minimum kernel. So if we pass O_RDONLY | O_PATH, it should behave the same as O_RDONLY.

but that would fail when traversing exec/search-only directories.

So now we're talking about the failure of a new fallback, which means this is a corner case that didn't work before due to long paths, and will still fail (on old kernels) if it can't read. This seems acceptable to me, but should we let that EACCES through or return the original ENAMETOOLONG error?

the8472 · 2021-10-04T23:30:19Z

So if we pass O_RDONLY | O_PATH, it should behave the same as O_RDONLY.

That may be good enough to always use openat in remove_dir_all instead of the current approach + fallback for each file visited.

This seems acceptable to me, but should we let that EACCES through or return the original ENAMETOOLONG error?

I lean towards passing them through since on newer kernels that do support O_PATH it'll be a genuine traversal error. And a complete lack of permissions is probably more common than -r+x.

the8472 · 2021-10-10T22:45:53Z

let's see if this also builds on the other unixes

@bors try

bors · 2021-10-10T22:46:03Z

⌛ Trying commit c2ca4be16e5dc9c0bf006849c7b2f1d76c086806 with merge 8b572758695f4536fbe69894c98b5560ae8ff1cc...

bors · 2021-10-11T00:05:59Z

☀️ Try build successful - checks-actions
Build commit: 8b572758695f4536fbe69894c98b5560ae8ff1cc (8b572758695f4536fbe69894c98b5560ae8ff1cc)

the8472 · 2021-10-11T08:00:42Z

It appears to work. remove_dir_all now walks the directory tree incrementally via openat which allows it to reach arbitrary depths. It now uses a loop instead of recursion and only keeps a limited number of file descriptors open, so it won't run into stack or rlimits either.

Protip while fiddling with recursive deletion: Use a sandbox or have btrfs snapshots ready 😅

library/std/src/sys/unix/fs.rs

cuviper · 2021-10-12T18:55:22Z

let's see if this also builds on the other unixes

@bors try doesn't run full CI, just dist-x86_64-linux.

workingjubilee · 2023-07-31T21:54:12Z

#95925 was closed so this is de facto unblocked, I guess.

the8472 · 2023-07-31T22:06:30Z

Ah yes. Though this PR was written prior to the fix for CVE-2022-21658 (which inconveniently has no associated issue...) so I'll have to go over it again to make sure it wouldn't reintroduce that.

the8472 · 2023-07-31T22:06:40Z

@rustbot author

workingjubilee · 2023-07-31T22:13:16Z

Oh. 😬

rustbot · 2023-08-05T16:54:33Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2023-08-06T12:10:47Z

library/std/src/sys/unix/fs.rs

+#[cfg(all(miri, any(target_os = "linux")))]
+use libc::open64;


I'm a bit surprised that this import is only present for Miri -- that seems strange?

There are two code-paths, one that uses openat and one that uses open. Miri only supports the latter, so the entire machinery around -at syscalls is configured-out under miri and the alternative is configured-in.

Ah, and there are some other import for open64 below for other targets. The entire set of cfgs is just tricky to have an overview of. Makes sense.

RalfJung · 2023-08-06T12:19:39Z

FWIW the reason Miri needs special cases here as that the standard library doesn't expose openat-like functionality itself itself -- something like a way to open a file on a ReadDir (or some other representation of a "directory FD"). Without that we can't implement an openat shim. (Miri can run Linux targets on Windows hosts, which is quite valuable and means falling back to the host openat isn't really an option.)

This adds a fallback to various fs methods that handles ENAMETOOLONG by splitting the path into relative segments and then opening each segment with openat(dirfd, segment, O_PATH). Limitations: * not all fs methods are covered. the primary motivation is to get remove_dir_all working to delete a deep directory structure created by accident * requires O_PATH, so this can currently only be a fallback and not the default due to our current minimum kernel version. If you're not dealing with long paths it won't be used. * currently linux-only. this could be extended to platforms which have either O_PATH or O_EXEC but there's no CI coverage for the BSDs so I don't want to foist it on them * O(n²) performance if remove_dir_all has to use the fallback, albeit a small constant factor due to the long path segments used but ideally it should be rewritten to use openat in its recursion steps. But to do it properly we need a higher minimum kernel version.

it can remove arbitrarily-deep directory trees without exhausting the stack or file descriptor limits. The symlink attack/TOCTOU from CVE-2022-21658 that can occur when traversing the directory hierarchy more than level at a time is addressed by retracing the .. hierarchy after opening a descendant. Opening .. isn't subject to symlink attacks so we can reliably compare dev/ino numbers.

the8472 · 2023-08-14T10:02:07Z

FWIW the reason Miri needs special cases here as that the standard library doesn't expose openat-like functionality itself itself -- something like a way to open a file on a ReadDir

You could use cap-std for that which offers that functionality. Maybe it's not race-free on all platforms if they don't support the necessary primitives but I assume at least on the major unix-likes it would be using openat too.

But yeah, we should get the basics into std. Though that'll pose a compatibility question around windows since the necessary API there isn't officially stable.

bors · 2023-09-02T08:26:28Z

☔ The latest upstream changes (presumably #115469) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2023-12-17T22:14:40Z

@the8472
ping from triage - can you post your status on this PR? This PR has not received an update in a few months, and the PR itself is ... 2021 . Thank you!

the8472 · 2023-12-19T21:08:15Z

Closing for now. This will be more easily implemented on top of ACP 259

rust-highfive assigned m-ou-se Sep 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021

fbstj reviewed Sep 7, 2021

View reviewed changes

library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved

the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 8, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2021

m-ou-se assigned joshtriplett and unassigned m-ou-se Oct 3, 2021

m-ou-se added the O-linux Operating system: Linux label Oct 3, 2021

joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2021

the8472 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2021

This comment has been minimized.

Sign in to view

the8472 force-pushed the deep-open branch from bffaa03 to c2ca4be Compare October 10, 2021 21:50

cuviper reviewed Oct 12, 2021

View reviewed changes

library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023

the8472 force-pushed the deep-open branch from 36c80ba to f044f2d Compare August 5, 2023 10:21

This comment has been minimized.

Sign in to view

the8472 force-pushed the deep-open branch from f044f2d to f997b14 Compare August 5, 2023 11:35

This comment has been minimized.

Sign in to view

the8472 force-pushed the deep-open branch from eba3dca to f313e61 Compare August 5, 2023 16:54

This comment has been minimized.

Sign in to view

the8472 force-pushed the deep-open branch from f313e61 to 563b075 Compare August 5, 2023 17:00

RalfJung reviewed Aug 6, 2023

View reviewed changes

the8472 added 7 commits August 12, 2023 22:18

also pass mode flag to openat()

faab629

implement openat-based remove_dir_all in a separate module

923ac9f

implement long path fallback for stat() when statx isn't available

2b0bee2

disable long filename fallback under miri

a603b07

bless miri test

9aea939

the8472 force-pushed the deep-open branch from 563b075 to 689f3d0 Compare August 12, 2023 20:18

the8472 closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use openat when encountering ENAMETOOLONG #88731

use openat when encountering ENAMETOOLONG #88731

the8472 commented Sep 7, 2021 •

edited

Loading

rust-highfive commented Sep 7, 2021

lukaslueg commented Sep 7, 2021

the8472 commented Sep 7, 2021 •

edited

Loading

m-ou-se commented Oct 3, 2021

joshtriplett commented Oct 4, 2021

the8472 commented Oct 4, 2021

cuviper commented Oct 4, 2021

the8472 commented Oct 4, 2021

This comment has been minimized.

the8472 commented Oct 10, 2021

bors commented Oct 10, 2021

bors commented Oct 11, 2021

the8472 commented Oct 11, 2021

cuviper commented Oct 12, 2021

workingjubilee commented Jul 31, 2023

the8472 commented Jul 31, 2023

the8472 commented Jul 31, 2023

workingjubilee commented Jul 31, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 5, 2023

This comment has been minimized.

RalfJung Aug 6, 2023

the8472 Aug 6, 2023

RalfJung Aug 6, 2023

RalfJung commented Aug 6, 2023 •

edited

Loading

the8472 commented Aug 14, 2023

bors commented Sep 2, 2023

JohnCSimon commented Dec 17, 2023

the8472 commented Dec 19, 2023

		#[cfg(all(miri, any(target_os = "linux")))]
		use libc::open64;

use openat when encountering ENAMETOOLONG #88731

use openat when encountering ENAMETOOLONG #88731

Conversation

the8472 commented Sep 7, 2021 • edited Loading

rust-highfive commented Sep 7, 2021

lukaslueg commented Sep 7, 2021

the8472 commented Sep 7, 2021 • edited Loading

m-ou-se commented Oct 3, 2021

joshtriplett commented Oct 4, 2021

the8472 commented Oct 4, 2021

cuviper commented Oct 4, 2021

the8472 commented Oct 4, 2021

This comment has been minimized.

the8472 commented Oct 10, 2021

bors commented Oct 10, 2021

bors commented Oct 11, 2021

the8472 commented Oct 11, 2021

cuviper commented Oct 12, 2021

workingjubilee commented Jul 31, 2023

the8472 commented Jul 31, 2023

the8472 commented Jul 31, 2023

workingjubilee commented Jul 31, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 5, 2023

This comment has been minimized.

RalfJung Aug 6, 2023

Choose a reason for hiding this comment

the8472 Aug 6, 2023

Choose a reason for hiding this comment

RalfJung Aug 6, 2023

Choose a reason for hiding this comment

RalfJung commented Aug 6, 2023 • edited Loading

the8472 commented Aug 14, 2023

bors commented Sep 2, 2023

JohnCSimon commented Dec 17, 2023

the8472 commented Dec 19, 2023

the8472 commented Sep 7, 2021 •

edited

Loading

the8472 commented Sep 7, 2021 •

edited

Loading

RalfJung commented Aug 6, 2023 •

edited

Loading