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.

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

@@ -654,7 +654,7 @@ Goal::Co DerivationBuildingGoal::buildLocally(
     };
 
     std::unique_ptr<Activity> actLock;
-    std::unique_ptr<DerivationBuilder> builder;
+    DerivationBuilderUnique builder;
     Descriptor builderOut;
 
     // Will continue here while waiting for a build user below

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

@@ -190,15 +190,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/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<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;
@@ -2025,17 +2041,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