From 38b880de9edc8499f67cffafd3b4c54e7674eafa Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com> Date: Thu, 14 Nov 2024 11:03:58 +0300 Subject: [PATCH 1/2] fix(libexpr/eval-inline): get rid of references to nullptr env When diagnosing infinite recursion references to nullptr `Env` can be formed. This happens only with `ExprBlackHole` is evaluated, which always leads to `InfiniteRecursionError`. UBSAN log for one such case: ``` ../src/libexpr/eval-inline.hh:94:31: runtime error: reference binding to null pointer of type 'Env' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/libexpr/eval-inline.hh:94:31 in ``` --- src/libexpr/eval-inline.hh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index d5ce238b2ed..398342e0fd6 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -87,11 +87,22 @@ void EvalState::forceValue(Value & v, const PosIdx pos) { if (v.isThunk()) { Env * env = v.payload.thunk.env; + assert(env || v.isBlackhole()); Expr * expr = v.payload.thunk.expr; try { v.mkBlackhole(); //checkInterrupt(); - expr->eval(*this, *env, v); + if (env) [[likely]] { + expr->eval(*this, *env, v); + } else { + // NOTE: This is necessary in order to avoid forming references to nullptr + // when the value is already a blackhole. This should always throw an + // InfiniteRecursionError. + // TBD: Maybe this case be handled directly without an idirection and a dummy + // environment (which isn't accesses anyway)? + Env dummyEnv; + expr->eval(*this, dummyEnv, v); + } } catch (...) { v.mkThunk(env, expr); tryFixupBlackHolePos(v, pos); From 8152f1680c64f1fd8d3c3d168aa4461a8ef2e494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 16 Nov 2024 13:44:27 +0100 Subject: [PATCH 2/2] Update src/libexpr/eval-inline.hh --- src/libexpr/eval-inline.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 398342e0fd6..00bdd668fc3 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -98,7 +98,7 @@ void EvalState::forceValue(Value & v, const PosIdx pos) // NOTE: This is necessary in order to avoid forming references to nullptr // when the value is already a blackhole. This should always throw an // InfiniteRecursionError. - // TBD: Maybe this case be handled directly without an idirection and a dummy + // TBD: Maybe this case be handled directly without an indirection and a dummy // environment (which isn't accesses anyway)? Env dummyEnv; expr->eval(*this, dummyEnv, v);