Revert "libstore/build: automatic clean up of unsuccessfully built scratch outputs"
This reverts commit a2189bcb2e as this is the root cause of the critical correctness bug.
Change-Id: Ic85d3d670dc9488d49f03988fe42feb0cf0e7084
This commit is contained in:
eldritch horrors
committed by
Raito Bezarius
Raito Bezarius
parent
d227ad324e
commit
92cc6193e0
@@ -0,0 +1,33 @@
|
||||
---
|
||||
synopsis: "Revert CVE-2025-52992 failed mitigation"
|
||||
issues: [fj#883, fj#887]
|
||||
cls: [3420, 3524, 3523, 3522]
|
||||
category: "Fixes"
|
||||
credits: ["raito", "horrors"]
|
||||
---
|
||||
|
||||
Following the initial mitigation of **CVE-2025-52992** in `cl/3420`, 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**.
|
||||
|
||||
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.
|
||||
@@ -363,13 +363,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 */
|
||||
@@ -527,10 +523,6 @@ void LocalDerivationGoal::startBuilder()
|
||||
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. */
|
||||
@@ -553,6 +545,8 @@ void LocalDerivationGoal::startBuilder()
|
||||
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. */
|
||||
@@ -2382,10 +2376,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
|
||||
localStore.registerValidPaths({{oldInfo.path, oldInfo}});
|
||||
}
|
||||
|
||||
/* 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.
|
||||
*/
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -2409,13 +2399,8 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
|
||||
/* 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)
|
||||
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));
|
||||
}
|
||||
@@ -2443,13 +2428,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
|
||||
infos2.insert_or_assign(newInfo.path, newInfo);
|
||||
}
|
||||
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
|
||||
@@ -2483,13 +2461,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
|
||||
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.
|
||||
*/
|
||||
|
||||
return builtOutputs;
|
||||
}
|
||||
|
||||
|
||||
@@ -107,6 +107,8 @@ struct LocalDerivationGoal : public DerivationGoal
|
||||
* Hash rewriting.
|
||||
*/
|
||||
StringMap inputRewrites, outputRewrites;
|
||||
typedef map<StorePath, StorePath> RedirectedOutputs;
|
||||
RedirectedOutputs redirectedOutputs;
|
||||
|
||||
/**
|
||||
* The outputs paths used during the build.
|
||||
@@ -123,19 +125,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
|
||||
|
||||
@@ -146,9 +146,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;
|
||||
|
||||
@@ -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}')
|
||||
'';
|
||||
}
|
||||
Reference in New Issue
Block a user