treewide: Replace PosixSourceAccessor::createAtRoot with makeFSSourceAccessor #14809
treewide: Replace PosixSourceAccessor::createAtRoot with makeFSSourceAccessor #14809
Conversation
We'd like to split out the implementation into Unix/Windows-specific parts to more easily iterate on improving UNIX accessors to make use of dirfd-based operations (or even openat2). This should be hidden behind the appropriate interface and not exposed as a static member function of the PosixSourceAccessor.
Should be pretty self-explanatory. We didn't really have unit tests for the filesystem source accessor. Now we do and this will be immensely useful for implementing a unix-only smarter accessor that doesn't suffer from TOCTOU on symlinks.
| * | ||
| * The `PosixSourceAccessor` is rooted as far up the tree as | ||
| * possible, (e.g. on Windows it could scoped to a drive like | ||
| * `C:\`). This allows more `..` parent accessing to work. |
There was a problem hiding this comment.
None of the call-sites actually relied on the .. behaviour it looks like. dumpPath only traverses down - not upwards.
| * @note A canonicalizing behavior is not built in `createAtRoot` so that | ||
| * callers do not accidentally introduce symlink-related security vulnerabilities. |
There was a problem hiding this comment.
Also considering just how racy the PosixSourceAccessor is due to the lstat cache this is pretty much just a farce anyway. We'll be redoing this with fd-based accessor. The correct approach would be to use openFileEnsureBeneathNoSymlinks for this path and that will do the right thing.
|
There is one use-case of For everything else, yes please let's use |
|
Can you make the second commit its own PR? I would like to land that, and per the above to |
Not easily done for now because that would mean that we'd start following symlinks in quite a few places. I've tried my best o not change the semantics of symlink following for the UNIX case (also for mingw we don't care about symlinks). |
But I don't think its on master for now? Anyway, I'd really like to hide PosixSourceAccessor from the header to make progress on this. |
2 participants
Motivation
We'd like to split out the implementation into Unix/Windows-specific
parts to more easily iterate on improving UNIX accessors to make use
of dirfd-based operations (or even openat2). This should be hidden behind
the appropriate interface and not exposed as a static member function of
the
PosixSourceAccessor.Tests should be pretty self-explanatory. We didn't really have unit tests
for the filesystem source accessor. Now we do and this will be immensely
useful for implementing a unix-only smarter accessor that doesn't suffer
from TOCTOU on symlinks.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.