From e98041bbb8da700cb192cf784c9a1ed3066403bf Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Feb 2025 18:46:37 -0500 Subject: [PATCH] `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/libfetchers/store-path-accessor.cc | 6 +-- src/libstore/build/worker.cc | 2 +- src/libstore/local-fs-store.cc | 30 ++++++++------ 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 +- 16 files changed, 173 insertions(+), 49 deletions(-) create mode 100644 src/libutil/subdir-source-accessor.cc diff --git a/src/libfetchers/store-path-accessor.cc b/src/libfetchers/store-path-accessor.cc index 528bf2a4f517..9d3b7c4ec093 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 b765fc2a002e..cb6ce028e295 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/local-fs-store.cc b/src/libstore/local-fs-store.cc index 5449b20eb3b3..4da7e14df6f6 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -33,30 +33,32 @@ 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())) - 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 +66,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 9fa68303f2ca..8edeba571690 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1099,7 +1099,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 7e360b5fef13..616c8443a14b 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 236622eae371..204f22eb58de 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 b6a33135a9b4..3ee73d3519da 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 78f038cf3772..2f5054b49670 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 b79f8af92cf0..5abbad52fa8f 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()`). @@ -216,4 +249,10 @@ ref makeFSSourceAccessor(std::filesystem::path root); ref makeMountedSourceAccessor(std::map> mounts); +/** + * 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 000000000000..f033fcd1ad30 --- /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 d182b1eee574..d34de0276fe5 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 e0179c3486c8..1dd8c5509803 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 832320320aec..0938744f6b89 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 63f97f2d3b66..e7f174854644 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 e299585ff88e..9c6289e11378 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 51032ff1b75f..bcb3c51630b5 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