From 3cd8c1e53d6409f1b2d39bd74de775e9f07ac928 Mon Sep 17 00:00:00 2001
From: Dominique Martinet <asmadeus@codewreck.org>
Date: Sun, 12 Jan 2025 15:01:49 +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 enqueuing 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.

[ lix porting note: nix does not name threads so the patch has been
adapted to not pass thread name ]

Link: https://git.lix.systems/lix-project/lix/issues/618
Link: https://gerrit.lix.systems/c/lix/+/2355
(cherry picked from commit afac093b347dbc8a745d9fbee92257bd1623426f)
---
 src/libstore/store-api.cc  | 8 ++------
 src/libutil/thread-pool.hh | 5 ++++-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index fc8d51c39..4e3faa92d 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -246,9 +246,7 @@ void Store::addMultipleToStore(
         act.progress(nrDone, pathsToCopy.size(), nrRunning, nrFailed);
     };
 
-    ThreadPool pool;
-
-    processGraph<StorePath>(pool,
+    processGraph<StorePath>(
         storePathsToAdd,
 
         [&](const StorePath & path) {
@@ -1028,12 +1026,10 @@ std::map<StorePath, StorePath> copyPaths(
     }
     auto pathsMap = copyPaths(srcStore, dstStore, storePaths, repair, checkSigs, substitute);
 
-    ThreadPool pool;
-
     try {
         // Copy the realisation closure
         processGraph<Realisation>(
-            pool, Realisation::closure(srcStore, toplevelRealisations),
+            Realisation::closure(srcStore, toplevelRealisations),
             [&](const Realisation & current) -> std::set<Realisation> {
                 std::set<Realisation> children;
                 for (const auto & [drvOutput, _] : current.dependentRealisations) {
diff --git a/src/libutil/thread-pool.hh b/src/libutil/thread-pool.hh
index 02765badc..dc056481a 100644
--- a/src/libutil/thread-pool.hh
+++ b/src/libutil/thread-pool.hh
@@ -83,7 +83,6 @@ private:
  */
 template<typename T>
 void processGraph(
-    ThreadPool & pool,
     const std::set<T> & nodes,
     std::function<std::set<T>(const T &)> getEdges,
     std::function<void(const T &)> processNode)
@@ -97,6 +96,10 @@ void processGraph(
 
     std::function<void(const T &)> worker;
 
+    /* Create pool last to ensure threads are stopped before other destructors
+     * run */
+    ThreadPool pool;
+
     worker = [&](const T & node) {
 
         {