guiwf/lix
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "libstore/build: automatic clean up of unsuccessfully built scratch outputs"

Browse Source 
This reverts commit f85c84db37 as this is the root cause for the critical correctness bug.

Change-Id: If71516db54138201039473485fb3cf7b5f49ccb0
This commit is contained in:
eldritch horrors
2025-06-29 20:19:30 +00:00
parent 9d40ddb627
commit 5a0ab5af09
5 changed files with 44 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files
doc/manual/rl-next/failed-cve-mitigation.md
+35
View File
@@ -0,0 +1,35 @@
---
synopsis: "Revert CVE-2025-52992 failed mitigation"
issues: [fj#883, fj#887]
cls: [3444, 3528]
category: "Fixes"
credits: ["raito", "horrors"]
---
Following the initial mitigation of **CVE-2025-52992** in `cl/3444`, we
received reports of **unexpected deletion of in-use store paths**.
Upon investigation, we found that the patch did **not correctly cancel all
automatic deleters**, resulting in potentially critical path loss during normal
operation.
Given the severity and time-sensitive nature of the situation ([see incident
report](https://lix.systems/blog/2025-06-27-lix-critical-bug/)), we evaluated
possible options to repair the behavior safely. However, we concluded that a
rushed fix would either
* **Overdelete**, i.e. breaking running systems, or,
* **Underdelete**, effectively **reopening CVE-2025-52992** while leaving
orphaned paths behind.
As **CVE-2025-52992 has no known exploit vector**, and correctness is critical
in the Lix project, we have **fully reverted the previous mitigations**.
The affected patches (`cl/3444`) have been rolled back for the time being.
Moving forward, the Lix team will rework this code path in a **long-term,
correctness-first fix** on the main branch. We will explore backporting it to
stable channels once its safety is assured.
We are deeply sorry for the stability incident and the Lix team remain
available for assisting you in recovering your systems.
lix/libstore/build/local-derivation-goal.cc
+7 -34
View File
@@ -394,13 +394,9 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck()
void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck()
{
/* In the past, redirected outputs were manually tracked for deletion.
* Now that we have the scratch outputs cleaner which are a superset of
* redirected outputs, we just fire all uncancelled automatic deleters now.
*
* This should clean up any paths that IS NOT registered in the database.
*/
scratchOutputsCleaner.clear();
/* Delete unused redirected outputs (when doing hash rewriting). */
for (auto & i : redirectedOutputs)
deletePath(worker.store.Store::toRealPath(i.second));
/* Delete the chroot (if we were using one). */
autoDelChroot.reset(); /* this runs the destructor */
@@ -535,10 +531,6 @@ try {
to use a temporary path */
makeFallbackPath(status.known->path);
scratchOutputs.insert_or_assign(outputName, scratchPath);
/* Schedule this scratch output path for automatic deletion
* if we do not cancel it, e.g. when registering the outputs.
*/
scratchOutputsCleaner.insert_or_assign(outputName, worker.store.printStorePath(scratchPath));
/* Substitute output placeholders with the scratch output paths.
We'll use during the build. */
@@ -561,6 +553,8 @@ try {
std::string h2 { scratchPath.hashPart() };
inputRewrites[h1] = h2;
}
redirectedOutputs.insert_or_assign(std::move(fixedFinalPath), std::move(scratchPath));
}
/* Construct the environment passed to the builder. */
@@ -2035,9 +2029,7 @@ try {
}
/* Don't register anything, since we already have the
previous versions which we're comparing.
NOTE: this means that the `.check` path will be automatically deleted.
*/
previous versions which we're comparing. */
continue;
}
@@ -2061,13 +2053,8 @@ try {
/* If it's a CA path, register it right away. This is necessary if it
isn't statically known so that we can safely unlock the path before
the next iteration */
if (newInfo.ca) {
if (newInfo.ca)
TRY_AWAIT(localStore.registerValidPaths({{newInfo.path, newInfo}}));
/* Cancel automatic deletion of that output if it was a scratch output. */
if (auto cleaner = scratchOutputsCleaner.extract(outputName)) {
cleaner.mapped().cancel();
}
}
infos.emplace(outputName, std::move(newInfo));
}
@@ -2107,13 +2094,6 @@ try {
infos2.insert_or_assign(newInfo.path, newInfo);
}
TRY_AWAIT(localStore.registerValidPaths(infos2));
/* Cancel automatic deletion of that output if it was a scratch output that we just registered. */
for (auto & [outputName, _ ] : infos) {
if (auto cleaner = scratchOutputsCleaner.extract(outputName)) {
cleaner.mapped().cancel();
}
}
}
/* In case of a fixed-output derivation hash mismatch, throw an
@@ -2147,13 +2127,6 @@ try {
builtOutputs.emplace(outputName, thisRealisation);
}
/* NOTE: At this point, all outputs MAY NOT have been registered.
* Therefore, there may remains auto-deleters pending in the cleaner list (`scratchOutputsCleaner`).
*
* They will be finally deleted but we have no way to assert they all have been, e.g.
* `assert(scratchOutputsCleaner.size() == 0)` cannot be written.
*/
co_return builtOutputs;
} catch (...) {
co_return result::current_exception();
lix/libstore/build/local-derivation-goal.hh
+2 -13
View File
@@ -100,6 +100,8 @@ struct LocalDerivationGoal : public DerivationGoal
* Hash rewriting.
*/
StringMap inputRewrites, outputRewrites;
typedef map<StorePath, StorePath> RedirectedOutputs;
RedirectedOutputs redirectedOutputs;
/**
* The outputs paths used during the build.
@@ -116,19 +118,6 @@ struct LocalDerivationGoal : public DerivationGoal
* self-references.
*/
OutputPathMap scratchOutputs;
/**
* Output paths used during the build are scheduled for
* automatic cleanup unless they have been successfully built.
*
* `registerOutputs` take care of cancelling the cleanups
* and clearing this vector.
*
* `startBuilder` take care of filling this vector
* as `scratchOutputs` gets filled.
*
* This is a map from output names to automatic delete handles.
*/
std::map<std::string, AutoDelete> scratchOutputsCleaner;
/**
* Path registration info from the previous round, if we're
tests/nixos/default.nix
-3
View File
@@ -169,9 +169,6 @@ in
symlinkResolvconf = runNixOSTestFor "x86_64-linux" ./symlink-resolvconf.nix;
# Use this test to test things that cannot easily be tested under chroot Nix stores in functional test suite.
non-chroot-misc = runNixOSTestFor "x86_64-linux" ./non-chroot-misc;
noNewPrivilegesInSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/sandbox.nix;
noNewPrivilegesOutsideSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/no-sandbox.nix;
tests/nixos/non-chroot-misc/default.nix
-34
View File
@@ -1,34 +0,0 @@
{ ... }:
# Misc things we want to test inside of a non redirected, non chroot Nix store.
let
nonAutoCleaningFailingDerivationCode = ''
derivation {
name = "scratch-failing";
system = builtins.currentSystem;
builder = "/bin/sh";
args = [ (builtins.toFile "builder.sh" "echo bonjour > $out; echo out: $out; false") ];
}
'';
in
{
name = "non-chroot-sandbox-misc";
nodes.machine = {
};
testScript = { nodes }: ''
import re
start_all()
# You might ask yourself why write such a convoluted thing?
# The condition for fooling Nix into NOT cleaning up the output path are non trivial and unclear.
# This is one of those: create a derivation, mkdir or touch the $out path, communicate it back.
# Even with a sandboxed Lix, you will observe leftovers before 2.93.0. After this version, this test passes.
result = machine.fail("""nix-build --substituters "" -E '${nonAutoCleaningFailingDerivationCode}' 2>&1""")
match = re.search(r'out: (\S+)', result)
assert match is not None, "Did not find Nix store path in the result of the failing build"
outpath = match.group(1).strip()
print(f"Found Nix store path: {outpath}")
machine.fail(f'stat {outpath}')
'';
}