Skip to content

Revert "Merge pull request #13938 from NixOS/import-thunk"#14013

Merged
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:revert-13938
Sep 17, 2025
Merged

Revert "Merge pull request #13938 from NixOS/import-thunk"#14013
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:revert-13938

Conversation

@xokdvium
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium commented Sep 17, 2025

Motivation

This has multiple dangling pointer issues that lead to segfaults in e.g.:

nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure

Context

This reverts commit ad17572, reversing changes made to d314750.

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@xokdvium
Revert "Merge pull request NixOS#13938 from NixOS/import-thunk"
fd03481 
This has multiple dangling pointer issues that lead to segfaults in e.g.:

nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure

This reverts commit ad17572, reversing
changes made to d314750.
@xokdvium xokdvium requested a review from edolstra as a code owner September 17, 2025 22:54
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Sep 17, 2025
@xokdvium xokdvium mentioned this pull request Sep 17, 2025
Merged
@Mic92 Mic92 enabled auto-merge September 17, 2025 22:58
@Mic92 Mic92 merged commit 9d8c6a6 into NixOS:master Sep 17, 2025
14 checks passed
@xokdvium xokdvium deleted the revert-13938 branch September 17, 2025 23:24
@edolstra
Copy link
Copy Markdown
Member

edolstra commented Sep 18, 2025

This has multiple dangling pointer issues that lead to segfaults in e.g.:

Can you be more specific?

nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure

I wasn't able to verify this since this command OOMs after consuming 40+ GB.

@xokdvium
Copy link
Copy Markdown
Contributor Author

xokdvium commented Sep 18, 2025

Can you be more specific?

Left comments on the original PR.

I wasn't able to verify this since this command OOMs after consuming 40+ GB.

In my machine it segfaulted in ExprParseFile::eval because path reference was dangling. You can also run ASAN and the issue will become apparent.

@edolstra
Copy link
Copy Markdown
Member

edolstra commented Sep 18, 2025

So it turns out that this was caused by #13930 not being merged yet. The "failed" value type ensures the evalFile() post-condition that the ExprParseFile thunk is finished, so there is no reference to it anymore. But on current master, we don't have that yet, so in case of an exception forcing the thunk, the value can revert to a thunk pointing to ExprParseFile. That then segfaults if the same file is evaluated again.

edolstra added a commit that referenced this pull request Sep 22, 2025
@edolstra
Allocate ExprParseFile on the heap for now
d32d77f 
#14013 (comment)
@edolstra edolstra mentioned this pull request Sep 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees

No one assigned

Labels

fetching Networking with the outside (non-Nix) world, input locking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xokdvium @edolstra @Mic92