From 3ca2ce200f7dc1dfde80561abe2624422ba5dda8 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sat, 24 Jan 2026 23:31:11 +0300 Subject: [PATCH] Fix destruction of DerivationBuilder implementations This unsures that we call the correct virtual functions when destroying a particular DerivationBuilder. Usually the order of destructors is in the reverse order of inheritance: ChrootLinuxDerivationBuilder -> ChrootDerivationBuilder -> DerivationBuilderImpl autoDelChroot was being destroyed before the DerivationBuilderImpl::killChild was run and it would fail to clean up the chroot directory, since there were still processes writing to it. Note that ChrootLinuxDerivationBuilder::killSandbox was never run in the interrupted case at all, since virtual functions in destructors do not call derived class methods. I could reproduce the issue with the following derivation: let pkgs = import { }; in pkgs.runCommand "chroot-cleanup-race" { } '' mkdir -p $out for i in $(seq 1 200); do ( mkfifo $out/fifo$i cat $out/fifo$i > /dev/null & while true; do : > $out/file$i done ) & done sleep 0.05 echo done > $out/main '' While interrupting it manually when it would hang. Wrapping the unique pointer in a custom deleter function we can run all of the necessary clean up code consistently and calling the right virtual functions. Ideally we'd have a lint that bans the usage of virtual functions in destructors completely. (cherry picked from commit b752c5cb64c2675dc51aef6eb6b97d16a2a477e4) (cherry picked from commit 70ecd8c8a9123bb6e79a19559fc1b5a5310ddbc8) --- .../build/derivation-building-goal.cc | 5 ++- .../nix/store/build/derivation-builder.hh | 9 +++++- .../store/build/derivation-building-goal.hh | 3 +- src/libstore/unix/build/derivation-builder.cc | 32 +++++++++++++++---- .../unix/build/external-derivation-builder.cc | 6 ++-- 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 310b2bf84..2b938adf8 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -717,8 +717,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { DerivationBuildingGoal & goal; - DerivationBuildingGoalCallbacks( - DerivationBuildingGoal & goal, std::unique_ptr & builder) + DerivationBuildingGoalCallbacks(DerivationBuildingGoal & goal) : goal{goal} { } @@ -775,7 +774,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() already be created, so we don't need to create it again. */ builder = makeDerivationBuilder( *localStoreP, - std::make_unique(*this, builder), + std::make_unique(*this), DerivationBuilderParams{ .drvPath = drvPath, .buildResult = buildResult, diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 98b87ae64..37fd1ebfb 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -179,8 +179,15 @@ struct DerivationBuilder : RestrictionContext virtual bool killChild() = 0; }; +struct DerivationBuilderDeleter +{ + void operator()(DerivationBuilder * builder) noexcept; +}; + +using DerivationBuilderUnique = std::unique_ptr; + #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows -std::unique_ptr makeDerivationBuilder( +DerivationBuilderUnique makeDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params); #endif diff --git a/src/libstore/include/nix/store/build/derivation-building-goal.hh b/src/libstore/include/nix/store/build/derivation-building-goal.hh index edb496024..8f1674862 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -5,6 +5,7 @@ #include "nix/store/parsed-derivations.hh" #include "nix/store/derivation-options.hh" #include "nix/store/build/derivation-building-misc.hh" +#include "nix/store/build/derivation-builder.hh" #include "nix/store/outputs-spec.hh" #include "nix/store/store-api.hh" #include "nix/store/pathlocks.hh" @@ -82,7 +83,7 @@ private: */ std::unique_ptr hook; - std::unique_ptr builder; + DerivationBuilderUnique builder; #endif BuildMode buildMode; diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 1fa6607c1..7be8c8e43 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -92,10 +92,13 @@ public: { } - ~DerivationBuilderImpl() + /** + * Cleanup code to run when destroying any DerivationBuilderImpl implementation. + */ + void cleanupOnDestruction() noexcept { /* Careful: we should never ever throw an exception from a - destructor. */ + noexcept function. */ try { killChild(); } catch (...) { @@ -1915,7 +1918,20 @@ StorePath DerivationBuilderImpl::makeFallbackPath(const StorePath & path) namespace nix { -std::unique_ptr makeDerivationBuilder( +void DerivationBuilderDeleter::operator()(DerivationBuilder * builder) noexcept +{ + if (!builder) /* Idempotent and handles nullptr as any deleter must. */ + return; + + if (auto builderImpl = dynamic_cast(builder)) + /* Note that this might call into virtual functions, which we can't do in a destructor of + the DerivationBuilderImpl itself. */ + builderImpl->cleanupOnDestruction(); + + delete builder; +} + +std::unique_ptr makeDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params) { if (auto builder = ExternalDerivationBuilder::newIfSupported(store, miscMethods, params)) @@ -1969,17 +1985,19 @@ std::unique_ptr makeDerivationBuilder( throw Error("feature 'uid-range' is only supported in sandboxed builds"); #ifdef __APPLE__ - return std::make_unique(store, std::move(miscMethods), std::move(params), useSandbox); + return DerivationBuilderUnique( + new DarwinDerivationBuilder(store, std::move(miscMethods), std::move(params), useSandbox)); #elif defined(__linux__) if (useSandbox) - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique( + new ChrootLinuxDerivationBuilder(store, std::move(miscMethods), std::move(params))); - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique(new LinuxDerivationBuilder(store, std::move(miscMethods), std::move(params))); #else if (useSandbox) throw Error("sandboxing builds is not supported on this platform"); - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique(new DerivationBuilderImpl(store, std::move(miscMethods), std::move(params))); #endif } diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index 71cfd1a62..0fa9700df 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -15,14 +15,14 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl experimentalFeatureSettings.require(Xp::ExternalBuilders); } - static std::unique_ptr newIfSupported( + static std::unique_ptr newIfSupported( LocalStore & store, std::unique_ptr & miscMethods, DerivationBuilderParams & params) { for (auto & handler : settings.externalBuilders.get()) { for (auto & system : handler.systems) if (params.drv.platform == system) - return std::make_unique( - store, std::move(miscMethods), std::move(params), handler); + return std::unique_ptr( + new ExternalDerivationBuilder(store, std::move(miscMethods), std::move(params), handler)); } return {}; }