Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builtins.path and builtins.filterSource use wrong filter paths with --store argument #11503

Open
Yarny0 opened this issue Sep 15, 2024 · 6 comments
Assignees
Labels

Comments

@Yarny0
Copy link

Yarny0 commented Sep 15, 2024

Describe the bug

The built-in functions builtins.path or builtins.filterSource both accept a root path and a filter function. The filter function is then called with subpaths of the root path as argument. Under certain circumstances, those subpaths are in fact not subpaths of the root path. As far as I could investigate, this happens if

  • the --store argument is used to point to another store that is not the "canonical" store ("canonical" usually is /nix/store),
  • the nix file that is evaluate and that contains the call to a built-in function is itself positioned in the "canonical" nix store,
  • the root directory points to ./., and
  • the derivation that contains the nix file is also present in the other nix store.

Note that these circumstances are usually given when you install NixOS from a live iso with nixos-install: Most nix files involved are contained in the nixos channel, which is somewhere in /nix/store. This channel is copied to /mnt/nix/store by nixos-install before anything else is done.

Steps To Reproduce

Put these two files in the same directory, then execute test.sh (this expects Nix to be installed and accessible, but no root privileges):

test.nix
let
  root = ./.;
  filter = path: type:
    let
      rootStr = builtins.toString ./.;
    in
      if builtins.substring 0 (builtins.stringLength rootStr) (builtins.toString path) == rootStr then true
      else builtins.throw "root path\n${rootStr}\nnot prefix of path\n${builtins.toString path}";
in

# each one will demonstrate the problem!:
#builtins.path { name="name"; path=root; inherit filter; }
builtins.filterSource filter root
test.sh
#! /usr/bin/env bash
tmpdir=$(mktemp -d)
mkdir $tmpdir/directory
cp test.nix $tmpdir/directory/default.nix
result=$(nix-store --add-fixed --recursive sha256 $tmpdir/directory)
nix-instantiate --eval $result
nix-instantiate --eval $result --store $tmpdir/2nd-store
nix-store --add-fixed --recursive sha256 $tmpdir/directory --store $tmpdir/2nd-store
echo this will fail...
nix-instantiate --eval $result --store $tmpdir/2nd-store

The final nix-instantiate will trigger the throw from text.nix -- apparently filterSource confuses the store paths:

error: root path
/nix/store/dv15ix385vx19wxac0v5h5n7x8i5iwgp-directory
not prefix of path
/tmp/tmp.f3xsGxSb42/2nd-store/nix/store/dv15ix385vx19wxac0v5h5n7x8i5iwgp-directory/default.nix

Expected behavior

The final nix-instantiate invocation should succeed.

nix-env --version output

nix-env (Nix) 2.18.5

Additional context

Nixpkgs contains many instances of src = lib.fileset.toSouce { root = ./.; fileset = ..something..; } which are liable to trigger this bug if evaluated from within nixos-install; in that case src ends up as empty directory because the filter function from the fileset library constantly returns false. One such instance is documented (with steps to reproduce with nixos-install) in NixOS/nixpkgs#334098 .

Priorities

Add 👍 to issues you find important.

@infinisil
Copy link
Member

Oh this actually sounds related to #9852

@Yarny0
Copy link
Author

Yarny0 commented Sep 16, 2024

Hmm, I think I dismissed that because it only occurs within the flake context, while the bug at hand is happening even without flakes. But now that I took a closer look: you're right, the symptoms are quite similar, so it might have the same root cause.

@edolstra
Copy link
Member

This is caused by this code in addPath():

        StorePathSet refs;

        if (path.accessor == state.rootFS && state.store->isInStore(path.path.abs())) {
            // FIXME: handle CA derivation outputs (where path needs to
            // be rewritten to the actual output).
            auto rewrites = state.realiseContext(context);
            path = {state.rootFS, CanonPath(state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context))};

            try {
                auto [storePath, subPath] = state.store->toStorePath(path.path.abs());
                // FIXME: we should scanForReferences on the path before adding it
                refs = state.store->queryPathInfo(storePath)->references;
                path = {state.rootFS, CanonPath(state.store->toRealPath(storePath) + subPath)};
            } catch (Error &) { // FIXME: should be InvalidPathError
            }
        }

This is there to handle IFD, where we have to rewrite the "logical" path resulting from the build (i.e. /nix/store/bla) to the physical path (i.e. /prefix/nix/store/bla) that the evaluator can use. But it shouldn't be applied to user-supplied paths.

A possible solution would be for paths resulting from derivations to use an accessor distinct from rootFS.

