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..a23d4eb80 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -628,6 +628,9 @@ string_t AttrCursor::getStringWithContext() [&](const NixStringContextElem::Opaque & o) -> const StorePath & { return o.path; }, + [&](const NixStringContextElem::Path & p) -> const StorePath & { + abort(); // FIXME + }, }, c.raw); if (!root->state.store->isValidPath(path)) { valid = false; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 6505de7bc..d44ec53c4 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 @@ -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,9 @@ std::pair EvalState::coerceToSingleDerivedP [&](NixStringContextElem::Built && b) -> SingleDerivedPath { return std::move(b); }, + [&](NixStringContextElem::Path && p) -> SingleDerivedPath { + abort(); // FIXME + }, }, ((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..886a581bc 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 + }, }, c.raw); } @@ -1438,6 +1441,9 @@ static void derivationStrictInternal( [&](const NixStringContextElem::Opaque & o) { drv.inputSrcs.insert(state.devirtualize(o.path, &rewrites)); }, + [&](const NixStringContextElem::Path & p) { + // FIXME: do something + }, }, c.raw); } @@ -2346,10 +2352,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) { + 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(p->storePath)); + state.devirtualize(p->storePath, &rewrites); + } + } else state.error( "files created by %1% may not reference derivations, but %2% references %3%", @@ -2359,6 +2376,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..d8fd03737 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,10 @@ 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 { + // FIXME: don't know what to do here. + abort(); + }, }, context.begin()->raw) }), }; @@ -206,6 +223,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/value-to-json.cc b/src/libexpr/value-to-json.cc index 6230fa585..e4e29e2c5 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) { @@ -33,6 +34,8 @@ json printValueAsJSON(EvalState & state, bool strict, copyContext(v, context); // FIXME: only use the context from `v`. // FIXME: make devirtualization configurable? + // FIXME: don't devirtualize here? It's redundant if + // 'toFile' or 'derivation' also do it. out = state.devirtualize(v.c_str(), context); break; 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..568c54572 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 { + abort(); // FIXME + }, }, c.raw)); }