From f5ed833482e2fd4851c6a34b2d084c5ad3824495 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 7 Oct 2026 13:42:55 +0200 Subject: [PATCH] Fix crash in the GC roots server on fd reuse The GC roots server started a thread per client connection and only afterwards inserted it into the `connections` map keyed by the client fd. The client thread's cleanup handler erases its own entry from that map, so if a client disconnected immediately, the cleanup could run before the insert. The cleanup then found nothing to erase, the fd was closed, and the server inserted a finished-but-joinable thread under a now-closed fd number. When a later client got the same fd number from accept(), the server tried to insert under an existing key. std::map::insert doesn't overwrite, so the temporary pair holding the new (joinable) std::thread was destroyed, which calls std::terminate(). This showed up in Sentry as a SIGABRT in nix-daemon with no exception in flight, and another thread blocked on the `connections` lock in its cleanup handler. Fix this by holding the `connections` lock while starting the client thread and inserting it. The cleanup handler can't run until the entry exists, and the fd is only closed after the cleanup, so fd numbers can no longer collide. Also assert that the insert didn't hit an existing key. Assisted-by: Claude Fable 5.1 --- src/libstore/gc.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 30242a5db3d7..f5ea569cda24 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -465,7 +465,17 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) debug("GC roots server accepted new client"); - /* Process the connection in a separate thread. */ + /* Process the connection in a separate thread. Hold + the `connections` lock while starting the thread + and registering it, so that the thread's cleanup + handler (which removes it from `connections`) + can't run before it has been inserted. Otherwise + a client that disconnects immediately would leave + a stale entry, and a later client that gets the + same fd number would cause a duplicate key insert, + destroying a joinable std::thread and thus calling + std::terminate(). */ + auto conn(connections.lock()); auto fdClient_ = fdClient.get(); std::thread clientThread([&, fdClient = std::move(fdClient)]() { Finally cleanup([&]() { @@ -516,7 +526,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } }); - connections.lock()->insert({fdClient_, std::move(clientThread)}); + auto [it, inserted] = conn->insert({fdClient_, std::move(clientThread)}); + assert(inserted); } } });