edolstra added a commit to DeterminateSystems/nix-src that referenced this issue Sep 18, 2024
infinisil referenced this issue in K900/nixpkgs Oct 5, 2024
This should be 0 rebuilds, except it fixes the installer tests, as
lib.filesets is somehow broken when combining chroot stores with
channel references (???).

Running nixosTests.installer.simple with this reverted will reproduce
the issue consistently.

CC @infinisil
K900 added a commit to K900/nixpkgs that referenced this issue Dec 30, 2024
This breaks the installer tests.

See NixOS/nix#11503 for more fileset-related nonsense.
emilazy added a commit to emilazy/nixpkgs that referenced this issue Dec 31, 2024
Due to Nix bug <NixOS/nix#11503>,
`builtins.filterSource` and chroot stores interact in a confusing
and broken way that breaks `lib.fileset`. This means that uses of
the API inside Nixpkgs keep breaking the NixOS installer, blocking
the channel. The resulting error messages are inscrutable (they look
like “the installer test is trying to download curl…?” and
eventually bottom out in a derivation that has the wrong `outPath`
because of the chroot store causing an incorrect `lib.fileset` result).

Whenever this happens, someone (well, in practice K900 or I)
has to bisect the change that introduced it and remove the use of
`lib.fileset`. This has happened at least three times in the past
four months (I believe I might actually be missing one here, but
these are the ones I remember and could easily dig up):

* <htxps://github.com/NixOS/pull/340046>
* <NixOS#352491>
* <NixOS#369459>

The options I see here are:

1. Forbid use of `lib.fileset` within Nixpkgs. This is the approach
   taken here. External users of Nixpkgs can continue to use the API as
   normal, but using it from within something that affects any release
   jobset `outPath`s will cause an evaluation failure with a hopefully‐
   helpful error message.

2. Forbid `lib.fileset` and also all of the other library APIs that use
   `builtins.filterSource`. I’m happy to do this, but so far none of
   those have broken the installer, so I decided to start small and
   worry about the others if they end up causing a problem in practice.

3. Forbid `builtins.filterSource` directly. This is hard and would
   require more invasive `builtins.scopedImport` crimes to do at
   evaluation time. I think this would realistically have to be done in
   something like nixpkgs-vet instead and I didn’t have much luck
   shoehorning a check like this into that codebase when I tried.

4. Fix the Nix bug. This would be great! But also it doesn’t seem to be
   happening any time soon, it seems difficult to fix in a way that
   doesn’t subtly break compatibility with the previous semantics, and
   arguably the fix would need backporting all the way back to 2.3
   given our minimum version policy.

5. Do nothing; have people continue to innocuously use `lib.fileset`
   throughout Nixpkgs, breaking the installer whenever one of them
   sneaks in to that closure, causing the channel to be blocked and
   requiring expensive bisections to narrow down the inscrutable test
   failure to the package using `lib.fileset`, which then needs moving
   back off it. This sucks for the people who keep having to track it
   down, holds back important channel bumps, and the criteria for when
   it’s okay to use `lib.fileset` are not realistically possible to
   teach to all contributors.

I'd be happy to work on (2) as an alternative; (3) would be difficult
and seems like overkill, (4) is not really something I trust myself
to do and wouldn’t address the immediate problem, and (5) isn’t
sustainable. I think that the current approach here is the best
trade‐off for now, as `lib.fileset` seems to be the only prominent
user of the `builtins.filterSource` API that works with full store
paths, exposing it to the Nix bug. It’s unfortunate to lose the
nice API, but since we can’t rely on it to produce correct results
and the channels keep getting blocked as a result, I don’t think
we really have an alternative right now.
emilazy added a commit to emilazy/nixpkgs that referenced this issue Dec 31, 2024
Due to Nix bug <NixOS/nix#11503>,
`builtins.filterSource` and chroot stores interact in a confusing
and broken way that breaks `lib.fileset`. This means that uses of
the API inside Nixpkgs keep breaking the NixOS installer, blocking
the channel. The resulting error messages are inscrutable (they look
like “the installer test is trying to download curl…?” and
eventually bottom out in a derivation that has the wrong `outPath`
because of the chroot store causing an incorrect `lib.fileset` result).

Whenever this happens, someone (well, in practice K900 or I)
has to bisect the change that introduced it and remove the use of
`lib.fileset`. This has happened at least three times in the past
four months (I believe I might actually be missing one here, but
these are the ones I remember and could easily dig up):

* <NixOS#340046>
* <NixOS#352491>
* <NixOS#369459>

