`Store::getFSAccessor`: Do not include the store dir by Ericson2314 · Pull Request #12531 · NixOS/nix

Ericson2314 · 2025-02-20T00:12:02Z

Motivation

Rather than "mounting" the store inside an empty virtual filesystem,
just return the store as a virtual filesystem. This is more modular.

Context

Depends on #12667

FWIW, it also supports two long term hopes of mind:

More capability-based Nix language mode. I dream of a "super pure
eval" where you can only use relative path literals (See no-absolute-path-literals experimental feature #8738), and
any fetchTree-fetched stuff + the store are all disjoint (none is
mounted in another) file systems.
Windows, where the store dir may include drive letters, etc., and is
thus unsuitable to be the prefix of any CanonPaths.

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2025-02-20T00:14:00Z

tests/functional/shell.sh


 # Test that symlinks outside of the store don't work.
-expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "is not in the Nix store"
+expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "points outside source accessor"


N.B. error message is quite arguably worse.

On the other hand, this error message should be something that can occur in other contexts for other reasons (e.g. symlinks escaping fetch-tree'd source), in which case having the same message to show the commonality is good.

The fullt expanded messages has all the relevant paths, so the user will still see /nix/store.

packaging/dev-shell.nix

edolstra

I don't really understand what ownLocationForSymlinkResolution is for. Can you give a concrete example where it's needed?

src/libexpr/eval.cc

src/libutil/subdir-source-accessor.cc

src/libutil/source-accessor.hh

src/libutil/subdir-source-accessor.cc

src/nix/cat.cc

tests/functional/shell.sh

edolstra · 2025-03-17T15:41:22Z

src/libutil/source-accessor.cc

+                           have host-OS-agnostic designs), we cannot simply
+                           use whether we are running on Unix or Windows for
+                           this. */
+                        if (!(hasPrefix(target, ownLocationForSymlinkResolution) && (target.size() == prefixLen || target[prefixLen] == '/'))) {


Should target be canonicalized before doing hasPrefix() on it? Otherwise a legal symlink target like /foo///bar won't match /foo/bar.

Also I think you can use isInDir() here.

Do we have code for a really weak canonicalization for this? We should not get rid of .. for example, we should just collapse adjacent /.

I guess I could write it, but it is tempting to factor out the CanonPath iterator that does this.

edolstra · 2025-03-19T19:41:44Z

src/libutil/source-accessor.hh

+     *
+     * (I made these examples use Windows/DOS paths to remind the reader
+     * that neither the symlink target nor
+     * `ownLocationForSymlinkResolution` should be a `CanonPath`.)


I'm not sure how this would work since our source accessor abstraction has no concept of drive letters. Its concept of a path is the CanonPath.

@edolstra (Just writing down some things we discussed on the call)

Its concept of a path is the CanonPath.

And that doesn't change. This is just about how we interact with the "external" paths in symlink targets, which we need to convert back to our own "internal" paths.

edolstra · 2025-03-19T19:45:39Z

src/libutil/source-accessor.hh

+     * to implement correctly that I (@Ericson2314) am not doing this at
+     * this time, however.
+     */
+    std::filesystem::path ownLocationForSymlinkResolution = "/";


I'm still not clear on why this is needed (given that we didn't need it prior to this PR). There may be an issue if you call resolveSymlinks on a projected store path accessor, but presumably we only call resolveSymlinks on storeFS (i.e. the MountedSourceAccessor that sees the entire store, and where we don't need to rewrite /nix/store).

As discussed on the call, without this there is a test failure because of /nix/store absolute paths in symlink targets. I split the history in two so that if one just tries the first commit it doesn't have this, and that test failure is reproduced.

dpulls · 2025-03-19T22:02:01Z

🎉 All dependencies have been resolved !

edolstra · 2025-03-26T20:12:40Z

@Ericson2314 I've made a different fix for the ownLocationForSymlinkResolution: f69bccf (this replaces the last commit of this PR). It makes nix shell use storeFS (which is the MountedSourceAccessor that wraps store->getFSAccessor()).

Having said that, I'm not sure whether it really makes sense to have Store::getFSAccessor() not include the store directory. Logically the store directory is essential, since absolute symlink resolution just doesn't work without it.

Ericson2314 · 2025-04-07T20:47:38Z

@edolstra, I am very much a fan of having a storeFS --- I was going to do that next after this --- but I am skeptical that solvesthe problem.

Having said that, I'm not sure whether it really makes sense to have Store::getFSAccessor() not include the store directory. Logically the store directory is essential, since absolute symlink resolution just doesn't work without it.

Bu that is what my commit you are skeptical of is for! Making things that don't "locally" include / while not breaking absolute symlink resolution.

Perhaps we should talk about these things wednesday, once we're good for Nix 2.28 in Nixpkgs.

Ericson2314 · 2025-04-09T21:25:39Z

OK we agreed to do it with @edolstra's second commit for now. I still want to do my second commit (and other changes too), but that can be a subsequent PR.

@edolstra will do one final review tomorrow.

Rather than "mounting" the store inside an empty virtual filesystem, just return the store as a virtual filesystem. This is more modular. (FWIW, it also supports two long term hopes of mind: 1. More capability-based Nix language mode. I dream of a "super pure eval" where you can only use relative path literals (See NixOS#8738), and any `fetchTree`-fetched stuff + the store are all disjoint (none is mounted in another) file systems. 2. Windows, where the store dir may include drive letters, etc., and is thus unsuitable to be the prefix of any `CanonPath`s. ) Co-authored-by: Eelco Dolstra <[email protected]>

`storeFS` is the `MountedSourceAccessor` that wraps `store->getFSAccessor()`.

Ericson2314 requested a review from edolstra as a code owner February 20, 2025 00:12

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Feb 20, 2025

Ericson2314 commented Feb 20, 2025

View reviewed changes

Ericson2314 force-pushed the store-accessor-root branch 2 times, most recently from e98041b to ca13970 Compare February 20, 2025 01:52

puffnfresh reviewed Feb 20, 2025

View reviewed changes

packaging/dev-shell.nix Outdated Show resolved Hide resolved

Ericson2314 force-pushed the store-accessor-root branch from ca13970 to ac1b340 Compare February 20, 2025 06:15

roberth reviewed Feb 20, 2025

View reviewed changes

packaging/dev-shell.nix Show resolved Hide resolved

Ericson2314 force-pushed the store-accessor-root branch from ac1b340 to c811b33 Compare February 20, 2025 18:04

edolstra self-assigned this Feb 26, 2025

Ericson2314 force-pushed the store-accessor-root branch from c811b33 to 46fbfea Compare March 17, 2025 04:18

edolstra reviewed Mar 17, 2025

View reviewed changes

Ericson2314 force-pushed the store-accessor-root branch 2 times, most recently from 27727a8 to ba3f481 Compare March 17, 2025 22:22

edolstra reviewed Mar 19, 2025

View reviewed changes

Ericson2314 force-pushed the store-accessor-root branch 2 times, most recently from 729a4dc to d5548a3 Compare March 19, 2025 21:46

Ericson2314 mentioned this pull request Apr 7, 2025

Source path error improvements #12915

Closed

Ericson2314 force-pushed the store-accessor-root branch from d5548a3 to 2d77c87 Compare April 9, 2025 21:25

Ericson2314 and others added 2 commits April 9, 2025 17:34

nix shell: Resolve symlinks in storeFS

9d35956

`storeFS` is the `MountedSourceAccessor` that wraps `store->getFSAccessor()`.

Ericson2314 force-pushed the store-accessor-root branch from 2d77c87 to 9d35956 Compare April 9, 2025 21:35

edolstra merged commit 26cb166 into NixOS:master Apr 10, 2025
12 checks passed

Ericson2314 deleted the store-accessor-root branch April 10, 2025 14:43

Ericson2314 mentioned this pull request Apr 10, 2025

FilteringSourceAccessor and SubdirSourceAccessor dedup #12996

Open

Uh oh!

Comments

Conversation

Ericson2314 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Ericson2314 Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edolstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edolstra Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

edolstra Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

edolstra Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

dpulls bot commented Mar 19, 2025

Uh oh!

edolstra commented Mar 26, 2025

Uh oh!

Ericson2314 commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ericson2314 commented Feb 20, 2025 •

edited

Loading

Ericson2314 commented Apr 7, 2025 •

edited

Loading

Ericson2314 commented Apr 9, 2025 •

edited

Loading