From 0c32fb3fa2d66448615744b502d06b6dea21d66e Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Tue, 14 Oct 2025 23:58:18 +0300 Subject: [PATCH 1/4] treewide: Add Store::requireStoreObjectAccessor, simplify uses of getFSAccessor This is a simple wrapper around getFSAccessor that throws an InvalidPath error. This simplifies usage in callsites that only care about getting a non-null accessor. --- src/libfetchers/fetchers.cc | 3 +-- src/libfetchers/github.cc | 10 ++++++---- src/libfetchers/mercurial.cc | 4 +--- src/libfetchers/path.cc | 2 +- src/libstore/include/nix/store/store-api.hh | 20 +++++++++++++++++++- src/libstore/store-api.cc | 2 +- src/nix/cat.cc | 5 +---- src/nix/ls.cc | 5 +---- src/nix/nix-store/nix-store.cc | 2 +- src/nix/why-depends.cc | 2 +- 10 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index f697ec6f5..7c741a7a3 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -332,8 +332,7 @@ std::pair, Input> Input::getAccessorUnchecked(ref sto debug("using substituted/cached input '%s' in '%s'", to_string(), store->printStorePath(storePath)); - // We just ensured the store object was there - auto accessor = ref{store->getFSAccessor(storePath)}; + auto accessor = store->requireStoreObjectAccessor(storePath); accessor->fingerprint = getFingerprint(store); diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index a905bb384..2479a57d2 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -399,7 +399,8 @@ struct GitHubInputScheme : GitArchiveInputScheme Headers headers = makeHeadersWithAuthTokens(*input.settings, host, input); auto downloadResult = downloadFile(store, *input.settings, url, "source", headers); - auto json = nlohmann::json::parse(store->getFSAccessor(downloadResult.storePath)->readFile(CanonPath::root)); + auto json = nlohmann::json::parse( + store->requireStoreObjectAccessor(downloadResult.storePath)->readFile(CanonPath::root)); return RefInfo{ .rev = Hash::parseAny(std::string{json["sha"]}, HashAlgorithm::SHA1), @@ -473,7 +474,8 @@ struct GitLabInputScheme : GitArchiveInputScheme Headers headers = makeHeadersWithAuthTokens(*input.settings, host, input); auto downloadResult = downloadFile(store, *input.settings, url, "source", headers); - auto json = nlohmann::json::parse(store->getFSAccessor(downloadResult.storePath)->readFile(CanonPath::root)); + auto json = nlohmann::json::parse( + store->requireStoreObjectAccessor(downloadResult.storePath)->readFile(CanonPath::root)); if (json.is_array() && json.size() >= 1 && json[0]["id"] != nullptr) { return RefInfo{.rev = Hash::parseAny(std::string(json[0]["id"]), HashAlgorithm::SHA1)}; @@ -549,7 +551,7 @@ struct SourceHutInputScheme : GitArchiveInputScheme std::string refUri; if (ref == "HEAD") { auto downloadFileResult = downloadFile(store, *input.settings, fmt("%s/HEAD", base_url), "source", headers); - auto contents = nix::ref(store->getFSAccessor(downloadFileResult.storePath))->readFile(CanonPath::root); + auto contents = store->requireStoreObjectAccessor(downloadFileResult.storePath)->readFile(CanonPath::root); auto remoteLine = git::parseLsRemoteLine(getLine(contents).first); if (!remoteLine) { @@ -563,7 +565,7 @@ struct SourceHutInputScheme : GitArchiveInputScheme auto downloadFileResult = downloadFile(store, *input.settings, fmt("%s/info/refs", base_url), "source", headers); - auto contents = nix::ref(store->getFSAccessor(downloadFileResult.storePath))->readFile(CanonPath::root); + auto contents = store->requireStoreObjectAccessor(downloadFileResult.storePath)->readFile(CanonPath::root); std::istringstream is(contents); std::string line; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index bf460d9c6..41bf6e2aa 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -329,9 +329,7 @@ struct MercurialInputScheme : InputScheme Input input(_input); auto storePath = fetchToStore(store, input); - - // We just added it, it should be there. - auto accessor = ref{store->getFSAccessor(storePath)}; + auto accessor = store->requireStoreObjectAccessor(storePath); accessor->setPathDisplay("«" + input.to_string() + "»"); diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index aa0411ff9..c4b5e2f1e 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -138,7 +138,7 @@ struct PathInputScheme : InputScheme storePath = store->addToStoreFromDump(*src, "source"); } - auto accessor = ref{store->getFSAccessor(*storePath)}; + auto accessor = store->requireStoreObjectAccessor(*storePath); // To prevent `fetchToStore()` copying the path again to Nix // store, pre-create an entry in the fetcher cache. diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index 1131ec975..5c96c5f80 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -724,10 +724,28 @@ public: * the Nix store. * * @return nullptr if the store doesn't contain an object at the - * givine path. + * given path. */ virtual std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath = true) = 0; + /** + * Get an accessor for the store object or throw an Error if it's invalid or + * doesn't exist. + * + * @throws InvalidPath if the store object doesn't exist or (if requireValidPath = true) is + * invalid. + */ + [[nodiscard]] ref requireStoreObjectAccessor(const StorePath & path, bool requireValidPath = true) + { + auto accessor = getFSAccessor(path, requireValidPath); + if (!accessor) { + throw InvalidPath( + requireValidPath ? "path '%1%' is not a valid store path" : "store path '%1%' does not exist", + printStorePath(path)); + } + return ref{accessor}; + } + /** * Repair the contents of the given path by redownloading it using * a substituter (if available). diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 4ce6b15fa..1335eb76a 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1130,7 +1130,7 @@ Derivation Store::derivationFromPath(const StorePath & drvPath) static Derivation readDerivationCommon(Store & store, const StorePath & drvPath, bool requireValidPath) { - auto accessor = store.getFSAccessor(drvPath, requireValidPath); + auto accessor = store.requireStoreObjectAccessor(drvPath, requireValidPath); try { return parseDerivation(store, accessor->readFile(CanonPath::root), Derivation::nameFromPath(drvPath)); } catch (FormatError & e) { diff --git a/src/nix/cat.cc b/src/nix/cat.cc index 145336723..effe544e6 100644 --- a/src/nix/cat.cc +++ b/src/nix/cat.cc @@ -41,10 +41,7 @@ struct CmdCatStore : StoreCommand, MixCat void run(ref store) override { auto [storePath, rest] = store->toStorePath(path); - auto accessor = store->getFSAccessor(storePath); - if (!accessor) - throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); - cat(ref{std::move(accessor)}, CanonPath{rest}); + cat(store->requireStoreObjectAccessor(storePath), CanonPath{rest}); } }; diff --git a/src/nix/ls.cc b/src/nix/ls.cc index 4952d5243..5cdfc2c0f 100644 --- a/src/nix/ls.cc +++ b/src/nix/ls.cc @@ -115,10 +115,7 @@ struct CmdLsStore : StoreCommand, MixLs void run(ref store) override { auto [storePath, rest] = store->toStorePath(path); - auto accessor = store->getFSAccessor(storePath); - if (!accessor) - throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); - list(ref{std::move(accessor)}, CanonPath{rest}); + list(store->requireStoreObjectAccessor(storePath), CanonPath{rest}); } }; diff --git a/src/nix/nix-store/nix-store.cc b/src/nix/nix-store/nix-store.cc index f8078426c..313a6398c 100644 --- a/src/nix/nix-store/nix-store.cc +++ b/src/nix/nix-store/nix-store.cc @@ -603,7 +603,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise) #endif if (!hashGiven) { HashResult hash = hashPath( - {ref{store->getFSAccessor(info->path, false)}}, + {store->requireStoreObjectAccessor(info->path, /*requireValidPath=*/false)}, FileSerialisationMethod::NixArchive, HashAlgorithm::SHA256); info->narHash = hash.hash; diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index 473827a93..dc30fabd7 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -207,7 +207,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions contain the reference. */ std::map hits; - auto accessor = store->getFSAccessor(node.path); + auto accessor = store->requireStoreObjectAccessor(node.path); auto visitPath = [&](this auto && recur, const CanonPath & p) -> void { auto st = accessor->maybeLstat(p); From 69c005e805859364bc98061852602d6ea2dd37c3 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 15 Oct 2025 00:15:48 +0300 Subject: [PATCH 2/4] libstore: Use getFSAccessor for store object in Worker::pathContentsGood We only care about the accessor for a single store object anyway, but the validity gets ignored. Also `pathExists(store.printStorePath(path))` is definitely incorrect since it confuses the logical location vs physical location in case of a chroot store. --- src/libstore/build/worker.cc | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 53175a8c4..d23c53e77 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -529,15 +529,9 @@ bool Worker::pathContentsGood(const StorePath & path) return i->second; printInfo("checking path '%s'...", store.printStorePath(path)); auto info = store.queryPathInfo(path); - bool res; - if (!pathExists(store.printStorePath(path))) - res = false; - else { - auto current = hashPath( - {store.getFSAccessor(), CanonPath(path.to_string())}, - FileIngestionMethod::NixArchive, - info->narHash.algo) - .first; + bool res = false; + if (auto accessor = store.getFSAccessor(path, /*requireValidPath=*/false)) { + auto current = hashPath({ref{accessor}}, FileIngestionMethod::NixArchive, info->narHash.algo).first; Hash nullHash(HashAlgorithm::SHA256); res = info->narHash == nullHash || info->narHash == current; } From 918a3cebaa439d2baba46c9ca7d0f1fc6da0db2b Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 15 Oct 2025 00:25:14 +0300 Subject: [PATCH 3/4] libexpr: Use Store::requireStoreObjectAccessor instead or toRealPath in fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This forces the code to go through proper abstractions instead of the raw filesystem API. This issue is evident from this reproducer: nix eval --expr 'builtins.fetchurl { url = "https://example.com"; sha256 = ""; }' --json --eval-store "dummy://?read-only=false" error: … while calling the 'fetchurl' builtin at «string»:1:1: 1| builtins.fetchurl { url = "https://example.com"; sha256 = ""; } | ^ error: opening file '/nix/store/r4f87yrl98f2m6v9z8ai2rbg4qwlcakq-example.com': No such file or directory --- src/libexpr/primops/fetchTree.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 48c03f177..ad76af5b5 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -588,7 +588,11 @@ static void fetch( if (expectedHash) { auto hash = unpack ? state.store->queryPathInfo(storePath)->narHash - : hashFile(HashAlgorithm::SHA256, state.store->toRealPath(storePath)); + : hashPath( + {state.store->requireStoreObjectAccessor(storePath)}, + FileSerialisationMethod::Flat, + HashAlgorithm::SHA256) + .hash; if (hash != *expectedHash) { state .error( From 0347958dd2c53763146e0227a1fbf6ffaa3d2c86 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 15 Oct 2025 00:48:39 +0300 Subject: [PATCH 4/4] nix/develop: Remove usage of toRealPath, replace with SourceAccessor --- src/nix/develop.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/nix/develop.cc b/src/nix/develop.cc index f78eee59a..28d0a7080 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -299,11 +299,9 @@ static StorePath getDerivationEnvironment(ref store, ref evalStore for (auto & [_0, optPath] : evalStore->queryPartialDerivationOutputMap(shellDrvPath)) { assert(optPath); - auto & outPath = *optPath; - assert(store->isValidPath(outPath)); - auto outPathS = store->toRealPath(outPath); - if (lstat(outPathS).st_size) - return outPath; + auto accessor = evalStore->requireStoreObjectAccessor(*optPath); + if (auto st = accessor->maybeLstat(CanonPath::root); st && st->fileSize.value_or(0)) + return *optPath; } throw Error("get-env.sh failed to produce an environment"); @@ -502,7 +500,9 @@ struct Common : InstallableCommand, MixProfile debug("reading environment file '%s'", strPath); - return {BuildEnvironment::parseJSON(readFile(store->toRealPath(shellOutPath))), strPath}; + return { + BuildEnvironment::parseJSON(store->requireStoreObjectAccessor(shellOutPath)->readFile(CanonPath::root)), + strPath}; } };