From 44255c316ded5f543a32649b50c6b130da68c972 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 01/10] libutil: remove SourcePath::getPhysicalPath all files are physical, so this doesn't have to be optional. if it only returns a copy of a member it's not useful either, but performance cost Change-Id: Ib2f935ae247d96418d55bc100e04765dc586528b --- lix/legacy/nix-instantiate.cc | 5 +---- lix/libcmd/editor-for.cc | 5 +---- lix/libutil/source-path.hh | 7 ------- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/lix/legacy/nix-instantiate.cc b/lix/legacy/nix-instantiate.cc index 201de62e4..6127ce27b 100644 --- a/lix/legacy/nix-instantiate.cc +++ b/lix/legacy/nix-instantiate.cc @@ -168,10 +168,7 @@ static int main_nix_instantiate(std::string programName, Strings argv) if (findFile) { for (auto & i : files) { auto p = evaluator->paths.findFile(i); - if (auto fn = p.getPhysicalPath()) - std::cout << fn->abs() << std::endl; - else - throw Error("'%s' has no physical path", p); + std::cout << p.path.abs() << std::endl; } return 0; } diff --git a/lix/libcmd/editor-for.cc b/lix/libcmd/editor-for.cc index fc0f13b45..5eecd27d2 100644 --- a/lix/libcmd/editor-for.cc +++ b/lix/libcmd/editor-for.cc @@ -7,9 +7,6 @@ namespace nix { Strings editorFor(const SourcePath & file, uint32_t line) { - auto path = file.getPhysicalPath(); - if (!path) - throw Error("cannot open '%s' in an editor because it has no physical path", file); auto editor = getEnv("EDITOR").value_or("cat"); auto args = tokenizeString(editor); if (line > 0 && ( @@ -18,7 +15,7 @@ Strings editorFor(const SourcePath & file, uint32_t line) editor.find("vim") != std::string::npos || editor.find("kak") != std::string::npos)) args.push_back(fmt("+%d", line)); - args.push_back(path->abs()); + args.push_back(file.path.abs()); return args; } diff --git a/lix/libutil/source-path.hh b/lix/libutil/source-path.hh index f64fd641f..1c9868b08 100644 --- a/lix/libutil/source-path.hh +++ b/lix/libutil/source-path.hh @@ -103,13 +103,6 @@ struct SourcePath PathFilter & filter = defaultPathFilter) const { sink << nix::dumpPath(path.abs(), filter); } - /** - * Return the location of this path in the "real" filesystem, if - * it has a physical location. - */ - std::optional getPhysicalPath() const - { return path; } - std::string to_string() const { return path.abs(); } From 3acba7951aa6597b789423233ad2b28f31ee83fb Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 02/10] libexpr: use StorePath::to_string in string contexts `path.abs()` does the same thing, but `to_string` communicates intent as well. Change-Id: I9619a9f2e32317f0a1cc8e092ce8898471b980ac --- lix/legacy/nix-env.cc | 2 +- lix/libexpr/eval.cc | 4 ++-- lix/libexpr/primops.cc | 2 +- lix/libexpr/value-to-json.cc | 2 +- lix/libexpr/value-to-xml.cc | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 3862288f4..97b59a7ab 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -138,7 +138,7 @@ static void getAllExprs(Evaluator & state, attrName = std::string(attrName, 0, attrName.size() - 4); if (!seen.insert(attrName).second) { std::string suggestionMessage = ""; - if (path2.path.abs().find("channels") != std::string::npos && path.path.abs().find("channels") != std::string::npos) + if (path2.to_string().find("channels") != std::string::npos && path.to_string().find("channels") != std::string::npos) suggestionMessage = fmt("\nsuggestion: remove '%s' from either the root channels or the user channels", attrName); printError("warning: name collision in input Nix expressions, skipping '%1%'" "%2%", path2, suggestionMessage); diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 6f52566a3..dae7daafc 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -805,7 +805,7 @@ void EvalState::mkPos(Value & v, PosIdx p) auto origin = ctx.positions.originOf(p); if (auto path = std::get_if(&origin)) { auto attrs = ctx.buildBindings(3); - attrs.alloc(ctx.s.file).mkString(path->path.abs()); + attrs.alloc(ctx.s.file).mkString(path->to_string()); makePositionThunks(*this, p, attrs.alloc(ctx.s.line), attrs.alloc(ctx.s.column)); v.mkAttrs(attrs); } else @@ -2269,7 +2269,7 @@ BackedStringView EvalState::coerceToString( v._path : copyToStore ? ctx.store->printStorePath(ctx.paths.copyPathToStore(context, v.path(), ctx.repair)) - : std::string(v.path().path.abs()); + : v.path().to_string(); } if (v.type() == nAttrs) { diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 328ce250b..d88a5525a 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -1153,7 +1153,7 @@ static void prim_toPath(EvalState & state, const PosIdx pos, Value * * args, Val { NixStringContext context; auto path = state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to builtins.toPath"); - v.mkString(path.path.abs(), context); + v.mkString(path.to_string(), context); } /* Allow a valid store path to be used in an expression. This is diff --git a/lix/libexpr/value-to-json.cc b/lix/libexpr/value-to-json.cc index 4a2ad7062..0ba517604 100644 --- a/lix/libexpr/value-to-json.cc +++ b/lix/libexpr/value-to-json.cc @@ -39,7 +39,7 @@ json printValueAsJSON(EvalState & state, bool strict, out = state.ctx.store->printStorePath( state.ctx.paths.copyPathToStore(context, v.path(), state.ctx.repair)); else - out = v.path().path.abs(); + out = v.path().to_string(); break; case nNull: diff --git a/lix/libexpr/value-to-xml.cc b/lix/libexpr/value-to-xml.cc index 17426b12e..49c21623f 100644 --- a/lix/libexpr/value-to-xml.cc +++ b/lix/libexpr/value-to-xml.cc @@ -22,7 +22,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, static void posToXML(EvalState & state, XMLAttrs & xmlAttrs, const Pos & pos) { if (auto path = std::get_if(&pos.origin)) - xmlAttrs["path"] = path->path.abs(); + xmlAttrs["path"] = path->to_string(); xmlAttrs["line"] = fmt("%1%", pos.line); xmlAttrs["column"] = fmt("%1%", pos.column); } From f93af1db1fbe8765d547d94b51fb68b2f40c3e8e Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 03/10] libutil: make SourcePath::path private use canonical() to get the disk path, to_string() to get the string form. Change-Id: I95bb6df53356f30290b487d1cca0aa2fb37249ed --- lix/legacy/nix-env.cc | 2 +- lix/legacy/nix-instantiate.cc | 2 +- lix/libcmd/editor-for.cc | 2 +- lix/libcmd/repl.cc | 2 +- lix/libexpr/attr-path.cc | 2 +- lix/libexpr/eval-cache.cc | 4 ++-- lix/libexpr/eval.cc | 12 ++++++------ lix/libexpr/parser/parser-impl1.inc.cc | 2 +- lix/libexpr/primops.cc | 20 ++++++++++---------- lix/libexpr/value.cc | 2 +- lix/libexpr/value.hh | 2 +- lix/libfetchers/fetch-to-store.cc | 4 ++-- lix/libutil/source-path.hh | 4 ++++ tests/unit/libcmd/args.cc | 8 ++++---- 14 files changed, 36 insertions(+), 32 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 97b59a7ab..079613cbb 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -146,7 +146,7 @@ static void getAllExprs(Evaluator & state, } /* Load the expression on demand. */ auto vArg = state.mem.allocValue(); - vArg->mkString(path2.path.abs()); + vArg->mkString(path2.canonical().abs()); if (seen.size() == maxAttrs) throw Error("too many Nix expressions in directory '%1%'", path); attrs.alloc(attrName).mkApp(&state.builtins.get("import"), vArg); diff --git a/lix/legacy/nix-instantiate.cc b/lix/legacy/nix-instantiate.cc index 6127ce27b..57653f8ab 100644 --- a/lix/legacy/nix-instantiate.cc +++ b/lix/legacy/nix-instantiate.cc @@ -168,7 +168,7 @@ static int main_nix_instantiate(std::string programName, Strings argv) if (findFile) { for (auto & i : files) { auto p = evaluator->paths.findFile(i); - std::cout << p.path.abs() << std::endl; + std::cout << p.canonical().abs() << std::endl; } return 0; } diff --git a/lix/libcmd/editor-for.cc b/lix/libcmd/editor-for.cc index 5eecd27d2..68ba5954a 100644 --- a/lix/libcmd/editor-for.cc +++ b/lix/libcmd/editor-for.cc @@ -15,7 +15,7 @@ Strings editorFor(const SourcePath & file, uint32_t line) editor.find("vim") != std::string::npos || editor.find("kak") != std::string::npos)) args.push_back(fmt("+%d", line)); - args.push_back(file.path.abs()); + args.push_back(file.canonical().abs()); return args; } diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index d1534a3d8..238565428 100644 --- a/lix/libcmd/repl.cc +++ b/lix/libcmd/repl.cc @@ -676,7 +676,7 @@ ProcessLineResult NixRepl::processLine(std::string line) // Reload right after exiting the editor if path is not in store // Store is immutable, so there could be no changes, so there's no need to reload - if (!evaluator.store->isInStore(path.resolveSymlinks().path.abs())) { + if (!evaluator.store->isInStore(path.resolveSymlinks().canonical().abs())) { state.resetFileCache(); reloadFiles(); } diff --git a/lix/libexpr/attr-path.cc b/lix/libexpr/attr-path.cc index 926e7c9fe..0aa62d355 100644 --- a/lix/libexpr/attr-path.cc +++ b/lix/libexpr/attr-path.cc @@ -178,7 +178,7 @@ std::pair findPackageFilename(EvalState & state, Value & v NixStringContext context; auto path = state.coerceToPath(noPos, *v2, context, "while evaluating the 'meta.position' attribute of a derivation"); - auto fn = path.path.abs(); + auto fn = path.canonical().abs(); auto fail = [fn]() { throw ParseError("cannot parse 'meta.position' attribute '%s'", fn); diff --git a/lix/libexpr/eval-cache.cc b/lix/libexpr/eval-cache.cc index 443177afd..2ca311dbf 100644 --- a/lix/libexpr/eval-cache.cc +++ b/lix/libexpr/eval-cache.cc @@ -439,8 +439,8 @@ Value & AttrCursor::forceValue(EvalState & state) cachedValue = {root->db->setString(getKey(), v.string.s, v.string.context), string_t{v.string.s, {}}}; else if (v.type() == nPath) { - auto path = v.path().path; - cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}}; + auto path = v.path().canonical().abs(); + cachedValue = {root->db->setString(getKey(), path), string_t{path, {}}}; } else if (v.type() == nBool) cachedValue = {root->db->setBool(getKey(), v.boolean), v.boolean}; diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index dae7daafc..4dee03c79 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -383,7 +383,7 @@ SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) { if (!allowedPaths) return path_; - auto i = resolvedPaths.find(path_.path.abs()); + auto i = resolvedPaths.find(path_.canonical().abs()); if (i != resolvedPaths.end()) return i->second; @@ -393,7 +393,7 @@ SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) * attacker can't append ../../... to a path that would be in allowedPaths * and thus leak symlink targets. */ - Path abspath = canonPath(path_.path.abs()); + Path abspath = canonPath(path_.canonical().abs()); if (abspath.starts_with(corepkgsPrefix)) return CanonPath(abspath); @@ -416,8 +416,8 @@ SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) SourcePath path = CanonPath(canonPath(abspath, true)); for (auto & i : *allowedPaths) { - if (isDirOrInDir(path.path.abs(), i)) { - resolvedPaths.insert_or_assign(path_.path.abs(), path); + if (isDirOrInDir(path.canonical().abs(), i)) { + resolvedPaths.insert_or_assign(path_.canonical().abs(), path); return path; } } @@ -2339,7 +2339,7 @@ BackedStringView EvalState::coerceToString( StorePath EvalPaths::copyPathToStore(NixStringContext & context, const SourcePath & path, RepairFlag repair) { - if (nix::isDerivation(path.path.abs())) + if (nix::isDerivation(path.canonical().abs())) errors.make("file names are not allowed to end in '%1%'", drvExtension).debugThrow(); auto i = srcToStore.find(path); @@ -2672,7 +2672,7 @@ SourcePath resolveExprPath(SourcePath path) if (++followCount >= maxFollow) throw Error("too many symbolic links encountered while traversing the path '%s'", path); if (path.lstat().type != InputAccessor::tSymlink) break; - path = {CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))}; + path = {CanonPath(path.readLink(), path.canonical().parent().value_or(CanonPath::root))}; } /* If `path' refers to a directory, append `/default.nix'. */ diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index efea9619d..7f8a9bb88 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/lix/libexpr/parser/parser-impl1.inc.cc @@ -605,7 +605,7 @@ template<> struct BuildAST : BuildAST struct BuildAST { static void apply(const auto & in, StringState & s, State & ps) { - Path path(absPath(in.string(), ps.basePath.path.abs())); + Path path(absPath(in.string(), ps.basePath.canonical().abs())); /* add back in the trailing '/' to the first segment */ if (in.string_view().ends_with('/') && in.size() > 1) path += "/"; diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index d88a5525a..4bf9a7fb9 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -131,7 +131,7 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, co try { StringMap rewrites = state.ctx.paths.realiseContext(context); - auto realPath = CanonPath(state.ctx.paths.toRealPath(rewriteStrings(path.path.abs(), rewrites), context)); + auto realPath = CanonPath(state.ctx.paths.toRealPath(rewriteStrings(path.canonical().abs(), rewrites), context)); return flags.checkForPureEval ? state.ctx.paths.checkSourcePath(realPath) @@ -176,7 +176,7 @@ static void mkOutputString( static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * vScope, Value & v) { auto path = realisePath(state, pos, vPath); - auto path2 = path.path.abs(); + auto path2 = path.canonical().abs(); // FIXME auto isValidDerivationInStore = [&]() -> std::optional { @@ -283,7 +283,7 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu std::string sym(state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.importNative")); - void *handle = dlopen(path.path.c_str(), RTLD_LAZY | RTLD_LOCAL); + void *handle = dlopen(path.canonical().c_str(), RTLD_LAZY | RTLD_LOCAL); if (!handle) state.ctx.errors.make("could not open '%1%': %2%", path, dlerror()).debugThrow(); @@ -1173,7 +1173,7 @@ static void prim_storePath(EvalState & state, const PosIdx pos, Value * * args, ).atPos(pos).debugThrow(); NixStringContext context; - auto path = state.ctx.paths.checkSourcePath(state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to builtins.storePath")).path; + auto path = state.ctx.paths.checkSourcePath(state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to builtins.storePath")).canonical(); /* Resolve symlinks in ‘path’, unless ‘path’ itself is a symlink directly in the store. The latter condition is necessary so e.g. nix-push does the right thing. */ @@ -1241,7 +1241,7 @@ static void prim_dirOf(EvalState & state, const PosIdx pos, Value * * args, Valu state.forceValue(*args[0], pos); if (args[0]->type() == nPath) { auto path = args[0]->path(); - v.mkPath(path.path.isRoot() ? path : path.parent()); + v.mkPath(path.canonical().isRoot() ? path : path.parent()); } else { NixStringContext context; auto path = state.coerceToString(pos, *args[0], context, @@ -1263,9 +1263,9 @@ static void prim_readFile(EvalState & state, const PosIdx pos, Value * * args, V path ).atPos(pos).debugThrow(); StorePathSet refs; - if (state.ctx.store->isInStore(path.path.abs())) { + if (state.ctx.store->isInStore(path.canonical().abs())) { try { - refs = state.ctx.store->queryPathInfo(state.ctx.store->toStorePath(path.path.abs()).first)->references; + refs = state.ctx.store->queryPathInfo(state.ctx.store->toStorePath(path.canonical().abs()).first)->references; } catch (Error &) { // FIXME: should be InvalidPathError } // Re-scan references to filter down to just the ones that actually occur in the file. @@ -1513,7 +1513,7 @@ static void addPath( path = evalSettings.pureEval && expectedHash ? path - : state.ctx.paths.checkSourcePath(CanonPath(path)).path.abs(); + : state.ctx.paths.checkSourcePath(CanonPath(path)).canonical().abs(); PathFilter filter = filterFun ? ([&](const Path & path) { auto st = lstat(path); @@ -1569,7 +1569,7 @@ static void prim_filterSource(EvalState & state, const PosIdx pos, Value * * arg auto path = state.coerceToPath(pos, *args[1], context, "while evaluating the second argument (the path to filter) passed to builtins.filterSource"); state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filterSource"); - addPath(state, pos, path.baseName(), path.path.abs(), args[0], FileIngestionMethod::Recursive, std::nullopt, v, context); + addPath(state, pos, path.baseName(), path.canonical().abs(), args[0], FileIngestionMethod::Recursive, std::nullopt, v, context); } static void prim_path(EvalState & state, const PosIdx pos, Value * * args, Value & v) @@ -1608,7 +1608,7 @@ static void prim_path(EvalState & state, const PosIdx pos, Value * * args, Value if (name.empty()) name = path->baseName(); - addPath(state, pos, name, path->path.abs(), filterFun, method, expectedHash, v, context); + addPath(state, pos, name, path->canonical().abs(), filterFun, method, expectedHash, v, context); } diff --git a/lix/libexpr/value.cc b/lix/libexpr/value.cc index ae62db3e8..5aea57096 100644 --- a/lix/libexpr/value.cc +++ b/lix/libexpr/value.cc @@ -100,7 +100,7 @@ void Value::mkStringMove(const char * s, const NixStringContext & context) void Value::mkPath(const SourcePath & path) { - mkPath(gcCopyStringIfNeeded(path.path.abs())); + *this = Value(NewValueAs::path, path); } } diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index b69049130..5d3a25695 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -347,7 +347,7 @@ public: /// dynamic (GC) allocation to do so. Value(path_t, SourcePath const & path) : internalType(tPath) - , _path(gcCopyStringIfNeeded(path.path.abs())) + , _path(gcCopyStringIfNeeded(path.canonical().abs())) , _path_pad(0) { } diff --git a/lix/libfetchers/fetch-to-store.cc b/lix/libfetchers/fetch-to-store.cc index 3fb53a437..ee7304338 100644 --- a/lix/libfetchers/fetch-to-store.cc +++ b/lix/libfetchers/fetch-to-store.cc @@ -18,8 +18,8 @@ StorePath fetchToStore( return settings.readOnlyMode - ? store.computeStorePathForPath(name, path.path.abs(), method, HashType::SHA256, filter2).first - : store.addToStore(name, path.path.abs(), method, HashType::SHA256, filter2, repair); + ? store.computeStorePathForPath(name, path.canonical().abs(), method, HashType::SHA256, filter2).first + : store.addToStore(name, path.canonical().abs(), method, HashType::SHA256, filter2, repair); } diff --git a/lix/libutil/source-path.hh b/lix/libutil/source-path.hh index 1c9868b08..1ad66e87f 100644 --- a/lix/libutil/source-path.hh +++ b/lix/libutil/source-path.hh @@ -42,8 +42,10 @@ enum class SymlinkResolution { */ struct SourcePath { +private: CanonPath path; +public: SourcePath(CanonPath path) : path(std::move(path)) { } @@ -103,6 +105,8 @@ struct SourcePath PathFilter & filter = defaultPathFilter) const { sink << nix::dumpPath(path.abs(), filter); } + const CanonPath & canonical() const { return path; } + std::string to_string() const { return path.abs(); } diff --git a/tests/unit/libcmd/args.cc b/tests/unit/libcmd/args.cc index 4283dd663..9ed41e28e 100644 --- a/tests/unit/libcmd/args.cc +++ b/tests/unit/libcmd/args.cc @@ -34,12 +34,12 @@ TEST(Arguments, lookupFileArg) { auto state = std::make_shared(searchPath, store, store); SourcePath const foundUnitData = lookupFileArg(*state, ""); - EXPECT_EQ(foundUnitData.path, canonDataPath); + EXPECT_EQ(foundUnitData.canonical(), canonDataPath); // lookupFileArg should not resolve if anything else is before or after it. SourcePath const yepEvenSpaces = lookupFileArg(*state, " "); - EXPECT_EQ(yepEvenSpaces.path, CanonPath::fromCwd(" ")); - EXPECT_EQ(lookupFileArg(*state, "/nixos").path, CanonPath::fromCwd("/nixos")); + EXPECT_EQ(yepEvenSpaces.canonical(), CanonPath::fromCwd(" ")); + EXPECT_EQ(lookupFileArg(*state, "/nixos").canonical(), CanonPath::fromCwd("/nixos")); try { lookupFileArg(*state, INVALID_CHANNEL); @@ -49,7 +49,7 @@ TEST(Arguments, lookupFileArg) { } SourcePath const normalFile = lookupFileArg(*state, unitDataPath); - EXPECT_EQ(normalFile.path, canonDataPath); + EXPECT_EQ(normalFile.canonical(), canonDataPath); } } From 5af069b248e7e47233d0317d1a6851d35176b905 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 04/10] libutil: remove SourcePath::resolveSymlinks in pure mode it is entirely useless. in impure mode it's mostly useless since the way in which it is used is either equivalent to not being run at all, or is equivalent to turning the following lstat into a stat. we add a stat method instead for all those who need final symlinks stat'd. Change-Id: I801886d18eb34b26e62b4c05d53318c6421a69bf --- lix/legacy/nix-env.cc | 4 +-- lix/libcmd/repl.cc | 2 +- lix/libexpr/primops.cc | 11 +++--- lix/libutil/source-path.cc | 70 ++++++++++++++------------------------ lix/libutil/source-path.hh | 42 +++++++---------------- 5 files changed, 47 insertions(+), 82 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 079613cbb..38664f63b 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -123,7 +123,7 @@ static void getAllExprs(Evaluator & state, InputAccessor::Stat st; try { - st = path2.resolveSymlinks().lstat(); + st = path2.stat(); } catch (Error &) { continue; // ignore dangling symlinks in ~/.nix-defexpr } @@ -162,7 +162,7 @@ static void getAllExprs(Evaluator & state, static void loadSourceExpr(EvalState & state, const SourcePath & path, Value & v) { - auto st = path.resolveSymlinks().lstat(); + auto st = path.stat(); if (isNixExpr(path, st)) state.evalFile(path, v); diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index 238565428..521d2898b 100644 --- a/lix/libcmd/repl.cc +++ b/lix/libcmd/repl.cc @@ -676,7 +676,7 @@ ProcessLineResult NixRepl::processLine(std::string line) // Reload right after exiting the editor if path is not in store // Store is immutable, so there could be no changes, so there's no need to reload - if (!evaluator.store->isInStore(path.resolveSymlinks().canonical().abs())) { + if (!evaluator.store->isInStore(canonPath(path.canonical().abs(), true))) { state.resetFileCache(); reloadFiles(); } diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 4bf9a7fb9..e2bddcd15 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -1206,12 +1206,13 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, || arg.str().ends_with("/.")); try { - auto checked = state - .ctx.paths - .checkSourcePath(path) - .resolveSymlinks(mustBeDir ? SymlinkResolution::Full : SymlinkResolution::Ancestors); + auto checked = state.ctx.paths.checkSourcePath(path); - auto st = checked.maybeLstat(); + // previously we fully resolved symlinks in the mustBeDir case or in pure eval + // mode (by accident, since checkSourcePath does this in that case), and up to + // the last component otherwise. this is equivalent to calling stat and lstat, + // respectively. (in neither case do intermediate symlinks affect the result.) + auto st = mustBeDir ? checked.maybeStat() : checked.maybeLstat(); auto exists = st && (!mustBeDir || st->type == InputAccessor::tDirectory); v.mkBool(exists); } catch (SysError & e) { diff --git a/lix/libutil/source-path.cc b/lix/libutil/source-path.cc index ce7c77b49..392794049 100644 --- a/lix/libutil/source-path.cc +++ b/lix/libutil/source-path.cc @@ -1,4 +1,5 @@ #include "lix/libutil/source-path.hh" +#include "file-system.hh" #include "lix/libutil/strings.hh" namespace nix { @@ -21,9 +22,8 @@ SourcePath SourcePath::parent() const return std::move(*p); } -InputAccessor::Stat SourcePath::lstat() const +static InputAccessor::Stat convertStat(const struct stat & st) { - auto st = nix::lstat(path.abs()); return InputAccessor::Stat { .type = S_ISREG(st.st_mode) ? InputAccessor::tRegular : @@ -34,12 +34,32 @@ InputAccessor::Stat SourcePath::lstat() const }; } +InputAccessor::Stat SourcePath::lstat() const +{ + return convertStat(nix::lstat(path.abs())); +} + std::optional SourcePath::maybeLstat() const { - // FIXME: merge these into one operation. - if (!pathExists()) - return {}; - return lstat(); + if (auto st = nix::maybeLstat(path.abs())) { + return convertStat(*st); + } else { + return std::nullopt; + } +} + +InputAccessor::Stat SourcePath::stat() const +{ + return convertStat(nix::stat(path.abs())); +} + +std::optional SourcePath::maybeStat() const +{ + if (auto st = nix::maybeStat(path.abs())) { + return convertStat(*st); + } else { + return std::nullopt; + } } InputAccessor::DirEntries SourcePath::readDirectory() const @@ -58,42 +78,4 @@ InputAccessor::DirEntries SourcePath::readDirectory() const return res; } -SourcePath SourcePath::resolveSymlinks(SymlinkResolution mode) const -{ - SourcePath res(CanonPath::root); - - int linksAllowed = 1024; - - std::list todo; - for (auto & c : path) - todo.push_back(std::string(c)); - - bool resolve_last = mode == SymlinkResolution::Full; - - while (!todo.empty()) { - auto c = *todo.begin(); - todo.pop_front(); - if (c == "" || c == ".") - ; - else if (c == "..") - res.path.pop(); - else { - res.path.push(c); - if (resolve_last || !todo.empty()) { - if (auto st = res.maybeLstat(); st && st->type == InputAccessor::tSymlink) { - if (!linksAllowed--) - throw Error("infinite symlink recursion in path '%s'", path); - auto target = res.readLink(); - res.path.pop(); - if (target.starts_with("/")) - res.path = CanonPath::root; - todo.splice(todo.begin(), tokenizeString>(target, "/")); - } - } - } - } - - return res; -} - } diff --git a/lix/libutil/source-path.hh b/lix/libutil/source-path.hh index 1ad66e87f..8267cf5e2 100644 --- a/lix/libutil/source-path.hh +++ b/lix/libutil/source-path.hh @@ -14,26 +14,6 @@ namespace nix { -/** - * Note there is a decent chance this type soon goes away because the problem is solved another way. - * See the discussion in https://github.com/NixOS/nix/pull/9985. - */ -enum class SymlinkResolution { - /** - * Resolve symlinks in the ancestors only. - * - * Only the last component of the result is possibly a symlink. - */ - Ancestors, - - /** - * Resolve symlinks fully, realpath(3)-style. - * - * No component of the result will be a symlink. - */ - Full, -}; - /** * An abstraction for accessing source files during * evaluation. Currently, it's just a wrapper around `CanonPath` that @@ -84,6 +64,18 @@ public: */ std::optional maybeLstat() const; + /** + * Return stats about this `SourcePath`, or throw an exception if + * it doesn't exist. Symlinks are resolved by this function. + */ + InputAccessor::Stat stat() const; + + /** + * Return stats about this `SourcePath`, or std::nullopt if it + * doesn't exist. Symlinks are resolved by this function. + */ + std::optional maybeStat() const; + /** * If this `SourcePath` denotes a directory (not a symlink), * return its directory entries; otherwise throw an error. @@ -138,16 +130,6 @@ public: { return path < x.path; } - - /** - * Resolve any symlinks in this `SourcePath` according to the - * given resolution mode. - * - * @param mode might only be a temporary solution for this. - * See the discussion in https://github.com/NixOS/nix/pull/9985. - */ - SourcePath resolveSymlinks( - SymlinkResolution mode = SymlinkResolution::Full) const; }; std::ostream & operator << (std::ostream & str, const SourcePath & path); From ede0851fb4fd31a6974c41900e4aa66778b5be58 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 05/10] libexpr: move resolveExprPath to EvalPaths Change-Id: I4f8e27bb816d6498df4d73a57e10b654eb995c32 --- lix/legacy/nix-build.cc | 8 ++++++-- lix/legacy/nix-instantiate.cc | 2 +- lix/libcmd/installables.cc | 6 +++--- lix/libexpr/eval.cc | 4 ++-- lix/libexpr/eval.hh | 10 +++++----- lix/libexpr/flake/flake.cc | 2 +- lix/libexpr/primops.cc | 2 +- lix/nix/prefetch.cc | 2 +- 8 files changed, 20 insertions(+), 16 deletions(-) diff --git a/lix/legacy/nix-build.cc b/lix/legacy/nix-build.cc index 0642a995b..dae707379 100644 --- a/lix/legacy/nix-build.cc +++ b/lix/legacy/nix-build.cc @@ -253,8 +253,12 @@ static void main_nix_build(std::string programName, Strings argv) else /* If we're in a #! script, interpret filenames relative to the script. */ - exprs.push_back(evaluator->parseExprFromFile(resolveExprPath(evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, - inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i))))); + exprs.push_back(evaluator->parseExprFromFile(evaluator->paths.resolveExprPath( + evaluator->paths.checkSourcePath(lookupFileArg( + *evaluator, + inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i + )) + ))); } } diff --git a/lix/legacy/nix-instantiate.cc b/lix/legacy/nix-instantiate.cc index 57653f8ab..688201f2f 100644 --- a/lix/legacy/nix-instantiate.cc +++ b/lix/legacy/nix-instantiate.cc @@ -183,7 +183,7 @@ static int main_nix_instantiate(std::string programName, Strings argv) for (auto & i : files) { Expr & e = fromArgs ? evaluator->parseExprFromString(i, CanonPath::fromCwd()) - : evaluator->parseExprFromFile(resolveExprPath(evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, i)))); + : evaluator->parseExprFromFile(evaluator->paths.resolveExprPath(evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, i)))); processExpr(*state, attrPaths, parseOnly, strict, autoArgs, evalOnly, outputKind, xmlOutputSourceLocation, e); } diff --git a/lix/libcmd/installables.cc b/lix/libcmd/installables.cc index 9970d4319..33a3acb4e 100644 --- a/lix/libcmd/installables.cc +++ b/lix/libcmd/installables.cc @@ -214,9 +214,9 @@ void SourceExprCommand::completeInstallable(EvalState & state, AddCompletions & auto evaluator = getEvaluator(); - Expr & e = evaluator->parseExprFromFile( - resolveExprPath(evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, *file))) - ); + Expr & e = evaluator->parseExprFromFile(evaluator->paths.resolveExprPath( + evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, *file)) + )); Value root; state.eval(e, root); diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 4dee03c79..da660d75a 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -951,7 +951,7 @@ void EvalState::evalFile(const SourcePath & path_, Value & v) return; } - auto resolvedPath = resolveExprPath(path); + auto resolvedPath = ctx.paths.resolveExprPath(path); if (auto i = ctx.caches.fileEval.find(resolvedPath); i != ctx.caches.fileEval.end()) { v = i->second->result; return; @@ -2661,7 +2661,7 @@ void Evaluator::printStatistics() } -SourcePath resolveExprPath(SourcePath path) +SourcePath EvalPaths::resolveExprPath(SourcePath path) { unsigned int followCount = 0, maxFollow = 1024; diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index b391f68f8..bb87add5d 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -418,6 +418,11 @@ public: */ SourcePath checkSourcePath(const SourcePath & path); + /** + * If `path` refers to a directory, then append "/default.nix". + */ + SourcePath resolveExprPath(SourcePath path); + void checkURI(const std::string & uri); /** @@ -848,11 +853,6 @@ private: std::string_view showType(ValueType type, bool withArticle = true); std::string showType(const Value & v); -/** - * If `path` refers to a directory, then append "/default.nix". - */ -SourcePath resolveExprPath(SourcePath path); - static constexpr std::string_view corepkgsPrefix{"/__corepkgs__/"}; diff --git a/lix/libexpr/flake/flake.cc b/lix/libexpr/flake/flake.cc index 8ccdf7373..653ee56d1 100644 --- a/lix/libexpr/flake/flake.cc +++ b/lix/libexpr/flake/flake.cc @@ -242,7 +242,7 @@ static Flake getFlake( }; // FIXME: symlink attack - auto resolvedFlakeFile = resolveExprPath(state.ctx.paths.checkSourcePath(CanonPath(flakeFile))); + auto resolvedFlakeFile = state.ctx.paths.resolveExprPath(state.ctx.paths.checkSourcePath(CanonPath(flakeFile))); Expr & flakeExpr = state.ctx.parseExprFromFile(state.ctx.paths.checkSourcePath(resolvedFlakeFile)); // Enforce that 'flake.nix' is a direct attrset, not a computation. diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index e2bddcd15..8ce50bf1a 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -251,7 +251,7 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v // args[0]->attrs is already sorted. debug("evaluating file '%1%'", path); - Expr & e = state.ctx.parseExprFromFile(resolveExprPath(path), staticEnv); + Expr & e = state.ctx.parseExprFromFile(state.ctx.paths.resolveExprPath(path), staticEnv); e.eval(state, *env, v); } diff --git a/lix/nix/prefetch.cc b/lix/nix/prefetch.cc index 9457ad797..c45caa063 100644 --- a/lix/nix/prefetch.cc +++ b/lix/nix/prefetch.cc @@ -201,7 +201,7 @@ static int main_nix_prefetch_url(std::string programName, Strings argv) } else { Value vRoot; state->evalFile( - resolveExprPath( + evaluator->paths.resolveExprPath( lookupFileArg(*evaluator, args.empty() ? "." : args[0])), vRoot); Value & v(*findAlongAttrPath(*state, attrPath, autoArgs, vRoot).first); From 3f6a1e45c903572a06608a79bcb5c61f749d9644 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 06/10] libexpr: always checkSourcePath in resolveExprPath the purpose of resolveExprPath is to produce a parser input path. parser input paths must be validated against the path allow list so they do not escape the restricted/pure eval sandbox. checking the input path and any intermediate paths during resolving makes this a lot harder to do badly. Change-Id: Ib31b5bca63fe26a5e08458a871cdc9f92f9b6a10 --- lix/legacy/nix-build.cc | 6 +++--- lix/legacy/nix-instantiate.cc | 2 +- lix/libcmd/installables.cc | 6 +++--- lix/libexpr/eval.cc | 7 +++++-- lix/libexpr/flake/flake.cc | 4 ++-- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lix/legacy/nix-build.cc b/lix/legacy/nix-build.cc index dae707379..5423cf5b9 100644 --- a/lix/legacy/nix-build.cc +++ b/lix/legacy/nix-build.cc @@ -253,12 +253,12 @@ static void main_nix_build(std::string programName, Strings argv) else /* If we're in a #! script, interpret filenames relative to the script. */ - exprs.push_back(evaluator->parseExprFromFile(evaluator->paths.resolveExprPath( - evaluator->paths.checkSourcePath(lookupFileArg( + exprs.push_back(evaluator->parseExprFromFile( + evaluator->paths.resolveExprPath(lookupFileArg( *evaluator, inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i )) - ))); + )); } } diff --git a/lix/legacy/nix-instantiate.cc b/lix/legacy/nix-instantiate.cc index 688201f2f..13a05c845 100644 --- a/lix/legacy/nix-instantiate.cc +++ b/lix/legacy/nix-instantiate.cc @@ -183,7 +183,7 @@ static int main_nix_instantiate(std::string programName, Strings argv) for (auto & i : files) { Expr & e = fromArgs ? evaluator->parseExprFromString(i, CanonPath::fromCwd()) - : evaluator->parseExprFromFile(evaluator->paths.resolveExprPath(evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, i)))); + : evaluator->parseExprFromFile(evaluator->paths.resolveExprPath(lookupFileArg(*evaluator, i))); processExpr(*state, attrPaths, parseOnly, strict, autoArgs, evalOnly, outputKind, xmlOutputSourceLocation, e); } diff --git a/lix/libcmd/installables.cc b/lix/libcmd/installables.cc index 33a3acb4e..1bf630883 100644 --- a/lix/libcmd/installables.cc +++ b/lix/libcmd/installables.cc @@ -214,9 +214,9 @@ void SourceExprCommand::completeInstallable(EvalState & state, AddCompletions & auto evaluator = getEvaluator(); - Expr & e = evaluator->parseExprFromFile(evaluator->paths.resolveExprPath( - evaluator->paths.checkSourcePath(lookupFileArg(*evaluator, *file)) - )); + Expr & e = evaluator->parseExprFromFile( + state.ctx.paths.resolveExprPath(lookupFileArg(*evaluator, *file)) + ); Value root; state.eval(e, root); diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index da660d75a..19fdd3b2b 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -2661,8 +2661,9 @@ void Evaluator::printStatistics() } -SourcePath EvalPaths::resolveExprPath(SourcePath path) +SourcePath EvalPaths::resolveExprPath(SourcePath path_) { + auto path = checkSourcePath(path_); unsigned int followCount = 0, maxFollow = 1024; /* If `path' is a symlink, follow it. This is so that relative @@ -2672,7 +2673,9 @@ SourcePath EvalPaths::resolveExprPath(SourcePath path) if (++followCount >= maxFollow) throw Error("too many symbolic links encountered while traversing the path '%s'", path); if (path.lstat().type != InputAccessor::tSymlink) break; - path = {CanonPath(path.readLink(), path.canonical().parent().value_or(CanonPath::root))}; + path = checkSourcePath( + CanonPath(path.readLink(), path.canonical().parent().value_or(CanonPath::root)) + ); } /* If `path' refers to a directory, append `/default.nix'. */ diff --git a/lix/libexpr/flake/flake.cc b/lix/libexpr/flake/flake.cc index 653ee56d1..33e2951bb 100644 --- a/lix/libexpr/flake/flake.cc +++ b/lix/libexpr/flake/flake.cc @@ -242,8 +242,8 @@ static Flake getFlake( }; // FIXME: symlink attack - auto resolvedFlakeFile = state.ctx.paths.resolveExprPath(state.ctx.paths.checkSourcePath(CanonPath(flakeFile))); - Expr & flakeExpr = state.ctx.parseExprFromFile(state.ctx.paths.checkSourcePath(resolvedFlakeFile)); + auto resolvedFlakeFile = state.ctx.paths.resolveExprPath(CanonPath(flakeFile)); + Expr & flakeExpr = state.ctx.parseExprFromFile(resolvedFlakeFile); // Enforce that 'flake.nix' is a direct attrset, not a computation. if (!(dynamic_cast(&flakeExpr))) { From 8db8ac9a671e7210c7e312522eb346f78ed89c86 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 07/10] always checkSource before accessing anything like earlier, anything accessed during eval must be checked against the list of path restrictions. this notably excludes `Pos::getSource` which is run only from an unrestricted context (resolving line/column numbers for expressions), but since positions require the parser to run and the parser requires a checked input to produce positions this is not a leak Change-Id: I337859e9c780590d4434885125a3ef70a11f6e93 --- lix/legacy/nix-env.cc | 22 ++++++++++++++-------- lix/libcmd/installable-value.cc | 2 +- lix/libcmd/repl.cc | 2 +- lix/libexpr/eval.cc | 4 ++-- lix/libexpr/primops.cc | 8 +++++++- lix/nix/flake.cc | 4 +++- 6 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 38664f63b..e83522dc6 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -96,11 +96,16 @@ static bool parseInstallSourceOptions(Globals & globals, } -static bool isNixExpr(const SourcePath & path, struct InputAccessor::Stat & st) +static bool isNixExpr(EvalPaths & paths, const SourcePath & path, struct InputAccessor::Stat & st) { - return - st.type == InputAccessor::tRegular - || (st.type == InputAccessor::tDirectory && (path + "default.nix").pathExists()); + if (st.type == InputAccessor::tRegular) { + return true; + } else if (st.type != InputAccessor::tDirectory) { + return false; + } else { + auto defaultNix = paths.checkSourcePath(path + "default.nix"); + return defaultNix.pathExists(); + } } @@ -119,7 +124,7 @@ static void getAllExprs(Evaluator & state, are implemented using profiles). */ if (i == "manifest.nix") continue; - SourcePath path2 = path + i; + SourcePath path2 = state.paths.checkSourcePath(path + i); InputAccessor::Stat st; try { @@ -128,7 +133,7 @@ static void getAllExprs(Evaluator & state, continue; // ignore dangling symlinks in ~/.nix-defexpr } - if (isNixExpr(path2, st) && (st.type != InputAccessor::tRegular || path2.baseName().ends_with(".nix"))) { + if (isNixExpr(state.paths, path2, st) && (st.type != InputAccessor::tRegular || path2.baseName().ends_with(".nix"))) { /* Strip off the `.nix' filename suffix (if applicable), otherwise the attribute cannot be selected with the `-A' option. Useful if you want to stick a Nix @@ -160,11 +165,12 @@ static void getAllExprs(Evaluator & state, -static void loadSourceExpr(EvalState & state, const SourcePath & path, Value & v) +static void loadSourceExpr(EvalState & state, const SourcePath & path_, Value & v) { + auto path = state.ctx.paths.checkSourcePath(path_); auto st = path.stat(); - if (isNixExpr(path, st)) + if (isNixExpr(state.ctx.paths, path, st)) state.evalFile(path, v); /* The path is a directory. Put the Nix expressions in the diff --git a/lix/libcmd/installable-value.cc b/lix/libcmd/installable-value.cc index b16e6589a..42d87936b 100644 --- a/lix/libcmd/installable-value.cc +++ b/lix/libcmd/installable-value.cc @@ -47,7 +47,7 @@ std::optional InstallableValue::trySinglePathToDerivedPaths ) { if (v.type() == nPath) { - auto storePath = fetchToStore(*evaluator->store, v.path()); + auto storePath = fetchToStore(*evaluator->store, state.ctx.paths.checkSourcePath(v.path())); return {{ .path = DerivedPath::Opaque { .path = std::move(storePath), diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index 521d2898b..56d9da9d8 100644 --- a/lix/libcmd/repl.cc +++ b/lix/libcmd/repl.cc @@ -1109,7 +1109,7 @@ void NixRepl::evalString(std::string s, Value & v) Value * NixRepl::evalFile(SourcePath & path) { - auto & expr = evaluator.parseExprFromFile(path, staticEnv); + auto & expr = evaluator.parseExprFromFile(evaluator.paths.checkSourcePath(path), staticEnv); Value * result(evaluator.mem.allocValue()); expr.eval(state, *env, *result); state.forceValue(*result, result->determinePos(noPos)); diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 19fdd3b2b..b59d52459 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -2347,7 +2347,7 @@ StorePath EvalPaths::copyPathToStore(NixStringContext & context, const SourcePat auto dstPath = i != srcToStore.end() ? i->second : [&]() { - auto dstPath = fetchToStore(*store, path, path.baseName(), FileIngestionMethod::Recursive, nullptr, repair); + auto dstPath = fetchToStore(*store, checkSourcePath(path), path.baseName(), FileIngestionMethod::Recursive, nullptr, repair); allowPath(dstPath); srcToStore.insert_or_assign(path, dstPath); printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath)); @@ -2680,7 +2680,7 @@ SourcePath EvalPaths::resolveExprPath(SourcePath path_) /* If `path' refers to a directory, append `/default.nix'. */ if (path.lstat().type == InputAccessor::tDirectory) - return path + "default.nix"; + return checkSourcePath(path + "default.nix"); return path; } diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 8ce50bf1a..d9ced1df7 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -1548,7 +1548,13 @@ static void addPath( if (!expectedHash || !state.ctx.store->isValidPath(*expectedStorePath)) { auto dstPath = fetchToStore( - *state.ctx.store, CanonPath(path), name, method, &filter, state.ctx.repair); + *state.ctx.store, + state.ctx.paths.checkSourcePath(CanonPath(path)), + name, + method, + &filter, + state.ctx.repair + ); if (expectedHash && expectedStorePath != dstPath) state.ctx.errors.make( "store path mismatch in (possibly filtered) path added from '%s'", diff --git a/lix/nix/flake.cc b/lix/nix/flake.cc index c0797e225..747cda52a 100644 --- a/lix/nix/flake.cc +++ b/lix/nix/flake.cc @@ -542,7 +542,9 @@ struct CmdFlakeCheck : FlakeCommand if (auto attr = v.attrs->get(evaluator->symbols.create("path"))) { if (attr->name == evaluator->symbols.create("path")) { NixStringContext context; - auto path = state->coerceToPath(attr->pos, *attr->value, context, ""); + auto path = state->ctx.paths.checkSourcePath( + state->coerceToPath(attr->pos, *attr->value, context, "") + ); if (!path.pathExists()) throw Error("template '%s' refers to a non-existent path '%s'", attrPath, path); // TODO: recursively check the flake in 'path'. From c948b350fb67550fade2209bbfa49b1d668285c8 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 17 Dec 2024 16:25:00 +0100 Subject: [PATCH 08/10] libutil: add CheckedSourcePath for accessing things SourcePath only manipulates path names now. all accesses must go through a checked path going forward to ensure we don't escape restriction lists of pure and restricted evaluation. if a directory path is checked it can safely be assumed that the directory itself is allowed, and its contents will likewise be safe to access. it is tempting to assumed that contents will also be fine, but that's only true if the content is not a symlink. Change-Id: Icec3098d53fe9dce50997954ba958fe4f304d59b --- lix/legacy/nix-env.cc | 6 +- lix/libcmd/repl.cc | 4 +- lix/libexpr/eval.cc | 25 ++++---- lix/libexpr/eval.hh | 10 +-- lix/libexpr/primops.cc | 22 +++---- lix/libexpr/value-to-xml.cc | 2 +- lix/libfetchers/fetch-to-store.cc | 2 +- lix/libfetchers/fetch-to-store.hh | 2 +- lix/libutil/position.cc | 2 +- lix/libutil/position.hh | 2 +- lix/libutil/source-path.cc | 10 +-- lix/libutil/source-path.hh | 103 ++++++++++++++++++------------ 12 files changed, 107 insertions(+), 83 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index e83522dc6..1a98bedaa 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -96,7 +96,7 @@ static bool parseInstallSourceOptions(Globals & globals, } -static bool isNixExpr(EvalPaths & paths, const SourcePath & path, struct InputAccessor::Stat & st) +static bool isNixExpr(EvalPaths & paths, const CheckedSourcePath & path, struct InputAccessor::Stat & st) { if (st.type == InputAccessor::tRegular) { return true; @@ -113,7 +113,7 @@ static constexpr size_t maxAttrs = 1024; static void getAllExprs(Evaluator & state, - const SourcePath & path, StringSet & seen, BindingsBuilder & attrs) + const CheckedSourcePath & path, StringSet & seen, BindingsBuilder & attrs) { StringSet namesSorted; for (auto & [name, _] : path.readDirectory()) namesSorted.insert(name); @@ -124,7 +124,7 @@ static void getAllExprs(Evaluator & state, are implemented using profiles). */ if (i == "manifest.nix") continue; - SourcePath path2 = state.paths.checkSourcePath(path + i); + auto path2 = state.paths.checkSourcePath(path + i); InputAccessor::Stat st; try { diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index 56d9da9d8..86feec53f 100644 --- a/lix/libcmd/repl.cc +++ b/lix/libcmd/repl.cc @@ -655,7 +655,7 @@ ProcessLineResult NixRepl::processLine(std::string line) return {path, 0}; } else if (v.isLambda()) { auto pos = evaluator.positions[v.lambda.fun->pos]; - if (auto path = std::get_if(&pos.origin)) + if (auto path = std::get_if(&pos.origin)) return {*path, pos.line}; else throw Error("'%s' cannot be shown in an editor", pos); @@ -820,7 +820,7 @@ ProcessLineResult NixRepl::processLine(std::string line) logger->cout(trim(renderMarkdownToTerminal(markdown))); } else if (v.isLambda()) { auto pos = evaluator.positions[v.lambda.fun->pos]; - if (auto path = std::get_if(&pos.origin)) { + if (auto path = std::get_if(&pos.origin)) { // Path and position have now been obtained, feed to nix-doc library to get data. auto docComment = lambdaDocsForPos(*path, pos); if (!docComment) { diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index b59d52459..6217bc555 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -379,9 +379,9 @@ void EvalPaths::allowAndSetStorePathString(const StorePath & storePath, Value & mkStorePathString(storePath, v); } -SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) +CheckedSourcePath EvalPaths::checkSourcePath(const SourcePath & path_) { - if (!allowedPaths) return path_; + if (!allowedPaths) return auto(path_).unsafeIntoChecked(); auto i = resolvedPaths.find(path_.canonical().abs()); if (i != resolvedPaths.end()) @@ -395,7 +395,9 @@ SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) */ Path abspath = canonPath(path_.canonical().abs()); - if (abspath.starts_with(corepkgsPrefix)) return CanonPath(abspath); + if (abspath.starts_with(corepkgsPrefix)) { + return SourcePath(CanonPath(abspath)).unsafeIntoChecked(); + } for (auto & i : *allowedPaths) { if (isDirOrInDir(abspath, i)) { @@ -417,8 +419,9 @@ SourcePath EvalPaths::checkSourcePath(const SourcePath & path_) for (auto & i : *allowedPaths) { if (isDirOrInDir(path.canonical().abs(), i)) { - resolvedPaths.insert_or_assign(path_.canonical().abs(), path); - return path; + auto checked = path.unsafeIntoChecked(); + resolvedPaths.insert_or_assign(path_.canonical().abs(), checked); + return checked; } } @@ -803,7 +806,7 @@ void Evaluator::evalLazily(Expr & e, Value & v) void EvalState::mkPos(Value & v, PosIdx p) { auto origin = ctx.positions.originOf(p); - if (auto path = std::get_if(&origin)) { + if (auto path = std::get_if(&origin)) { auto attrs = ctx.buildBindings(3); attrs.alloc(ctx.s.file).mkString(path->to_string()); makePositionThunks(*this, p, attrs.alloc(ctx.s.line), attrs.alloc(ctx.s.column)); @@ -2621,7 +2624,7 @@ void Evaluator::printStatistics() else obj["name"] = nullptr; if (auto pos = positions[fun->pos]) { - if (auto path = std::get_if(&pos.origin)) + if (auto path = std::get_if(&pos.origin)) obj["file"] = path->to_string(); obj["line"] = pos.line; obj["column"] = pos.column; @@ -2636,7 +2639,7 @@ void Evaluator::printStatistics() for (auto & i : stats.attrSelects) { json obj = json::object(); if (auto pos = positions[i.first]) { - if (auto path = std::get_if(&pos.origin)) + if (auto path = std::get_if(&pos.origin)) obj["file"] = path->to_string(); obj["line"] = pos.line; obj["column"] = pos.column; @@ -2661,7 +2664,7 @@ void Evaluator::printStatistics() } -SourcePath EvalPaths::resolveExprPath(SourcePath path_) +CheckedSourcePath EvalPaths::resolveExprPath(SourcePath path_) { auto path = checkSourcePath(path_); unsigned int followCount = 0, maxFollow = 1024; @@ -2686,13 +2689,13 @@ SourcePath EvalPaths::resolveExprPath(SourcePath path_) } -Expr & Evaluator::parseExprFromFile(const SourcePath & path) +Expr & Evaluator::parseExprFromFile(const CheckedSourcePath & path) { return parseExprFromFile(path, builtins.staticEnv); } -Expr & Evaluator::parseExprFromFile(const SourcePath & path, std::shared_ptr & staticEnv) +Expr & Evaluator::parseExprFromFile(const CheckedSourcePath & path, std::shared_ptr & staticEnv) { auto buffer = path.readFile(); return *parse(buffer.data(), buffer.size(), Pos::Origin(path), path.parent(), staticEnv); diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index bb87add5d..5c0ddc978 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -393,7 +393,7 @@ private: /** * Cache used by checkSourcePath(). */ - std::unordered_map resolvedPaths; + std::unordered_map resolvedPaths; public: /** @@ -416,12 +416,12 @@ public: * Check whether access to a path is allowed and throw an error if * not. Otherwise return the canonicalised path. */ - SourcePath checkSourcePath(const SourcePath & path); + CheckedSourcePath checkSourcePath(const SourcePath & path); /** * If `path` refers to a directory, then append "/default.nix". */ - SourcePath resolveExprPath(SourcePath path); + CheckedSourcePath resolveExprPath(SourcePath path); void checkURI(const std::string & uri); @@ -536,8 +536,8 @@ public: /** * Parse a Nix expression from the specified file. */ - Expr & parseExprFromFile(const SourcePath & path); - Expr & parseExprFromFile(const SourcePath & path, std::shared_ptr & staticEnv); + Expr & parseExprFromFile(const CheckedSourcePath & path); + Expr & parseExprFromFile(const CheckedSourcePath & path, std::shared_ptr & staticEnv); /** * Parse a Nix expression from the specified string. diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index d9ced1df7..cfbf86ced 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -117,12 +117,7 @@ StringMap EvalPaths::realiseContext(const NixStringContext & context) return res; } -struct RealisePathFlags { - // Whether to check that the path is allowed in pure eval mode - bool checkForPureEval = true; -}; - -static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, const RealisePathFlags flags = {}) +static auto realisePath(EvalState & state, const PosIdx pos, Value & v, auto checkFn) { NixStringContext context; @@ -131,17 +126,20 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, co try { StringMap rewrites = state.ctx.paths.realiseContext(context); - auto realPath = CanonPath(state.ctx.paths.toRealPath(rewriteStrings(path.canonical().abs(), rewrites), context)); - - return flags.checkForPureEval - ? state.ctx.paths.checkSourcePath(realPath) - : realPath; + return checkFn(SourcePath(CanonPath( + state.ctx.paths.toRealPath(rewriteStrings(path.canonical().abs(), rewrites), context) + ))); } catch (Error & e) { e.addTrace(state.ctx.positions[pos], "while realising the context of path '%s'", path); throw; } } +static CheckedSourcePath realisePath(EvalState & state, const PosIdx pos, Value & v) +{ + return realisePath(state, pos, v, [&](auto p) { return state.ctx.paths.checkSourcePath(p); }); +} + /** * Add and attribute to the given attribute map from the output name to * the output path, or a placeholder. @@ -1198,7 +1196,7 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, can’t just catch the exception here because we still want to throw if something in the evaluation of `arg` tries to access an unauthorized path). */ - auto path = realisePath(state, pos, arg, { .checkForPureEval = false }); + auto path = realisePath(state, pos, arg, std::identity{}); /* SourcePath doesn't know about trailing slash. */ auto mustBeDir = arg.type() == nString diff --git a/lix/libexpr/value-to-xml.cc b/lix/libexpr/value-to-xml.cc index 49c21623f..e0b4357ba 100644 --- a/lix/libexpr/value-to-xml.cc +++ b/lix/libexpr/value-to-xml.cc @@ -21,7 +21,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, static void posToXML(EvalState & state, XMLAttrs & xmlAttrs, const Pos & pos) { - if (auto path = std::get_if(&pos.origin)) + if (auto path = std::get_if(&pos.origin)) xmlAttrs["path"] = path->to_string(); xmlAttrs["line"] = fmt("%1%", pos.line); xmlAttrs["column"] = fmt("%1%", pos.column); diff --git a/lix/libfetchers/fetch-to-store.cc b/lix/libfetchers/fetch-to-store.cc index ee7304338..a276a48fd 100644 --- a/lix/libfetchers/fetch-to-store.cc +++ b/lix/libfetchers/fetch-to-store.cc @@ -6,7 +6,7 @@ namespace nix { StorePath fetchToStore( Store & store, - const SourcePath & path, + const CheckedSourcePath & path, std::string_view name, FileIngestionMethod method, PathFilter * filter, diff --git a/lix/libfetchers/fetch-to-store.hh b/lix/libfetchers/fetch-to-store.hh index b0472dbaa..6d4d072aa 100644 --- a/lix/libfetchers/fetch-to-store.hh +++ b/lix/libfetchers/fetch-to-store.hh @@ -13,7 +13,7 @@ namespace nix { */ StorePath fetchToStore( Store & store, - const SourcePath & path, + const CheckedSourcePath & path, std::string_view name = "source", FileIngestionMethod method = FileIngestionMethod::Recursive, PathFilter * filter = nullptr, diff --git a/lix/libutil/position.cc b/lix/libutil/position.cc index cf2ff4b45..d98cd4e93 100644 --- a/lix/libutil/position.cc +++ b/lix/libutil/position.cc @@ -62,7 +62,7 @@ std::optional Pos::getSource() const // Get rid of the null terminators added by the parser. return std::string(s.source->c_str()); }, - [](const SourcePath & path) -> std::optional { + [](const CheckedSourcePath & path) -> std::optional { try { return path.readFile(); } catch (Error &) { diff --git a/lix/libutil/position.hh b/lix/libutil/position.hh index 3f89a0f5a..98fd7f15b 100644 --- a/lix/libutil/position.hh +++ b/lix/libutil/position.hh @@ -42,7 +42,7 @@ struct Pos auto operator<=>(const Hidden &) const = default; }; - typedef std::variant