From 4737d8b65e9055fad7f1cef3cbce05b2a75a9d50 Mon Sep 17 00:00:00 2001
From: Dominique Martinet <asmadeus@codewreck.org>
Date: Sat, 11 Jan 2025 12:30:02 +0900
Subject: [PATCH] libutil: thread-pool: ensure threads finished on error

This fixes segfaults with nix copy when there was an error processing
addMultipleToStore.

Running with ASAN/TSAN pointed at an use-after-free with threads from
the pool accessing the graph declared in processGraph after the function
was exiting and destructing the variables.

It turns out that if there is an error before pool.process() is called,
for example while we are still enqueueing tasks, then pool.process()
isn't called and threads are still left to run.

By creating the pool last we ensure that it is stopped first before
running other destructors even if an exception happens early.

fixes #618

Change-Id: I42a355f632aa0354df94c5d5d8cbe7ab5196c9a6
---
 lix/libstore/store-api.cc  | 8 ++------
 lix/libutil/thread-pool.hh | 7 ++++++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lix/libstore/store-api.cc b/lix/libstore/store-api.cc
index e7e030a55..aca310fec 100644
--- a/lix/libstore/store-api.cc
+++ b/lix/libstore/store-api.cc
@@ -306,9 +306,7 @@ void Store::addMultipleToStore(
         act.progress(nrDone, pathsToCopy.size(), nrRunning, nrFailed);
     };
 
-    ThreadPool pool{"addMultipleToStore pool"};
-
-    processGraph<StorePath>(pool,
+    processGraph<StorePath>("addMultipleToStore pool",
         storePathsToAdd,
 
         [&](const StorePath & path) {
@@ -1137,12 +1135,10 @@ std::map<StorePath, StorePath> copyPaths(
     }
     auto pathsMap = copyPaths(srcStore, dstStore, storePaths, repair, checkSigs, substitute);
 
-    ThreadPool pool{"copyPaths pool"};
-
     try {
         // Copy the realisation closure
         processGraph<Realisation>(
-            pool, Realisation::closure(srcStore, toplevelRealisations),
+            "copyPaths pool", Realisation::closure(srcStore, toplevelRealisations),
             [&](const Realisation & current) -> std::set<Realisation> {
                 std::set<Realisation> children;
                 for (const auto & [drvOutput, _] : current.dependentRealisations) {
diff --git a/lix/libutil/thread-pool.hh b/lix/libutil/thread-pool.hh
index 089fdd3f3..9db56805a 100644
--- a/lix/libutil/thread-pool.hh
+++ b/lix/libutil/thread-pool.hh
@@ -85,7 +85,7 @@ private:
  */
 template<typename T>
 void processGraph(
-    ThreadPool & pool,
+    const char *poolName,
     const std::set<T> & nodes,
     std::function<std::set<T>(const T &)> getEdges,
     std::function<void(const T &)> processNode)
@@ -99,6 +99,11 @@ void processGraph(
 
     std::function<void(const T &)> worker;
 
+    /* Create pool last to ensure threads are stopped before other destructors
+     * run */
+    ThreadPool pool{poolName};
+
+
     worker = [&](const T & node) {
 
         {