From b752c5cb64c2675dc51aef6eb6b97d16a2a477e4 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. --- .../build/derivation-building-goal.cc | 2 +- .../nix/store/build/derivation-builder.hh | 11 +++++-- src/libstore/unix/build/derivation-builder.cc | 32 +++++++++++++++---- .../unix/build/external-derivation-builder.cc | 5 +-- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 1a0111107..a1fe2adb4 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -654,7 +654,7 @@ Goal::Co DerivationBuildingGoal::buildLocally( }; std::unique_ptr actLock; - std::unique_ptr builder; + DerivationBuilderUnique builder; Descriptor builderOut; // Will continue here while waiting for a build user below diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index f19b7002f..23d368bb1 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -190,15 +190,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/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 8e5dc0a72..ce3d5b729 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 (...) { @@ -1974,7 +1977,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; @@ -2025,17 +2041,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 9e11c1c0a..96301af1d 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