Merge pull request #15078 from xokdvium/backport-15072-to-2.33-maintenance

[Backport 2.33-maintenance] Fix destruction of DerivationBuilder implementations

src/libstore/build/derivation-building-goal.cc

@@ -563,8 +563,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             {
                 DerivationBuildingGoal & goal;
 
-                DerivationBuildingGoalCallbacks(
-                    DerivationBuildingGoal & goal, std::unique_ptr<DerivationBuilder> & 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<DerivationBuildingGoalCallbacks>(*this, builder),
-                                            std::move(params),
-                                            *externalBuilder)
-                                      : makeDerivationBuilder(
-                                            *localStoreP,
-                                            std::make_unique<DerivationBuildingGoalCallbacks>(*this, builder),
-                                            std::move(params));
+            builder =
+                externalBuilder
+                    ? makeExternalDerivationBuilder(
+                          *localStoreP,
+                          std::make_unique<DerivationBuildingGoalCallbacks>(*this),
+                          std::move(params),
+                          *externalBuilder)
+                    : makeDerivationBuilder(
+                          *localStoreP, std::make_unique<DerivationBuildingGoalCallbacks>(*this), std::move(params));
         }
 
         if (auto builderOutOpt = builder->startBuild()) {

src/libstore/include/nix/store/build/derivation-builder.hh

@@ -189,15 +189,22 @@ struct ExternalBuilder
     std::vector<std::string> args;
 };
 
+struct DerivationBuilderDeleter
+{
+    void operator()(DerivationBuilder * builder) noexcept;
+};
+
+using DerivationBuilderUnique = std::unique_ptr<DerivationBuilder, DerivationBuilderDeleter>;
+
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
-std::unique_ptr<DerivationBuilder> makeDerivationBuilder(
+DerivationBuilderUnique makeDerivationBuilder(
     LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> miscMethods, DerivationBuilderParams params);
 
 /**
  * @param handler Must be chosen such that it supports the given
  * derivation.
  */
-std::unique_ptr<DerivationBuilder> makeExternalDerivationBuilder(
+DerivationBuilderUnique makeExternalDerivationBuilder(
     LocalStore & store,
     std::unique_ptr<DerivationBuilderCallbacks> miscMethods,
     DerivationBuilderParams params,

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<HookInstance> hook;
 
-    std::unique_ptr<DerivationBuilder> builder;
+    DerivationBuilderUnique builder;
 #endif
 
     BuildMode buildMode;

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<DerivationBuilder> makeDerivationBuilder(
+void DerivationBuilderDeleter::operator()(DerivationBuilder * builder) noexcept
+{
+    if (!builder) /* Idempotent and handles nullptr as any deleter must. */
+        return;
+
+    if (auto builderImpl = dynamic_cast<DerivationBuilderImpl *>(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<DerivationBuilder, DerivationBuilderDeleter> makeDerivationBuilder(
     LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> miscMethods, DerivationBuilderParams params)
 {
     bool useSandbox = false;
@@ -2013,17 +2029,19 @@ std::unique_ptr<DerivationBuilder> makeDerivationBuilder(
         throw Error("feature 'uid-range' is only supported in sandboxed builds");
 
 #ifdef __APPLE__
-    return std::make_unique<DarwinDerivationBuilder>(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<ChrootLinuxDerivationBuilder>(store, std::move(miscMethods), std::move(params));
+        return DerivationBuilderUnique(
+            new ChrootLinuxDerivationBuilder(store, std::move(miscMethods), std::move(params)));
 
-    return std::make_unique<LinuxDerivationBuilder>(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<DerivationBuilderImpl>(store, std::move(miscMethods), std::move(params));
+    return DerivationBuilderUnique(new DerivationBuilderImpl(store, std::move(miscMethods), std::move(params)));
 #endif
 }
 

src/libstore/unix/build/external-derivation-builder.cc

@@ -106,13 +106,14 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl
     }
 };
 
-std::unique_ptr<DerivationBuilder> makeExternalDerivationBuilder(
+DerivationBuilderUnique makeExternalDerivationBuilder(
     LocalStore & store,
     std::unique_ptr<DerivationBuilderCallbacks> miscMethods,
     DerivationBuilderParams params,
     const ExternalBuilder & handler)
 {
-    return std::make_unique<ExternalDerivationBuilder>(store, std::move(miscMethods), std::move(params), handler);
+    return DerivationBuilderUnique(
+        new ExternalDerivationBuilder(store, std::move(miscMethods), std::move(params), handler));
 }
 
 } // namespace nix