From b3e3be1e644941a29a422e2853a4b3ec435cf633 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Mon, 10 Nov 2025 21:12:07 +0300 Subject: [PATCH] Restore isAllowed check in ChrootLinuxDerivationBuilder This early return was lost in d4ef822add1074483627c5dbbaa9077f15daf7bc. By doing some https://en.wikipedia.org/wiki/Non-virtual_interface_pattern, we can ensure that we don't make this mistake again --- implementations are no longer responsible for implementing the caching/memoization mechanism. (cherry picked from commit 496e43ec72643ad4fc48ce15e6b7220763e823a8) --- .../include/nix/store/restricted-store.hh | 16 +++++++++++++++- src/libstore/unix/build/derivation-builder.cc | 7 ++----- .../unix/build/linux-derivation-builder.cc | 5 ++++- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/libstore/include/nix/store/restricted-store.hh b/src/libstore/include/nix/store/restricted-store.hh index 8bbb2ff54..62cac3856 100644 --- a/src/libstore/include/nix/store/restricted-store.hh +++ b/src/libstore/include/nix/store/restricted-store.hh @@ -52,7 +52,21 @@ struct RestrictionContext * Add 'path' to the set of paths that may be referenced by the * outputs, and make it appear in the sandbox. */ - virtual void addDependency(const StorePath & path) = 0; + void addDependency(const StorePath & path) + { + if (isAllowed(path)) + return; + addDependencyImpl(path); + } + +protected: + + /** + * This is the underlying implementation to be defined. The caller + * will ensure that this is only called on newly added dependencies, + * and that idempotent calls are a no-op. + */ + virtual void addDependencyImpl(const StorePath & path) = 0; }; /** diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index f94bb40cc..5d26ab8fd 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -284,7 +284,7 @@ public: protected: - void addDependency(const StorePath & path) override; + void addDependencyImpl(const StorePath & path) override; /** * Make a file owned by the builder. @@ -1167,11 +1167,8 @@ void DerivationBuilderImpl::stopDaemon() daemonSocket.close(); } -void DerivationBuilderImpl::addDependency(const StorePath & path) +void DerivationBuilderImpl::addDependencyImpl(const StorePath & path) { - if (isAllowed(path)) - return; - addedPaths.insert(path); } diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc index 0d9dc4a85..d7ae35f61 100644 --- a/src/libstore/unix/build/linux-derivation-builder.cc +++ b/src/libstore/unix/build/linux-derivation-builder.cc @@ -681,8 +681,11 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu DerivationBuilderImpl::killSandbox(getStats); } - void addDependency(const StorePath & path) override + void addDependencyImpl(const StorePath & path) override { + if (isAllowed(path)) + return; + auto [source, target] = ChrootDerivationBuilder::addDependencyPrep(path); /* Bind-mount the path into the sandbox. This requires