From 1929d84f9e9e58466efbd05ec518360d8ac1d098 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 27 Jun 2025 14:33:18 +0200 Subject: [PATCH 1/5] nix store delete: Give a more specific error message --- src/libstore/gc.cc | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 48467381d3f..680a9f0e178 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -622,10 +622,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) GC root. Any new roots will be sent to our socket. */ Roots tempRoots; findTempRoots(tempRoots, true); - for (auto & root : tempRoots) { + for (auto & root : tempRoots) _shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); - roots.insert(root.first); - } /* Synchronisation point for testing, see tests/functional/gc-non-blocking.sh. */ if (auto p = getEnv("_NIX_TEST_GC_SYNC_2")) @@ -718,19 +716,31 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* If this is a root, bail out. */ if (roots.count(*path)) { + if (options.action == GCOptions::gcDeleteSpecific) + throw Error( + "Cannot delete path '%s' because it's a GC root.", + printStorePath(start)); debug("cannot delete '%s' because it's a root", printStorePath(*path)); return markAlive(); } if (options.action == GCOptions::gcDeleteSpecific && !options.pathsToDelete.count(*path)) - return; + { + throw Error( + "Cannot delete path '%s' because it's referenced by path '%s'.", + printStorePath(start), + printStorePath(*path)); + } { auto hashPart = std::string(path->hashPart()); auto shared(_shared.lock()); if (shared->tempRoots.count(hashPart)) { - debug("cannot delete '%s' because it's a temporary root", printStorePath(*path)); + if (options.action == GCOptions::gcDeleteSpecific) + throw Error( + "Cannot delete path '%s' because it's in use by a Nix process.", + printStorePath(start)); return markAlive(); } shared->pending = hashPart; @@ -789,12 +799,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { deleteReferrersClosure(i); - if (!dead.count(i)) - throw Error( - "Cannot delete path '%1%' since it is still alive. " - "To find out why, use: " - "nix-store --query --roots and nix-store --query --referrers", - printStorePath(i)); + assert(dead.count(i)); } } else if (options.maxFreed > 0) { From 59700c09788ef1933aa8a70f7cea74d5f0b42ed0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 27 Jun 2025 15:08:17 +0200 Subject: [PATCH 2/5] nix store delete: Show the first root that prevents deletion Examples: error: Cannot delete path '/nix/store/6fcrjgfjip2ww3sx51rrmmghfsf60jvi-patchelf-0.14.3' because it's referenced by the GC root '/home/eelco/Dev/nix-master/build/result'. error: Cannot delete path '/nix/store/rn0qyn3kmky26xgpr2n10vr787g57lff-cowsay-3.8.4' because it's referenced by the GC root '/proc/3600568/environ'. --- src/libstore/gc.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 680a9f0e178..05b1a1d0c5b 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -458,7 +458,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) bool gcKeepOutputs = settings.gcKeepOutputs; bool gcKeepDerivations = settings.gcKeepDerivations; - std::unordered_set roots, dead, alive; + Roots roots; + std::unordered_set dead, alive; struct Shared { @@ -612,11 +613,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Find the roots. Since we've grabbed the GC lock, the set of permanent roots cannot increase now. */ printInfo("finding garbage collector roots..."); - Roots rootMap; if (!options.ignoreLiveness) - findRootsNoTemp(rootMap, true); - - for (auto & i : rootMap) roots.insert(i.first); + findRootsNoTemp(roots, true); /* Read the temporary roots created before we acquired the global GC root. Any new roots will be sent to our socket. */ @@ -715,11 +713,12 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) }; /* If this is a root, bail out. */ - if (roots.count(*path)) { + if (auto i = roots.find(*path); i != roots.end()) { if (options.action == GCOptions::gcDeleteSpecific) throw Error( - "Cannot delete path '%s' because it's a GC root.", - printStorePath(start)); + "Cannot delete path '%s' because it's referenced by the GC root '%s'.", + printStorePath(start), + *i->second.begin()); debug("cannot delete '%s' because it's a root", printStorePath(*path)); return markAlive(); } From 774c4ba7403e52a7839585bbabce3f39c0ee176b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 27 Jun 2025 15:09:19 +0200 Subject: [PATCH 3/5] Don't censor root info for trusted users --- src/libstore/daemon.cc | 1 + src/libstore/gc.cc | 4 ++-- src/libstore/include/nix/store/gc-store.hh | 6 ++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index dfc068bc775..4bca7522876 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -730,6 +730,7 @@ static void performOp(TunnelLogger * logger, ref store, options.action = (GCOptions::GCAction) readInt(conn.from); options.pathsToDelete = WorkerProto::Serialise::read(*store, rconn); conn.from >> options.ignoreLiveness >> options.maxFreed; + options.censor = !trusted; // obsolete fields readInt(conn.from); readInt(conn.from); diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 05b1a1d0c5b..7a79b08d6a7 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -614,12 +614,12 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) permanent roots cannot increase now. */ printInfo("finding garbage collector roots..."); if (!options.ignoreLiveness) - findRootsNoTemp(roots, true); + findRootsNoTemp(roots, options.censor); /* Read the temporary roots created before we acquired the global GC root. Any new roots will be sent to our socket. */ Roots tempRoots; - findTempRoots(tempRoots, true); + findTempRoots(tempRoots, options.censor); for (auto & root : tempRoots) _shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); diff --git a/src/libstore/include/nix/store/gc-store.hh b/src/libstore/include/nix/store/gc-store.hh index 8b25ec8d4cb..f99bea4624e 100644 --- a/src/libstore/include/nix/store/gc-store.hh +++ b/src/libstore/include/nix/store/gc-store.hh @@ -53,6 +53,12 @@ struct GCOptions * Stop after at least `maxFreed` bytes have been freed. */ uint64_t maxFreed{std::numeric_limits::max()}; + + /** + * Whether to hide potentially sensitive information about GC + * roots (such as PIDs). + */ + bool censor = false; }; From 5b2388521654b3ea5181a15256f92b8194fa4a58 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 27 Jun 2025 16:50:48 +0200 Subject: [PATCH 4/5] Show which PID is causing a temp root Example: error: Cannot delete path '/nix/store/klyng5rpdkwi5kbxkncy4gjwb490dlhb-foo.drv' because it's in use by Nix process '{nix-process:3605324}'. --- src/libstore/gc.cc | 26 ++++++++++++------- src/libstore/include/nix/store/gc-store.hh | 5 +++- tests/functional/gc-runtime.nix | 1 + tests/functional/gc-runtime.sh | 9 +++++-- tests/functional/gc.sh | 4 +-- .../local-overlay-store/delete-refs-inner.sh | 10 +++---- 6 files changed, 35 insertions(+), 20 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 7a79b08d6a7..15c05cd08d1 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -208,7 +208,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor) while ((end = contents.find((char) 0, pos)) != std::string::npos) { Path root(contents, pos, end - pos); debug("got temporary root '%s'", root); - tempRoots[parseStorePath(root)].emplace(censor ? censored : fmt("{temp:%d}", pid)); + tempRoots[parseStorePath(root)].emplace(censor ? censored : fmt("{nix-process:%d}", pid)); pos = end + 1; } } @@ -465,7 +465,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) { // The temp roots only store the hash part to make it easier to // ignore suffixes like '.lock', '.chroot' and '.check'. - std::unordered_set tempRoots; + std::unordered_map tempRoots; // Hash part of the store path currently being deleted, if // any. @@ -574,7 +574,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) debug("got new GC root '%s'", path); auto hashPart = std::string(storePath->hashPart()); auto shared(_shared.lock()); - shared->tempRoots.insert(hashPart); + // FIXME: could get the PID from the socket. + shared->tempRoots.insert_or_assign(hashPart, "{nix-process:unknown}"); /* If this path is currently being deleted, then we have to wait until deletion is finished to ensure that @@ -618,10 +619,14 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Read the temporary roots created before we acquired the global GC root. Any new roots will be sent to our socket. */ - Roots tempRoots; - findTempRoots(tempRoots, options.censor); - for (auto & root : tempRoots) - _shared.lock()->tempRoots.insert(std::string(root.first.hashPart())); + { + Roots tempRoots; + findTempRoots(tempRoots, options.censor); + for (auto & root : tempRoots) + _shared.lock()->tempRoots.insert_or_assign( + std::string(root.first.hashPart()), + *root.second.begin()); + } /* Synchronisation point for testing, see tests/functional/gc-non-blocking.sh. */ if (auto p = getEnv("_NIX_TEST_GC_SYNC_2")) @@ -735,11 +740,12 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) { auto hashPart = std::string(path->hashPart()); auto shared(_shared.lock()); - if (shared->tempRoots.count(hashPart)) { + if (auto i = shared->tempRoots.find(hashPart); i != shared->tempRoots.end()) { if (options.action == GCOptions::gcDeleteSpecific) throw Error( - "Cannot delete path '%s' because it's in use by a Nix process.", - printStorePath(start)); + "Cannot delete path '%s' because it's in use by '%s'.", + printStorePath(start), + i->second); return markAlive(); } shared->pending = hashPart; diff --git a/src/libstore/include/nix/store/gc-store.hh b/src/libstore/include/nix/store/gc-store.hh index f99bea4624e..8d9a83e67ac 100644 --- a/src/libstore/include/nix/store/gc-store.hh +++ b/src/libstore/include/nix/store/gc-store.hh @@ -7,8 +7,11 @@ namespace nix { +// FIXME: should turn this into an std::variant to represent the +// several root types. +using GcRootInfo = std::string; -typedef std::unordered_map> Roots; +typedef std::unordered_map> Roots; struct GCOptions diff --git a/tests/functional/gc-runtime.nix b/tests/functional/gc-runtime.nix index ee5980bdff9..df7f8ad1647 100644 --- a/tests/functional/gc-runtime.nix +++ b/tests/functional/gc-runtime.nix @@ -9,6 +9,7 @@ mkDerivation { cat > $out/program < \$TEST_ROOT/fifo sleep 10000 EOF diff --git a/tests/functional/gc-runtime.sh b/tests/functional/gc-runtime.sh index 0cccaaf16ab..34e99415d5c 100755 --- a/tests/functional/gc-runtime.sh +++ b/tests/functional/gc-runtime.sh @@ -21,11 +21,16 @@ nix-env -p "$profiles/test" -f ./gc-runtime.nix -i gc-runtime outPath=$(nix-env -p "$profiles/test" -q --no-name --out-path gc-runtime) echo "$outPath" +fifo="$TEST_ROOT/fifo" +mkfifo "$fifo" + echo "backgrounding program..." -"$profiles"/test/program & -sleep 2 # hack - wait for the program to get started +"$profiles"/test/program "$fifo" & child=$! echo PID=$child +cat "$fifo" + +expectStderr 1 nix-store --delete "$outPath" | grepQuiet "Cannot delete path.*because it's referenced by the GC root '/proc/" nix-env -p "$profiles/test" -e gc-runtime nix-env -p "$profiles/test" --delete-generations old diff --git a/tests/functional/gc.sh b/tests/functional/gc.sh index c58f47021f8..66dd12eac7e 100755 --- a/tests/functional/gc.sh +++ b/tests/functional/gc.sh @@ -23,10 +23,10 @@ if nix-store --gc --print-dead | grep -E "$outPath"$; then false; fi nix-store --gc --print-dead inUse=$(readLink "$outPath/reference-to-input-2") -if nix-store --delete "$inUse"; then false; fi +expectStderr 1 nix-store --delete "$inUse" | grepQuiet "Cannot delete path.*because it's referenced by the GC root " test -e "$inUse" -if nix-store --delete "$outPath"; then false; fi +expectStderr 1 nix-store --delete "$outPath" | grepQuiet "Cannot delete path.*because it's referenced by the GC root " test -e "$outPath" for i in "$NIX_STORE_DIR"/*; do diff --git a/tests/functional/local-overlay-store/delete-refs-inner.sh b/tests/functional/local-overlay-store/delete-refs-inner.sh index 385eeadc923..01b6162c529 100644 --- a/tests/functional/local-overlay-store/delete-refs-inner.sh +++ b/tests/functional/local-overlay-store/delete-refs-inner.sh @@ -22,14 +22,14 @@ input2=$(nix-build ../hermetic.nix --no-out-link --arg busybox "$busybox" --arg input3=$(nix-build ../hermetic.nix --no-out-link --arg busybox "$busybox" --arg withFinalRefs true --arg seed 2 -A passthru.input3 -j0) # Can't delete because referenced -expectStderr 1 nix-store --delete $input1 | grepQuiet "Cannot delete path" -expectStderr 1 nix-store --delete $input2 | grepQuiet "Cannot delete path" -expectStderr 1 nix-store --delete $input3 | grepQuiet "Cannot delete path" +expectStderr 1 nix-store --delete $input1 | grepQuiet "Cannot delete path.*because it's referenced by path" +expectStderr 1 nix-store --delete $input2 | grepQuiet "Cannot delete path.*because it's referenced by path" +expectStderr 1 nix-store --delete $input3 | grepQuiet "Cannot delete path.*because it's referenced by path" # These same paths are referenced in the lower layer (by the seed 1 # build done in `initLowerStore`). -expectStderr 1 nix-store --store "$storeA" --delete $input2 | grepQuiet "Cannot delete path" -expectStderr 1 nix-store --store "$storeA" --delete $input3 | grepQuiet "Cannot delete path" +expectStderr 1 nix-store --store "$storeA" --delete $input2 | grepQuiet "Cannot delete path.*because it's referenced by path" +expectStderr 1 nix-store --store "$storeA" --delete $input3 | grepQuiet "Cannot delete path.*because it's referenced by path" # Can delete nix-store --delete $hermetic From 2059f72d02c973ba8101aeb64ec0b7503c9f1fa8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 30 Jun 2025 11:30:24 +0200 Subject: [PATCH 5/5] Fix test --- src/libstore/gc.cc | 18 +++++++++--------- tests/functional/gc.sh | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 15c05cd08d1..d1bbe1571d6 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -717,6 +717,15 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } catch (InvalidPath &) { } }; + if (options.action == GCOptions::gcDeleteSpecific + && !options.pathsToDelete.count(*path)) + { + throw Error( + "Cannot delete path '%s' because it's referenced by path '%s'.", + printStorePath(start), + printStorePath(*path)); + } + /* If this is a root, bail out. */ if (auto i = roots.find(*path); i != roots.end()) { if (options.action == GCOptions::gcDeleteSpecific) @@ -728,15 +737,6 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) return markAlive(); } - if (options.action == GCOptions::gcDeleteSpecific - && !options.pathsToDelete.count(*path)) - { - throw Error( - "Cannot delete path '%s' because it's referenced by path '%s'.", - printStorePath(start), - printStorePath(*path)); - } - { auto hashPart = std::string(path->hashPart()); auto shared(_shared.lock()); diff --git a/tests/functional/gc.sh b/tests/functional/gc.sh index 66dd12eac7e..92ac7fac41d 100755 --- a/tests/functional/gc.sh +++ b/tests/functional/gc.sh @@ -23,7 +23,7 @@ if nix-store --gc --print-dead | grep -E "$outPath"$; then false; fi nix-store --gc --print-dead inUse=$(readLink "$outPath/reference-to-input-2") -expectStderr 1 nix-store --delete "$inUse" | grepQuiet "Cannot delete path.*because it's referenced by the GC root " +expectStderr 1 nix-store --delete "$inUse" | grepQuiet "Cannot delete path.*because it's referenced by path '" test -e "$inUse" expectStderr 1 nix-store --delete "$outPath" | grepQuiet "Cannot delete path.*because it's referenced by the GC root "