diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13d911518..539a90e5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -89,7 +89,15 @@ jobs: | ".#hydraJobs.tests." + .') flake_regressions: - if: github.event_name == 'merge_group' + if: | + github.event_name == 'merge_group' + || ( + github.event.pull_request.head.repo.full_name == 'DeterminateSystems/nix-src' + && ( + (github.event.action == 'labeled' && github.event.label.name == 'flake-regression-test') + || (github.event.action != 'labeled' && contains(github.event.pull_request.labels.*.name, 'flake-regression-test')) + ) + ) needs: build_x86_64-linux runs-on: namespace-profile-x86-32cpu-64gb steps: @@ -112,7 +120,15 @@ jobs: - run: nix build -L --out-link ./new-nix && PATH=$(pwd)/new-nix/bin:$PATH PARALLEL="-P 50%" flake-regressions/eval-all.sh flake_regressions_lazy: - if: github.event_name == 'merge_group' + if: | + github.event_name == 'merge_group' + || ( + github.event.pull_request.head.repo.full_name == 'DeterminateSystems/nix-src' + && ( + (github.event.action == 'labeled' && github.event.label.name == 'flake-regression-test') + || (github.event.action != 'labeled' && contains(github.event.pull_request.labels.*.name, 'flake-regression-test')) + ) + ) needs: build_x86_64-linux runs-on: namespace-profile-x86-32cpu-64gb steps: diff --git a/src/libexpr-test-support/include/nix/expr/tests/value/context.hh b/src/libexpr-test-support/include/nix/expr/tests/value/context.hh index a6a851d3a..a473f6f12 100644 --- a/src/libexpr-test-support/include/nix/expr/tests/value/context.hh +++ b/src/libexpr-test-support/include/nix/expr/tests/value/context.hh @@ -23,6 +23,11 @@ struct Arbitrary { static Gen arbitrary(); }; +template<> +struct Arbitrary { + static Gen arbitrary(); +}; + template<> struct Arbitrary { static Gen arbitrary(); diff --git a/src/libexpr-test-support/tests/value/context.cc b/src/libexpr-test-support/tests/value/context.cc index 51ff1b2ae..9a27f8730 100644 --- a/src/libexpr-test-support/tests/value/context.cc +++ b/src/libexpr-test-support/tests/value/context.cc @@ -15,6 +15,15 @@ Gen Arbitrary::arb }); } +Gen Arbitrary::arbitrary() +{ + return gen::map(gen::arbitrary(), [](StorePath storePath) { + return NixStringContextElem::Path{ + .storePath = storePath, + }; + }); +} + Gen Arbitrary::arbitrary() { return gen::mapcat( @@ -30,6 +39,9 @@ Gen Arbitrary::arbitrary() case 2: return gen::map( gen::arbitrary(), [](NixStringContextElem a) { return a; }); + case 3: + return gen::map( + gen::arbitrary(), [](NixStringContextElem a) { return a; }); default: assert(false); } diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 30aa6076a..4e44e68cf 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -618,18 +618,21 @@ string_t AttrCursor::getStringWithContext() if (auto s = std::get_if(&cachedValue->second)) { bool valid = true; for (auto & c : s->second) { - const StorePath & path = std::visit(overloaded { - [&](const NixStringContextElem::DrvDeep & d) -> const StorePath & { - return d.drvPath; + const StorePath * path = std::visit(overloaded { + [&](const NixStringContextElem::DrvDeep & d) -> const StorePath * { + return &d.drvPath; }, - [&](const NixStringContextElem::Built & b) -> const StorePath & { - return b.drvPath->getBaseStorePath(); + [&](const NixStringContextElem::Built & b) -> const StorePath * { + return &b.drvPath->getBaseStorePath(); }, - [&](const NixStringContextElem::Opaque & o) -> const StorePath & { - return o.path; + [&](const NixStringContextElem::Opaque & o) -> const StorePath * { + return &o.path; + }, + [&](const NixStringContextElem::Path & p) -> const StorePath * { + return nullptr; }, }, c.raw); - if (!root->state.store->isValidPath(path)) { + if (!path || !root->state.store->isValidPath(*path)) { valid = false; break; } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 6505de7bc..85c044c2f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -952,8 +952,8 @@ void EvalState::mkPos(Value & v, PosIdx p) // FIXME: only do this for virtual store paths? attrs.alloc(sFile).mkString(path->path.abs(), { - NixStringContextElem::Opaque{ - .path = store->toStorePath(path->path.abs()).first + NixStringContextElem::Path{ + .storePath = store->toStorePath(path->path.abs()).first } }); else @@ -2078,7 +2078,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) else if (firstType == nFloat) v.mkFloat(nf); else if (firstType == nPath) { - if (!context.empty()) + if (hasContext(context)) state.error("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow(); v.mkPath(state.rootPath(CanonPath(str()))); } else @@ -2277,7 +2277,10 @@ std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::s { auto s = forceString(v, pos, errorCtx); if (v.context()) { - error("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string_view(), v.context()[0]).withTrace(pos, errorCtx).debugThrow(); + NixStringContext context; + copyContext(v, context); + if (hasContext(context)) + error("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string_view(), v.context()[0]).withTrace(pos, errorCtx).debugThrow(); } return s; } @@ -2336,7 +2339,16 @@ BackedStringView EvalState::coerceToString( v.payload.path.path : copyToStore ? store->printStorePath(copyPathToStore(context, v.path())) - : std::string(v.path().path.abs()); + : ({ + auto path = v.path(); + if (path.accessor == rootFS && store->isInStore(path.path.abs())) { + context.insert( + NixStringContextElem::Path{ + .storePath = store->toStorePath(path.path.abs()).first + }); + } + std::string(path.path.abs()); + }); } if (v.type() == nAttrs) { @@ -2499,6 +2511,11 @@ std::pair EvalState::coerceToSingleDerivedP [&](NixStringContextElem::Built && b) -> SingleDerivedPath { return std::move(b); }, + [&](NixStringContextElem::Path && p) -> SingleDerivedPath { + error( + "string '%s' has no context", + s).withTrace(pos, errorCtx).debugThrow(); + }, }, ((NixStringContextElem &&) *context.begin()).raw); return { std::move(derivedPath), diff --git a/src/libexpr/include/nix/expr/value/context.hh b/src/libexpr/include/nix/expr/value/context.hh index f2de184ea..f53c9b997 100644 --- a/src/libexpr/include/nix/expr/value/context.hh +++ b/src/libexpr/include/nix/expr/value/context.hh @@ -54,10 +54,35 @@ struct NixStringContextElem { */ using Built = SingleDerivedPath::Built; + /** + * A store path that will not result in a store reference when + * used in a derivation or toFile. + * + * When you apply `builtins.toString` to a path value representing + * a path in the Nix store (as is the case with flake inputs), + * historically you got a string without context + * (e.g. `/nix/store/...-source`). This is broken, since it allows + * you to pass a store path to a derivation/toFile without a + * proper store reference. This is especially a problem with lazy + * trees, since the store path is a virtual path that doesn't + * exist. + * + * For backwards compatibility, and to warn users about this + * unsafe use of `toString`, we keep track of such strings as a + * special type of context. + */ + struct Path + { + StorePath storePath; + + GENERATE_CMP(Path, me->storePath); + }; + using Raw = std::variant< Opaque, DrvDeep, - Built + Built, + Path >; Raw raw; @@ -82,4 +107,10 @@ struct NixStringContextElem { typedef std::set NixStringContext; +/** + * Returns false if `context` has no elements other than + * `NixStringContextElem::Path`. + */ +bool hasContext(const NixStringContext & context); + } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7243f09ce..c6a97fdae 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -89,6 +89,9 @@ StringMap EvalState::realiseContext(const NixStringContext & context, StorePathS if (maybePathsOut) maybePathsOut->emplace(d.drvPath); }, + [&](const NixStringContextElem::Path & p) { + // FIXME: do something? + }, }, c.raw); } @@ -1414,6 +1417,8 @@ static void derivationStrictInternal( derivation. */ StringMap rewrites; + std::optional drvS; + for (auto & c : context) { std::visit(overloaded { /* Since this allows the builder to gain access to every @@ -1438,6 +1443,17 @@ static void derivationStrictInternal( [&](const NixStringContextElem::Opaque & o) { drv.inputSrcs.insert(state.devirtualize(o.path, &rewrites)); }, + [&](const NixStringContextElem::Path & p) { + if (!drvS) drvS = drv.unparse(*state.store, true); + if (drvS->find(p.storePath.to_string()) != drvS->npos) { + auto devirtualized = state.devirtualize(p.storePath, &rewrites); + warn( + "Using 'builtins.derivation' to create a derivation named '%s' that references the store path '%s' without a proper context. " + "The resulting derivation will not have a correct store reference, so this is unreliable and may stop working in the future.", + drvName, + state.store->printStorePath(devirtualized)); + } + }, }, c.raw); } @@ -2346,10 +2362,21 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val std::string contents(state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.toFile")); StorePathSet refs; + StringMap rewrites; for (auto c : context) { if (auto p = std::get_if(&c.raw)) refs.insert(p->path); + else if (auto p = std::get_if(&c.raw)) { + if (contents.find(p->storePath.to_string()) != contents.npos) { + auto devirtualized = state.devirtualize(p->storePath, &rewrites); + warn( + "Using 'builtins.toFile' to create a file named '%s' that references the store path '%s' without a proper context. " + "The resulting file will not have a correct store reference, so this is unreliable and may stop working in the future.", + name, + state.store->printStorePath(devirtualized)); + } + } else state.error( "files created by %1% may not reference derivations, but %2% references %3%", @@ -2359,6 +2386,8 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val ).atPos(pos).debugThrow(); } + contents = rewriteStrings(contents, rewrites); + auto storePath = settings.readOnlyMode ? state.store->makeFixedOutputPathFromCA(name, TextInfo { .hash = hashString(HashAlgorithm::SHA256, contents), diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index 6a7284e05..28153c778 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -7,9 +7,15 @@ namespace nix { static void prim_unsafeDiscardStringContext(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - NixStringContext context; + NixStringContext context, filtered; + auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.unsafeDiscardStringContext"); - v.mkString(*s); + + for (auto & c : context) + if (auto * p = std::get_if(&c.raw)) + filtered.insert(*p); + + v.mkString(*s, filtered); } static RegisterPrimOp primop_unsafeDiscardStringContext({ @@ -21,12 +27,19 @@ static RegisterPrimOp primop_unsafeDiscardStringContext({ .fun = prim_unsafeDiscardStringContext, }); +bool hasContext(const NixStringContext & context) +{ + for (auto & c : context) + if (!std::get_if(&c.raw)) + return true; + return false; +} static void prim_hasContext(EvalState & state, const PosIdx pos, Value * * args, Value & v) { NixStringContext context; state.forceString(*args[0], context, pos, "while evaluating the argument passed to builtins.hasContext"); - v.mkBool(!context.empty()); + v.mkBool(hasContext(context)); } static RegisterPrimOp primop_hasContext({ @@ -103,7 +116,7 @@ static void prim_addDrvOutputDependencies(EvalState & state, const PosIdx pos, V NixStringContext context; auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.addDrvOutputDependencies"); - auto contextSize = context.size(); + auto contextSize = context.size(); if (contextSize != 1) { state.error( "context of string '%s' must have exactly one element, but has %d", @@ -136,6 +149,11 @@ static void prim_addDrvOutputDependencies(EvalState & state, const PosIdx pos, V above does not make much sense. */ return std::move(c); }, + [&](const NixStringContextElem::Path & p) -> NixStringContextElem::DrvDeep { + state.error( + "`addDrvOutputDependencies` does not work on a string without context" + ).atPos(pos).debugThrow(); + }, }, context.begin()->raw) }), }; @@ -206,6 +224,8 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, [&](NixStringContextElem::Opaque && o) { contextInfos[std::move(o.path)].path = true; }, + [&](NixStringContextElem::Path && p) { + }, }, ((NixStringContextElem &&) i).raw); } diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 06bae9c5c..2badbb1bb 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -249,7 +249,11 @@ private: void printString(Value & v) { - printLiteralString(output, v.string_view(), options.maxStringLength, options.ansiColors); + NixStringContext context; + copyContext(v, context); + std::ostringstream s; + printLiteralString(s, v.string_view(), options.maxStringLength, options.ansiColors); + output << state.devirtualize(s.str(), context); } void printPath(Value & v) diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 6230fa585..a50687f37 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -7,9 +7,10 @@ #include #include - namespace nix { + using json = nlohmann::json; + json printValueAsJSON(EvalState & state, bool strict, Value & v, const PosIdx pos, NixStringContext & context, bool copyToStore) { @@ -31,9 +32,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nString: copyContext(v, context); - // FIXME: only use the context from `v`. - // FIXME: make devirtualization configurable? - out = state.devirtualize(v.c_str(), context); + out = v.c_str(); break; case nPath: diff --git a/src/libexpr/value/context.cc b/src/libexpr/value/context.cc index 40d08da59..cb3e6b691 100644 --- a/src/libexpr/value/context.cc +++ b/src/libexpr/value/context.cc @@ -57,6 +57,11 @@ NixStringContextElem NixStringContextElem::parse( .drvPath = StorePath { s.substr(1) }, }; } + case '@': { + return NixStringContextElem::Path { + .storePath = StorePath { s.substr(1) }, + }; + } default: { // Ensure no '!' if (s.find("!") != std::string_view::npos) { @@ -100,6 +105,10 @@ std::string NixStringContextElem::to_string() const res += '='; res += d.drvPath.to_string(); }, + [&](const NixStringContextElem::Path & p) { + res += '@'; + res += p.storePath.to_string(); + }, }, raw); return res; diff --git a/src/nix/app.cc b/src/nix/app.cc index 75ef874ba..0ba231c41 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -92,6 +92,9 @@ UnresolvedApp InstallableValue::toApp(EvalState & state) .path = o.path, }; }, + [&](const NixStringContextElem::Path & p) -> DerivedPath { + throw Error("'program' attribute of an 'app' output cannot have no context"); + }, }, c.raw)); } diff --git a/src/nix/eval.cc b/src/nix/eval.cc index d03d09916..bd58ba010 100644 --- a/src/nix/eval.cc +++ b/src/nix/eval.cc @@ -122,7 +122,10 @@ struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption } else if (json) { - logger->cout("%s", printValueAsJSON(*state, true, *v, pos, context, false)); + logger->cout("%s", + state->devirtualize( + printValueAsJSON(*state, true, *v, pos, context, false).dump(), + context)); } else {