The options I see here are:

1. Forbid use of `lib.fileset` within Nixpkgs. This is the approach
   taken here. External users of Nixpkgs can continue to use the API as
   normal, but using it from within something that affects any release
   jobset `outPath`s will cause an evaluation failure with a hopefully‐
   helpful error message.

2. Forbid `lib.fileset` and also all of the other library APIs that use
   `builtins.filterSource`. I’m happy to do this, but so far none of
   those have broken the installer, so I decided to start small and
   worry about the others if they end up causing a problem in practice.

3. Forbid `builtins.filterSource` directly. This is hard and would
   require more invasive `builtins.scopedImport` crimes to do at
   evaluation time. I think this would realistically have to be done in
   something like nixpkgs-vet instead and I didn’t have much luck
   shoehorning a check like this into that codebase when I tried.

4. Fix the Nix bug. This would be great! But also it doesn’t seem to be
   happening any time soon, it seems difficult to fix in a way that
   doesn’t subtly break compatibility with the previous semantics, and
   arguably the fix would need backporting all the way back to 2.3
   given our minimum version policy.

5. Do nothing; have people continue to innocuously use `lib.fileset`
   throughout Nixpkgs, breaking the installer whenever one of them
   sneaks in to that closure, causing the channel to be blocked and
   requiring expensive bisections to narrow down the inscrutable test
   failure to the package using `lib.fileset`, which then needs moving
   back off it. This sucks for the people who keep having to track it
   down, holds back important channel bumps, and the criteria for when
   it’s okay to use `lib.fileset` are not realistically possible to
   teach to all contributors.

I'd be happy to work on (2) as an alternative; (3) would be difficult
and seems like overkill, (4) is not really something I trust myself
to do and wouldn’t address the immediate problem, and (5) isn’t
sustainable. I think that the current approach here is the best
trade‐off for now, as `lib.fileset` seems to be the only prominent
user of the `builtins.filterSource` API that works with full store
paths, exposing it to the Nix bug. It’s unfortunate to lose the
nice API, but since we can’t rely on it to produce correct results
and the channels keep getting blocked as a result, I don’t think
we really have an alternative right now.
emilazy added a commit to emilazy/nixpkgs that referenced this issue Dec 31, 2024
Due to Nix bug <NixOS/nix#11503>,
`builtins.filterSource` and chroot stores interact in a confusing
and broken way that breaks `lib.fileset`. This means that uses of
the API inside Nixpkgs keep breaking the NixOS installer, blocking
the channel. The resulting error messages are inscrutable (they look
like “the installer test is trying to download `curl`…?” and
eventually bottom out in a derivation that has the wrong `outPath`
because of the chroot store causing an incorrect `lib.fileset` result).

Whenever this happens, someone (well, in practice K900 or I)
has to bisect the change that introduced it and remove the use of
`lib.fileset`. This has happened at least three times in the past
four months (I believe I might actually be missing one here, but
these are the ones I remember and could easily dig up):

* <NixOS#340046>
* <NixOS#352491>
* <NixOS#369459>

The options I see here are:

1. Forbid use of `lib.fileset` within Nixpkgs until the Nix bug is
   fixed. This is the approach taken here. External users of Nixpkgs
   can continue to use the API as normal, but using it from within
   something that affects any release jobset `outPath`s will cause an
   evaluation failure with a hopefully‐helpful error message.

2. Forbid `lib.fileset` and also all of the other library APIs that use
   `builtins.filterSource`. I’m happy to do this, but so far none of
   those have broken the installer, so I decided to start small and
   worry about the others if they end up causing a problem in practice.

3. Forbid `builtins.filterSource` directly. This is hard and would
   require more invasive `builtins.scopedImport` crimes to do at
   evaluation time. I think this would realistically have to be done in
   something like nixpkgs-vet instead and I didn’t have much luck
   shoehorning a check like this into that codebase when I tried.

4. Fix the Nix bug. This would be great! But also it doesn’t seem to be
   happening any time soon, it seems difficult to fix in a way that
   doesn’t subtly break compatibility with the previous semantics, and
   arguably the fix would need backporting all the way back to 2.3
   given our minimum version policy.

5. Do nothing; have people continue to innocuously use `lib.fileset`
   throughout Nixpkgs, breaking the installer whenever one of them
   sneaks in to that closure, causing the channel to be blocked and
   requiring expensive bisections to narrow down the inscrutable test
   failure to the package using `lib.fileset`, which then needs moving
   back off it. This sucks for the people who keep having to track it
   down, holds back important channel bumps, and the criteria for when
   it’s okay to use `lib.fileset` are not realistically possible to
   teach to all contributors.

