From af2ddfdb3b8ff171bb50b84819f4561aade86e0d Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com> Date: Fri, 21 Feb 2025 12:11:31 +0000 Subject: [PATCH 1/2] libexpr: Fix use-after-free of StaticEnv::up It's not very clear what the ownership model is here, but one thing is certain: `.up` can't be destroyed before the StaticEnv that refers to it is. Changing a non-owning pointer to taking shared ownership of the parent `StaticEnv` prevents the `.up` from being freed. I'm not a huge fan of the inverted ownership, where child `StaticEnv` takes a refcount of the parent, but this seems like the least intrusive way to fix the use-after-free. This shouldn't cause any shared_ptr cycles to appear (hopefully). --- src/libcmd/repl.cc | 2 +- src/libexpr/nixexpr.cc | 18 +++++++++--------- src/libexpr/nixexpr.hh | 7 +++++-- src/libexpr/primops.cc | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 3043be7a370..e2aa99cfc38 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -129,7 +129,7 @@ NixRepl::NixRepl(const LookupPath & lookupPath, nix::ref store, refstaticBaseEnv.get())) + , staticEnv(new StaticEnv(nullptr, state->staticBaseEnv)) , runNixPtr{runNix} , interacter(make_unique(getDataDir() + "/repl-history")) { diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 063ff07537b..e8bd02b9bc9 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -310,7 +310,7 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & const StaticEnv * curEnv; Level level; int withLevel = -1; - for (curEnv = env.get(), level = 0; curEnv; curEnv = curEnv->up, level++) { + for (curEnv = env.get(), level = 0; curEnv; curEnv = curEnv->up.get(), level++) { if (curEnv->isWith) { if (withLevel == -1) withLevel = level; } else { @@ -331,7 +331,7 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & "undefined variable '%1%'", es.symbols[name] ).atPos(pos).debugThrow(); - for (auto * e = env.get(); e && !fromWith; e = e->up) + for (auto * e = env.get(); e && !fromWith; e = e->up.get()) fromWith = e->isWith; this->level = withLevel; } @@ -379,7 +379,7 @@ std::shared_ptr ExprAttrs::bindInheritSources( // and displacement, and nothing else is allowed to access it. ideally we'd // not even *have* an expr that grabs anything from this env since it's fully // invisible, but the evaluator does not allow for this yet. - auto inner = std::make_shared(nullptr, env.get(), 0); + auto inner = std::make_shared(nullptr, env, 0); for (auto from : *inheritFromExprs) from->bindVars(es, env); @@ -393,7 +393,7 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr if (recursive) { auto newEnv = [&] () -> std::shared_ptr { - auto newEnv = std::make_shared(nullptr, env.get(), attrs.size()); + auto newEnv = std::make_shared(nullptr, env, attrs.size()); Displacement displ = 0; for (auto & i : attrs) @@ -440,7 +440,7 @@ void ExprLambda::bindVars(EvalState & es, const std::shared_ptr es.exprEnvs.insert(std::make_pair(this, env)); auto newEnv = std::make_shared( - nullptr, env.get(), + nullptr, env, (hasFormals() ? formals->formals.size() : 0) + (!arg ? 0 : 1)); @@ -474,7 +474,7 @@ void ExprCall::bindVars(EvalState & es, const std::shared_ptr & void ExprLet::bindVars(EvalState & es, const std::shared_ptr & env) { auto newEnv = [&] () -> std::shared_ptr { - auto newEnv = std::make_shared(nullptr, env.get(), attrs->attrs.size()); + auto newEnv = std::make_shared(nullptr, env, attrs->attrs.size()); Displacement displ = 0; for (auto & i : attrs->attrs) @@ -500,7 +500,7 @@ void ExprWith::bindVars(EvalState & es, const std::shared_ptr & es.exprEnvs.insert(std::make_pair(this, env)); parentWith = nullptr; - for (auto * e = env.get(); e && !parentWith; e = e->up) + for (auto * e = env.get(); e && !parentWith; e = e->up.get()) parentWith = e->isWith; /* Does this `with' have an enclosing `with'? If so, record its @@ -509,14 +509,14 @@ void ExprWith::bindVars(EvalState & es, const std::shared_ptr & const StaticEnv * curEnv; Level level; prevWith = 0; - for (curEnv = env.get(), level = 1; curEnv; curEnv = curEnv->up, level++) + for (curEnv = env.get(), level = 1; curEnv; curEnv = curEnv->up.get(), level++) if (curEnv->isWith) { prevWith = level; break; } attrs->bindVars(es, env); - auto newEnv = std::make_shared(this, env.get()); + auto newEnv = std::make_shared(this, env); body->bindVars(es, newEnv); } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index a7ad580d2df..88ebc80f8f9 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -480,13 +480,16 @@ extern ExprBlackHole eBlackHole; struct StaticEnv { ExprWith * isWith; - const StaticEnv * up; + std::shared_ptr up; // Note: these must be in sorted order. typedef std::vector> Vars; Vars vars; - StaticEnv(ExprWith * isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { + StaticEnv(ExprWith * isWith, std::shared_ptr up, size_t expectedSize = 0) + : isWith(isWith) + , up(std::move(up)) + { vars.reserve(expectedSize); }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7c9ce71045d..a2ea029eab8 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -238,7 +238,7 @@ static void scopedImport(EvalState & state, const PosIdx pos, SourcePath & path, Env * env = &state.allocEnv(vScope->attrs()->size()); env->up = &state.baseEnv; - auto staticEnv = std::make_shared(nullptr, state.staticBaseEnv.get(), vScope->attrs()->size()); + auto staticEnv = std::make_shared(nullptr, state.staticBaseEnv, vScope->attrs()->size()); unsigned int displ = 0; for (auto & attr : *vScope->attrs()) { From 0d5004508f7be6b75a2fd5127144bff4ce925b65 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com> Date: Fri, 21 Feb 2025 12:29:54 +0000 Subject: [PATCH 2/2] tests/functional: Add flake-based regression for debugger use-after-free This is the simplest reproducer I have. It would be great to find a repro without flakes, but I guess this should be ok for now. --- tests/functional/flakes/debugger.sh | 28 ++++++++++++++++++++++++++++ tests/functional/flakes/meson.build | 3 ++- 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 tests/functional/flakes/debugger.sh diff --git a/tests/functional/flakes/debugger.sh b/tests/functional/flakes/debugger.sh new file mode 100644 index 00000000000..f75f0ad39ef --- /dev/null +++ b/tests/functional/flakes/debugger.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash + +source ./common.sh + +requireGit + +flakeDir="$TEST_ROOT/flake" +createGitRepo "$flakeDir" + +cat >"$flakeDir/flake.nix" <