SourceAccessor: Allow the lstat cache to be invalidated#14957
Conversation
|
Not sure we should make such a deficiency a part of the object model of the SourceAccessor. The stat cache is sadly very TOCTOU prone. If anything, we should cache parent directory file descriptors — that wouldn't be such a footgun and wouldn't need explicit invalidation. |
|
How would that work? It's not the directory that's being invalidated here. |
|
Just never do stat caching. It's only hot at the moment because of the symlink check and that can be easily remedied by either using openat2 with RESOLVE_NO_SYMLINKS to ban symlink traversal or by doing one-path-component at a time traversal with O_NOFOLLOW. That would solve the primary issue (of not allowing symlink traversal) in a pretty cheap and race-free manner. I have a PoC on the unix-source-accessor branch, but I still need to implement parent directory file descriptor caching to work better when openat2 is not available. |
|
What I'm really worried about at the moment is that the stat cache makes the TOCTOU window arbitrarily large. Suppose it happens that daemon (for whatever reason) does a Then the daemon does: |
Sure, that would be much nicer, but we can always remove this invalidation code when we get rid of the cache. |
|
Also true. Just pointing out that it's not the best approach in the long run. @Ericson2314 would love to hear your opinion on this too. |
After lockFlake() creates flake.lock in a PosixSourceAccessor, it needs to be able to invalidate the lstat cache.
Ericson2314
left a comment
There was a problem hiding this comment.
I am OK doing this on a temporary basis, but once we have @xokdvium's DescriptorSourceAccessor we should revert it and delete the lstat cache. And we should do that soon!
0ef98fe to
139d05a
Compare
SourceAccessor: Allow the lstat cache to be invalidated
Motivation
After
lockFlake()creates aflake.lockin aPosixSourceAccessor, it needs to be able to invalidate the lstat cache, otherwise a subsequent attempt to readflake.lockwill fail with an erroneous "path does not exist" error.This isn't an issue yet on master, but it arises when we make the path fetcher lazy (DeterminateSystems#312).
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.