From 70ecd8c8a9123bb6e79a19559fc1b5a5310ddbc8 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) --- .../build/derivation-building-goal.cc | 21 ++++++------ .../nix/store/build/derivation-builder.hh | 11 +++++-- .../store/build/derivation-building-goal.hh | 3 +- src/libstore/unix/build/derivation-builder.cc | 32 +++++++++++++++---- .../unix/build/external-derivation-builder.cc | 5 +-- 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 12344a8c2..6c43dcc98 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -563,8 +563,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { DerivationBuildingGoal & goal; - DerivationBuildingGoalCallbacks( - DerivationBuildingGoal & goal, std::unique_ptr & builder) + DerivationBuildingGoalCallbacks(DerivationBuildingGoal & goal) : goal{goal} { } @@ -632,15 +631,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* If we have to wait and retry (see below), then `builder` will already be created, so we don't need to create it again. */ - builder = externalBuilder ? makeExternalDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params), - *externalBuilder) - : makeDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params)); + builder = + externalBuilder + ? makeExternalDerivationBuilder( + *localStoreP, + std::make_unique(*this), + std::move(params), + *externalBuilder) + : makeDerivationBuilder( + *localStoreP, std::make_unique(*this), std::move(params)); } if (auto builderOutOpt = builder->startBuild()) { diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index f28fd8e56..ca6319855 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -189,15 +189,22 @@ struct ExternalBuilder std::vector args; }; +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); /** * @param handler Must be chosen such that it supports the given * derivation. */ -std::unique_ptr makeExternalDerivationBuilder( +DerivationBuilderUnique makeExternalDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params, 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 be95c796b..e745c12c7 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" @@ -89,7 +90,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 eb0bf5757..360b6a484 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -98,10 +98,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 (...) { @@ -1962,7 +1965,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) { bool useSandbox = false; @@ -2013,17 +2029,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 7ddb6e093..6c1a00f91 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -106,13 +106,14 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl } }; -std::unique_ptr makeExternalDerivationBuilder( +DerivationBuilderUnique makeExternalDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params, const ExternalBuilder & handler) { - return std::make_unique(store, std::move(miscMethods), std::move(params), handler); + return DerivationBuilderUnique( + new ExternalDerivationBuilder(store, std::move(miscMethods), std::move(params), handler)); } } // namespace nix