Skip to content

Ensure that files are parsed/evaluated only once (2nd attempt)#14040

Merged
roberth merged 2 commits into
masterfrom
import-thunk
Sep 22, 2025
Merged

Ensure that files are parsed/evaluated only once (2nd attempt)#14040
roberth merged 2 commits into
masterfrom
import-thunk

Conversation

@edolstra
Copy link
Copy Markdown
Member

Motivation

Fixes the segfault in #13938 that necessitated the revert in #14013.

ExprParseFile is allocated on the heap for now. We can put it back on the stack once if #13930 is merged (since it ensures the postcondition that the expr is unreachable after forceValue() returns).

Context


Add 👍 to pull requests you find important.

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

@github-actions github-actions Bot added the fetching Networking with the outside (non-Nix) world, input locking label Sep 22, 2025
@edolstra edolstra requested a review from xokdvium September 22, 2025 10:11
@roberth roberth merged commit 169a368 into master Sep 22, 2025
29 checks passed
@roberth roberth deleted the import-thunk branch September 22, 2025 16:33
@corngood
Copy link
Copy Markdown
Contributor

corngood commented Jan 9, 2026

I was hitting this test failure on cygwin:

[ RUN      ] nix_api_store_test.nix_api_load_flake
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot remove all: Device or resource busy [/nix/var/nix/builds/nix-20054-1636773246/tests_nix-store.B99QIL] [/nix/var/nix/builds/nix-20054-1636773246/tests_nix-store.B99QIL/my_state/db/db.sqlite]
/nix/var/nix/builds/nix-20054-1636773246/.attr-0l2nkwhif96f51f4amnlf414lhl4rv9vh8iffyp431v6s28gsr90: line 3: 20069 Aborted                    /nix/store/hl337315ycb68z69mrhwwr8h0w8w8q6g-exec/bin/exec /nix/store/zw2l6hajhrrllj22pbzdqz47zgryrf72-nix-flake-tests-2.34pre20251217_b6add8dc+4/bin/nix-flake-tests.exe

And I tracked it to the leaking of the ExprParseFile object. I guess it somehow leaves a reference on the store, and the store (LocalStore via nix_api_store_test) is never destroyed.

This sort of thing has happened before (#14576), and it's quite difficult to track down. I wonder if we should assert that the store is actually being destroyed with nix_store_free in the tests?

In terms of working around this, would it be safe to store the pointers in EvalState and delete them in the destructor?

edit:

something like this

commit 868b29c39a7c2e2dd8722a9e7208f193779a9e05
Author: David McFarland <corngood@gmail.com>
Date:   Fri Jan 9 15:53:46 2026 -0400

    Fix leak in EvalState::evalFile

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 84f14198f..40f0b4b7b 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -1129,14 +1129,14 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
     // https://github.com/NixOS/nix/pull/13930 is merged. That will ensure
     // the post-condition that `expr` is unreachable after
     // `forceValue()` returns.
-    auto expr = new ExprParseFile{*resolvedPath, mustBeTrivial};
+    auto expr = parseFileExprs.emplace_back(std::make_shared<ExprParseFile>(*resolvedPath, mustBeTrivial));
 
     fileEvalCache->try_emplace_and_cvisit(
         *resolvedPath,
         nullptr,
         [&](auto & i) {
             vExpr = allocValue();
-            vExpr->mkThunk(&baseEnv, expr);
+            vExpr->mkThunk(&baseEnv, expr.get());
             i.second = vExpr;
         },
         [&](auto & i) { vExpr = i.second; });
diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh
index b70c9db78..ae6e32505 100644
--- a/src/libexpr/include/nix/expr/eval.hh
+++ b/src/libexpr/include/nix/expr/eval.hh
@@ -366,6 +366,8 @@ private:
     Statistics stats;
 };
 
+struct ExprParseFile;
+
 class EvalState : public std::enable_shared_from_this<EvalState>
 {
 public:
@@ -508,6 +510,8 @@ private:
      */
     const ref<RegexCache> regexCache;
 
+    std::vector<std::shared_ptr<ExprParseFile> > parseFileExprs;
+
 public:
 
     /**

@xokdvium
Copy link
Copy Markdown
Contributor

I wonder if we should assert that the store is actually being destroyed with nix_store_free in the test

Ideally we'd just enable leak sanitizer once all parser leaks are fully plugged. Not sure about the status of that currently, but we should be closer to that now.

@Ericson2314
Copy link
Copy Markdown
Member

CC @Radvendii

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants