From 6877988515cd624109b18c835d2d06f065c432d4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Feb 2025 14:45:41 -0500 Subject: [PATCH 1/2] Fix dev shell There was one `inputs.nixFmt` left after 573ffac2e67659dd00904e8bf53a66adfc40f95e. --- flake.nix | 2 +- packaging/dev-shell.nix | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/flake.nix b/flake.nix index c2461a2ebc7..8283ccf970d 100644 --- a/flake.nix +++ b/flake.nix @@ -403,7 +403,7 @@ devShells = let - makeShell = import ./packaging/dev-shell.nix { inherit inputs lib devFlake; }; + makeShell = import ./packaging/dev-shell.nix { inherit lib devFlake; }; prefixAttrs = prefix: lib.concatMapAttrs (k: v: { "${prefix}-${k}" = v; }); in forAllSystems ( diff --git a/packaging/dev-shell.nix b/packaging/dev-shell.nix index 8e1bb89368a..1b6c37f354d 100644 --- a/packaging/dev-shell.nix +++ b/packaging/dev-shell.nix @@ -1,6 +1,5 @@ { lib, - inputs, devFlake, }: @@ -117,7 +116,7 @@ pkgs.nixComponents.nix-util.overrideAttrs ( pkgs.buildPackages.changelog-d modular.pre-commit.settings.package (pkgs.writeScriptBin "pre-commit-hooks-install" modular.pre-commit.settings.installationScript) - inputs.nixfmt.packages.${pkgs.hostPlatform.system}.default + pkgs.buildPackages.nixfmt-rfc-style ] # TODO: Remove the darwin check once # https://github.com/NixOS/nixpkgs/pull/291814 is available From c811b338457b85962afe19441aeb6775ad1b3fba Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Feb 2025 18:46:37 -0500 Subject: [PATCH 2/2] `Store::getFSAccessor`: Do not include the store dir Rather than "mounting" the store inside an empty virtual filesystem, just return the store as a virtual filesystem. This is more modular. (FWIW, it also supports two long term hopes of mind: 1. More capability-based Nix language mode. I dream of a "super pure eval" where you can only use relative path literals (See #8738), and any `fetchTree`-fetched stuff + the store are all disjoint (none is mounted in another) file systems. 2. Windows, where the store dir may include drive letters, etc., and is thus unsuitable to be the prefix of any `CanonPath`s. ) --- src/libexpr/eval.cc | 20 ++++++++- src/libfetchers/store-path-accessor.cc | 6 +-- src/libstore/build/worker.cc | 2 +- src/libstore/dummy-store.cc | 4 +- src/libstore/local-fs-store.cc | 31 +++++++++----- src/libstore/local-store.cc | 2 +- src/libstore/remote-fs-accessor.cc | 5 ++- src/libstore/store-api.cc | 2 +- src/libutil/meson.build | 1 + src/libutil/source-accessor.cc | 24 ++++++++++- src/libutil/source-accessor.hh | 39 ++++++++++++++++++ src/libutil/subdir-source-accessor.cc | 56 ++++++++++++++++++++++++++ src/nix-store/nix-store.cc | 2 +- src/nix/cat.cc | 19 +++++---- src/nix/env.cc | 4 +- src/nix/ls.cc | 26 ++++++------ src/nix/why-depends.cc | 2 +- tests/functional/shell.sh | 2 +- 18 files changed, 198 insertions(+), 49 deletions(-) create mode 100644 src/libutil/subdir-source-accessor.cc diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 6a45f24b82a..6a907e57e10 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -263,7 +263,25 @@ EvalState::EvalState( auto storeFS = makeMountedSourceAccessor( { {CanonPath::root, makeEmptySourceAccessor()}, - {CanonPath(store->storeDir), makeFSSourceAccessor(realStoreDir)} + /* In the pure eval case, we can simply require + valid paths. However, in the *impure* eval + case this gets in the way of the union + mechanism, because an invalid access in the + upper layer will *not* be caught by the union + source accessor, but instead abort the entire + lookup. + + This happens when the store dir in the + ambient file system has a path (e.g. because + another Nix store there), but the relocated + store does not. + + TODO make the various source accessors doing + access control all throw the same time of + exception, and make union source accessor + catch it, so we don't need to do this hack. + */ + {CanonPath(store->storeDir), store->getFSAccessor(settings.pureEval)}, }); accessor = settings.pureEval ? storeFS diff --git a/src/libfetchers/store-path-accessor.cc b/src/libfetchers/store-path-accessor.cc index 528bf2a4f51..9d3b7c4ec09 100644 --- a/src/libfetchers/store-path-accessor.cc +++ b/src/libfetchers/store-path-accessor.cc @@ -5,11 +5,7 @@ namespace nix { ref makeStorePathAccessor(ref store, const StorePath & storePath) { - // FIXME: should use `store->getFSAccessor()` - auto root = std::filesystem::path{store->toRealPath(storePath)}; - auto accessor = makeFSSourceAccessor(root); - accessor->setPathDisplay(root.string()); - return accessor; + return projectSubdirSourceAccessor(store->getFSAccessor(), storePath.to_string()); } } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index b765fc2a002..cb6ce028e29 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -568,7 +568,7 @@ bool Worker::pathContentsGood(const StorePath & path) res = false; else { auto current = hashPath( - {store.getFSAccessor(), CanonPath(store.printStorePath(path))}, + {store.getFSAccessor(), CanonPath(path.to_string())}, FileIngestionMethod::NixArchive, info->narHash.algo).first; Hash nullHash(HashAlgorithm::SHA256); res = info->narHash == nullHash || info->narHash == current; diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index c1e871e9384..9bacfb8b1de 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -83,7 +83,9 @@ struct DummyStore : public virtual DummyStoreConfig, public virtual Store { callback(nullptr); } virtual ref getFSAccessor(bool requireValidPath) override - { unsupported("getFSAccessor"); } + { + return makeEmptySourceAccessor(); + } }; static RegisterStoreImplementation regDummyStore; diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index 5449b20eb3b..f63585678e3 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -33,30 +33,37 @@ struct LocalStoreAccessor : PosixSourceAccessor bool requireValidPath; LocalStoreAccessor(ref store, bool requireValidPath) - : store(store) + : PosixSourceAccessor(std::filesystem::path{store->realStoreDir.get()}) + , store(store) , requireValidPath(requireValidPath) - { } + { + // Not `realStoreDir`, because this is where the store objects "morally" reside. + ownLocationForSymlinkResolution = store->storeDir; + } + - CanonPath toRealPath(const CanonPath & path) + void requireStoreObject(const CanonPath & path) { - auto [storePath, rest] = store->toStorePath(path.abs()); + auto [storePath, rest] = store->toStorePath(store->storeDir + path.abs()); if (requireValidPath && !store->isValidPath(storePath)) throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); - return CanonPath(store->getRealStoreDir()) / storePath.to_string() / CanonPath(rest); } std::optional maybeLstat(const CanonPath & path) override { - /* Handle the case where `path` is (a parent of) the store. */ - if (isDirOrInDir(store->storeDir, path.abs())) + /* Also allow `path` to point to the entire store, which is + needed for resolving symlinks. */ + if (path.isRoot()) return Stat{ .type = tDirectory }; - return PosixSourceAccessor::maybeLstat(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::maybeLstat(path); } DirEntries readDirectory(const CanonPath & path) override { - return PosixSourceAccessor::readDirectory(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::readDirectory(path); } void readFile( @@ -64,12 +71,14 @@ struct LocalStoreAccessor : PosixSourceAccessor Sink & sink, std::function sizeCallback) override { - return PosixSourceAccessor::readFile(toRealPath(path), sink, sizeCallback); + requireStoreObject(path); + return PosixSourceAccessor::readFile(path, sink, sizeCallback); } std::string readLink(const CanonPath & path) override { - return PosixSourceAccessor::readLink(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::readLink(path); } }; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 67d5a1dcb7d..c5e8ff49a5a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1104,7 +1104,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, auto & specified = *info.ca; auto actualHash = ({ auto accessor = getFSAccessor(false); - CanonPath path { printStorePath(info.path) }; + CanonPath path { info.path.to_string() }; Hash h { HashAlgorithm::SHA256 }; // throwaway def to appease C++ auto fim = specified.method.getFileIngestionMethod(); switch (fim) { diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 7e360b5fef1..616c8443a14 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -15,6 +15,9 @@ RemoteFSAccessor::RemoteFSAccessor(ref store, bool requireValidPath, cons { if (cacheDir != "") createDirs(cacheDir); + + // Not `realStoreDir`, because this is where the store objects "morally" reside. + ownLocationForSymlinkResolution = store->storeDir; } Path RemoteFSAccessor::makeCacheFile(std::string_view hashPart, const std::string & ext) @@ -51,7 +54,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std: std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPath & path) { - auto [storePath, restPath_] = store->toStorePath(path.abs()); + auto [storePath, restPath_] = store->toStorePath(store->storeDir + path.abs()); auto restPath = CanonPath(restPath_); if (requireValidPath && !store->isValidPath(storePath)) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 236622eae37..204f22eb58d 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1232,7 +1232,7 @@ static Derivation readDerivationCommon(Store & store, const StorePath & drvPath, auto accessor = store.getFSAccessor(requireValidPath); try { return parseDerivation(store, - accessor->readFile(CanonPath(store.printStorePath(drvPath))), + accessor->readFile(CanonPath(drvPath.to_string())), Derivation::nameFromPath(drvPath)); } catch (FormatError & e) { throw Error("error parsing derivation '%s': %s", store.printStorePath(drvPath), e.msg()); diff --git a/src/libutil/meson.build b/src/libutil/meson.build index df459f0e57f..35757b0646c 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -162,6 +162,7 @@ sources = files( 'signature/signer.cc', 'source-accessor.cc', 'source-path.cc', + 'subdir-source-accessor.cc', 'strings.cc', 'suggestions.cc', 'tarfile.cc', diff --git a/src/libutil/source-accessor.cc b/src/libutil/source-accessor.cc index 78f038cf377..2f5054b4967 100644 --- a/src/libutil/source-accessor.cc +++ b/src/libutil/source-accessor.cc @@ -114,9 +114,29 @@ CanonPath SourceAccessor::resolveSymlinks( if (!linksAllowed--) throw Error("infinite symlink recursion in path '%s'", showPath(path)); auto target = readLink(res); - res.pop(); - if (isAbsolute(target)) + if (isAbsolute(target)) { + auto prefixLen = ownLocationForSymlinkResolution.size(); + /* TODO need to support windows paths without making + Unix vulnerability. The problem is we don't know whether + the target (which is *not* a `CanonPath`) is a + Windows or Unix path. Because of remote stores to + other OSes, and abstract stores (which should + have host-OS-agnostic designs), we cannot simply + use whether we are running on Unix or Windows for + this. */ + if (!(hasPrefix(target, ownLocationForSymlinkResolution) && (target.size() == prefixLen || target[prefixLen] == '/'))) { + throw Error( + "cannot resolve '%s': " + "symlink target '%s' points outside source accessor that is considered to reside at '%s'", + showPath(path), + target, + ownLocationForSymlinkResolution); + } + target = std::move(target).substr(prefixLen); res = CanonPath::root; + } else { + res.pop(); + } todo.splice(todo.begin(), tokenizeString>(target, "/")); } } diff --git a/src/libutil/source-accessor.hh b/src/libutil/source-accessor.hh index 79ae092ac18..c49551d3d5c 100644 --- a/src/libutil/source-accessor.hh +++ b/src/libutil/source-accessor.hh @@ -176,6 +176,39 @@ struct SourceAccessor : std::enable_shared_from_this const CanonPath & path, SymlinkResolution mode = SymlinkResolution::Full); + /** + * When `resolveSymlinks` encounters an absolute target, that path + * may or may not denote a local inside the current source accessor + * --- there is no general answer to the question, it depends on + * what the current source accessor "represents". + * + * For now, we use this variable to decide: if the path has this + * path as a prefix, then the suffix points inside this store + * prefix. If the path does not have path as prefix, than the + * symlink escapes the source acccessor. + * + * Note, that we do *not* want this to become a general "location of + * this source accessor" variable: That would further violate the + * "don't know your own name principle. Indeed, the same source + * accessor may be used in multiple different contexts, at different + * locations, and so its "location" is a property of the caller + * rather than the thing itself. + * + * @todo Based on the above observations, we should instead get rid + * of this field, and separate out symlink resolving (other than the + * low-level, interpretation-*agnostic* `readLink`) from the source + * accessor itself. + * + * Until we do the above proper fix, the best we can do is *only* + * use this for `resolveSymlinks`. + * + * This variable does not include a trailing directory separator. + * Rather a directory separate must come after the prefix. For + * example, the empty string means the root path, i.e. any absolute + * path points within the source accessor. + */ + std::string ownLocationForSymlinkResolution = ""; + /** * A string that uniquely represents the contents of this * accessor. This is used for caching lookups (see `fetchToStore()`). @@ -222,4 +255,10 @@ ref makeMountedSourceAccessor(std::map makeUnionSourceAccessor(std::vector> && accessors); +/** + * Creates a new source accessor which is confined to the subdirectory + * of the given source accessor. + */ +ref projectSubdirSourceAccessor(ref, CanonPath subdirectory); + } diff --git a/src/libutil/subdir-source-accessor.cc b/src/libutil/subdir-source-accessor.cc new file mode 100644 index 00000000000..f033fcd1ad3 --- /dev/null +++ b/src/libutil/subdir-source-accessor.cc @@ -0,0 +1,56 @@ +#include "source-accessor.hh" + +namespace nix { + +struct SubdirSourceAccessor : SourceAccessor +{ + ref parent; + + CanonPath subdirectory; + + SubdirSourceAccessor(ref && parent, CanonPath && subdirectory) + : parent(std::move(parent)) + , subdirectory(std::move(subdirectory)) + { + displayPrefix.clear(); + + ownLocationForSymlinkResolution = this->parent->ownLocationForSymlinkResolution + subdirectory.abs(); + } + + std::string readFile(const CanonPath & path) override + { + return parent->readFile(subdirectory / path); + } + + bool pathExists(const CanonPath & path) override + { + return parent->pathExists(subdirectory / path); + } + + std::optional maybeLstat(const CanonPath & path) override + { + return parent->maybeLstat(subdirectory / path); + } + + DirEntries readDirectory(const CanonPath & path) override + { + return parent->readDirectory(subdirectory / path); + } + + std::string readLink(const CanonPath & path) override + { + return parent->readLink(subdirectory / path); + } + + std::string showPath(const CanonPath & path) override + { + return displayPrefix + parent->showPath(subdirectory / path) + displaySuffix; + } +}; + +ref projectSubdirSourceAccessor(ref parent, CanonPath subdirectory) +{ + return make_ref(std::move(parent), std::move(subdirectory)); +} + +} diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index d182b1eee57..d34de0276fe 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -563,7 +563,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise) #endif if (!hashGiven) { HashResult hash = hashPath( - {store->getFSAccessor(false), CanonPath { store->printStorePath(info->path) }}, + {store->getFSAccessor(false), CanonPath { info->path.to_string() }}, FileSerialisationMethod::NixArchive, HashAlgorithm::SHA256); info->narHash = hash.first; info->narSize = hash.second; diff --git a/src/nix/cat.cc b/src/nix/cat.cc index e0179c3486c..1dd8c550980 100644 --- a/src/nix/cat.cc +++ b/src/nix/cat.cc @@ -7,21 +7,21 @@ using namespace nix; struct MixCat : virtual Args { - std::string path; - - void cat(ref accessor) + void cat(ref accessor, CanonPath path) { - auto st = accessor->lstat(CanonPath(path)); + auto st = accessor->lstat(path); if (st.type != SourceAccessor::Type::tRegular) - throw Error("path '%1%' is not a regular file", path); + throw Error("path '%1%' is not a regular file", path.abs()); stopProgressBar(); - writeFull(getStandardOutput(), accessor->readFile(CanonPath(path))); + writeFull(getStandardOutput(), accessor->readFile(path)); } }; struct CmdCatStore : StoreCommand, MixCat { + std::string path; + CmdCatStore() { expectArgs({ @@ -45,7 +45,8 @@ struct CmdCatStore : StoreCommand, MixCat void run(ref store) override { - cat(store->getFSAccessor()); + auto [storePath, rest] = store->toStorePath(path); + cat(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); } }; @@ -53,6 +54,8 @@ struct CmdCatNar : StoreCommand, MixCat { Path narPath; + std::string path; + CmdCatNar() { expectArgs({ @@ -77,7 +80,7 @@ struct CmdCatNar : StoreCommand, MixCat void run(ref store) override { - cat(makeNarAccessor(readFile(narPath))); + cat(makeNarAccessor(readFile(narPath)), CanonPath{path}); } }; diff --git a/src/nix/env.cc b/src/nix/env.cc index 832320320ae..0938744f6b8 100644 --- a/src/nix/env.cc +++ b/src/nix/env.cc @@ -87,8 +87,8 @@ struct CmdShell : InstallablesCommand, MixEnvironment if (true) pathAdditions.push_back(store->printStorePath(path) + "/bin"); - auto propPath = accessor->resolveSymlinks( - CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages"); + auto propPath = + accessor->resolveSymlinks(CanonPath(path.to_string()) / "nix-support" / "propagated-user-env-packages"); if (auto st = accessor->maybeLstat(propPath); st && st->type == SourceAccessor::tRegular) { for (auto & p : tokenizeString(accessor->readFile(propPath))) todo.push(store->parseStorePath(p)); diff --git a/src/nix/ls.cc b/src/nix/ls.cc index 63f97f2d3b6..e7f17485464 100644 --- a/src/nix/ls.cc +++ b/src/nix/ls.cc @@ -8,8 +8,6 @@ using namespace nix; struct MixLs : virtual Args, MixJSON { - std::string path; - bool recursive = false; bool verbose = false; bool showDirectory = false; @@ -38,7 +36,7 @@ struct MixLs : virtual Args, MixJSON }); } - void listText(ref accessor) + void listText(ref accessor, CanonPath path) { std::function doPath; @@ -77,26 +75,27 @@ struct MixLs : virtual Args, MixJSON showFile(curPath, relPath); }; - auto path2 = CanonPath(path); - auto st = accessor->lstat(path2); - doPath(st, path2, - st.type == SourceAccessor::Type::tDirectory ? "." : path2.baseName().value_or(""), + auto st = accessor->lstat(path); + doPath(st, path, + st.type == SourceAccessor::Type::tDirectory ? "." : path.baseName().value_or(""), showDirectory); } - void list(ref accessor) + void list(ref accessor, CanonPath path) { if (json) { if (showDirectory) throw UsageError("'--directory' is useless with '--json'"); - logger->cout("%s", listNar(accessor, CanonPath(path), recursive)); + logger->cout("%s", listNar(accessor, path, recursive)); } else - listText(accessor); + listText(accessor, std::move(path)); } }; struct CmdLsStore : StoreCommand, MixLs { + std::string path; + CmdLsStore() { expectArgs({ @@ -120,7 +119,8 @@ struct CmdLsStore : StoreCommand, MixLs void run(ref store) override { - list(store->getFSAccessor()); + auto [storePath, rest] = store->toStorePath(path); + list(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); } }; @@ -128,6 +128,8 @@ struct CmdLsNar : Command, MixLs { Path narPath; + std::string path; + CmdLsNar() { expectArgs({ @@ -152,7 +154,7 @@ struct CmdLsNar : Command, MixLs void run() override { - list(makeNarAccessor(readFile(narPath))); + list(makeNarAccessor(readFile(narPath)), CanonPath{path}); } }; diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index e299585ff88..9c6289e1137 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -175,7 +175,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions struct BailOut { }; printNode = [&](Node & node, const std::string & firstPad, const std::string & tailPad) { - CanonPath pathS(store->printStorePath(node.path)); + CanonPath pathS(node.path.to_string()); assert(node.dist != inf); if (precise) { diff --git a/tests/functional/shell.sh b/tests/functional/shell.sh index 51032ff1b75..bcb3c51630b 100755 --- a/tests/functional/shell.sh +++ b/tests/functional/shell.sh @@ -21,7 +21,7 @@ nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2' nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' # Test that symlinks outside of the store don't work. -expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "is not in the Nix store" +expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "points outside source accessor" # Test that we're not setting any more environment variables than necessary. # For instance, we might set an environment variable temporarily to affect some