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 <nixpkgs> { };
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 b752c5cb64)
(cherry picked from commit 70ecd8c8a9)

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

@@ -717,8 +717,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             {
                 DerivationBuildingGoal & goal;
 
-                DerivationBuildingGoalCallbacks(
-                    DerivationBuildingGoal & goal, std::unique_ptr<DerivationBuilder> & 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<DerivationBuildingGoalCallbacks>(*this, builder),
+                std::make_unique<DerivationBuildingGoalCallbacks>(*this),
                 DerivationBuilderParams{
                     .drvPath = drvPath,
                     .buildResult = buildResult,

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<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);
 #endif
 

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

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<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)
 {
     if (auto builder = ExternalDerivationBuilder::newIfSupported(store, miscMethods, params))
@@ -1969,17 +1985,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

@@ -15,14 +15,14 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl
         experimentalFeatureSettings.require(Xp::ExternalBuilders);
     }
 
-    static std::unique_ptr<ExternalDerivationBuilder> newIfSupported(
+    static std::unique_ptr<ExternalDerivationBuilder, DerivationBuilderDeleter> newIfSupported(
         LocalStore & store, std::unique_ptr<DerivationBuilderCallbacks> & miscMethods, DerivationBuilderParams & params)
     {
         for (auto & handler : settings.externalBuilders.get()) {
             for (auto & system : handler.systems)
                 if (params.drv.platform == system)
-                    return std::make_unique<ExternalDerivationBuilder>(
-                        store, std::move(miscMethods), std::move(params), handler);
+                    return std::unique_ptr<ExternalDerivationBuilder, DerivationBuilderDeleter>(
+                        new ExternalDerivationBuilder(store, std::move(miscMethods), std::move(params), handler));
         }
         return {};
     }