I'd be happy to work on (2) as an alternative; (3) would be difficult
and seems like overkill, (4) is not really something I trust myself
to do and wouldn’t address the immediate problem, and (5) isn’t
sustainable. I think that the current approach here is the best
trade‐off for now, as `lib.fileset` seems to be the only prominent
user of the `builtins.filterSource` API that works with full store
paths, exposing it to the Nix bug. It’s unfortunate to lose the
nice API, but since we can’t rely on it to produce correct results
and the channels keep getting blocked as a result, I don’t think
we really have an alternative right now.
@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Dec 31, 2024

IMO this bug has reached the point of being pretty high severity for Nixpkgs.

IMO this bug should be a high priority for the Nix team.

@roberth roberth added this to Nix team Jan 1, 2025
@github-project-automation github-project-automation bot moved this to To triage in Nix team Jan 1, 2025
emilazy added a commit to NixOS/nixpkgs that referenced this issue Jan 1, 2025
Due to Nix bug <NixOS/nix#11503>,
`builtins.filterSource` and chroot stores interact in a confusing
and broken way that breaks `lib.fileset`. This means that uses of
the API inside Nixpkgs keep breaking the NixOS installer, blocking
the channel. The resulting error messages are inscrutable (they look
like “the installer test is trying to download `curl`…?” and
eventually bottom out in a derivation that has the wrong `outPath`
because of the chroot store causing an incorrect `lib.fileset` result).

Whenever this happens, someone (well, in practice K900 or I)
has to bisect the change that introduced it and remove the use of
`lib.fileset`. This has happened at least three times in the past
four months (I believe I might actually be missing one here, but
these are the ones I remember and could easily dig up):

* <#340046>
* <#352491>
* <#369459>

The options I see here are:

1. Forbid use of `lib.fileset` within Nixpkgs until the Nix bug is
   fixed. This is the approach taken here. External users of Nixpkgs
   can continue to use the API as normal, but using it from within
   something that affects any release jobset `outPath`s will cause an
   evaluation failure with a hopefully‐helpful error message.

2. Forbid `lib.fileset` and also all of the other library APIs that use
   `builtins.filterSource`. I’m happy to do this, but so far none of
   those have broken the installer, so I decided to start small and
   worry about the others if they end up causing a problem in practice.

3. Forbid `builtins.filterSource` directly. This is hard and would
   require more invasive `builtins.scopedImport` crimes to do at
   evaluation time. I think this would realistically have to be done in
   something like nixpkgs-vet instead and I didn’t have much luck
   shoehorning a check like this into that codebase when I tried.

4. Fix the Nix bug. This would be great! But also it doesn’t seem to be
   happening any time soon, it seems difficult to fix in a way that
   doesn’t subtly break compatibility with the previous semantics, and
   arguably the fix would need backporting all the way back to 2.3
   given our minimum version policy.

5. Do nothing; have people continue to innocuously use `lib.fileset`
   throughout Nixpkgs, breaking the installer whenever one of them
   sneaks in to that closure, causing the channel to be blocked and
   requiring expensive bisections to narrow down the inscrutable test
   failure to the package using `lib.fileset`, which then needs moving
   back off it. This sucks for the people who keep having to track it
   down, holds back important channel bumps, and the criteria for when
   it’s okay to use `lib.fileset` are not realistically possible to
   teach to all contributors.

I'd be happy to work on (2) as an alternative; (3) would be difficult
and seems like overkill, (4) is not really something I trust myself
to do and wouldn’t address the immediate problem, and (5) isn’t
sustainable. I think that the current approach here is the best
trade‐off for now, as `lib.fileset` seems to be the only prominent
user of the `builtins.filterSource` API that works with full store
paths, exposing it to the Nix bug. It’s unfortunate to lose the
nice API, but since we can’t rely on it to produce correct results
and the channels keep getting blocked as a result, I don’t think
we really have an alternative right now.

(cherry picked from commit 8725e46)
@Mic92
Copy link
Member

Mic92 commented Jan 7, 2025

Will be discussed in the next nix team meeting tomorrow.

@Mic92 Mic92 self-assigned this Jan 8, 2025
@Mic92 Mic92 moved this from To triage to ⚖ To discuss in Nix team Jan 8, 2025
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2025-01-08-nix-team-meeting-minutes-207/58523/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ⚖ To discuss
Development

No branches or pull requests

6 participants