Skip to content

libexpr: add temp roots during evaluation to prevent GC races #15795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
martinjlowm wants to merge 1 commit into from
Closed

libexpr: add temp roots during evaluation to prevent GC races #15795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/libexpr/get-drvs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ PackageInfo::PackageInfo(EvalState & state, ref<Store> store, const std::string

this->drvPath = drvPath;

/* Prevent GC from deleting the .drv before we read it. This
constructor is only called from CLI entry points (nix-build)
where the path originates from a previous session, so it
has no temp root from the current process. */
store->addTempRoot(drvPath);
Comment on lines +27 to +31
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be that it should be the caller of this constructor that ensures that the path is rooted, not the constructor itself. Otherwise the path could disappear

Copy link
Copy Markdown
Author

@martinjlowm martinjlowm May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think it's smarter to couple the info about a temp root to being as close to derivationFromPath (and readDerivation) as possible? That is, avoiding the possibility of initializing PackageInfo and forgetting about locking it down just before it being parsed?

Copy link
Copy Markdown
Contributor

@xokdvium xokdvium May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you doing a readDerivation on an unrooted path it's already too late. It must be rooted before (either by writeDerivation or by a permanent root created by a cli command that's passing it to another one). We are not trying to make it les racy, but rather address the root cause in a correct way - not just one that happens to reduce the race window.

Copy link
Copy Markdown
Author

@martinjlowm martinjlowm May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not concerned about the input derivation to nix-build abc.drv (https://github.com/NixOS/nix/blob/master/src/nix/nix-build/nix-build.cc#L395-L397) from CLI, but rather transitive derivations referenced from it for a deeply nested derivation: https://github.com/NixOS/nix/blob/master/src/libexpr/get-drvs.cc#L392 - this instantiates PackageInfo - I'd say that is as close to the root as we can get without introducing a separate "root" pass that immediately resolves the full tree of drvs and their paths.

Walking down the call branch of nix build familiarizing myself more with the codebase (thought this would be it), I see, from his contributions, dramforever has been on a spree lately on related files: #15616 - I think perhaps his work solves the one specific problem we're facing.

Copy link
Copy Markdown
Contributor

@xokdvium xokdvium May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those go through writeDerivation since evaluator roots all derivations, so this is not an issue.

Copy link
Copy Markdown
Member

@edolstra edolstra May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's already too late to add a GC root here, since the path has already been created. So if a GC was started between then and this point, the path may be deleted.

However I also don't see why it's needed in the first place, since Store::writeDerivation() already has a call to addTempRoot().

auto drv = store->derivationFromPath(drvPath);

name = drvPath.name();
Expand Down
8 changes: 7 additions & 1 deletion src/libexpr/primops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,14 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v
auto isValidDerivationInStore = [&]() -> std::optional<StorePath> {
if (!state.store->isStorePath(path2))
return std::nullopt;
if (!isDerivation(path2))
return std::nullopt;
auto storePath = state.store->parseStorePath(path2);
if (!(state.store->isValidPath(storePath) && isDerivation(path2)))
/* Add a temp root before checking validity to prevent GC
from deleting the path between our check and the
subsequent readDerivation(). */
state.store->addTempRoot(storePath);
Copy link
Copy Markdown
Member

@edolstra edolstra May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same applies here: the drv was presumably added by a call to writeDerivation(), so it should already be a temp root.

Copy link
Copy Markdown
Author

@martinjlowm martinjlowm May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm missing something, but to my understanding writeDerivation is only called on the initial session when the drv is yet to be realized, stored in the store and for any succeeding calls directly nix eval --impure --expr 'let drv = /nix/store/foo.drv; in (import drv)', for transitive drv paths would call readDerivation if they already exist? At this point there is nothing to write because they should already live in store, ready to be executed for the build phase.

if (!state.store->isValidPath(storePath))
return std::nullopt;
return storePath;
};
Expand Down
Loading