From 001be527944665433a149add979b9a8139787ee6 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 4 Mar 2024 06:18:20 +0100 Subject: [PATCH] Merge pull request #9430 from hercules-ci/remove-vlas Fix stack overflow in `filter` (cherry picked from commit cb7f25869daa2491eeac5fee49ad8f31b2218c15) Change-Id: Ib90f97a9805bbb4d0e2741551d490f054fc0a675 --- boehmgc-traceable_allocator-public.diff | 12 +++++++ flake.nix | 3 ++ src/libexpr/eval.cc | 13 +++++--- src/libexpr/gc-small-vector.hh | 42 +++++++++++++++++++++++++ src/libexpr/primops.cc | 9 +++--- 5 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 boehmgc-traceable_allocator-public.diff create mode 100644 src/libexpr/gc-small-vector.hh diff --git a/boehmgc-traceable_allocator-public.diff b/boehmgc-traceable_allocator-public.diff new file mode 100644 index 000000000..903c707a6 --- /dev/null +++ b/boehmgc-traceable_allocator-public.diff @@ -0,0 +1,12 @@ +diff --git a/include/gc_allocator.h b/include/gc_allocator.h +index 597c7f13..587286be 100644 +--- a/include/gc_allocator.h ++++ b/include/gc_allocator.h +@@ -312,6 +312,7 @@ public: + + template<> + class traceable_allocator { ++public: + typedef size_t size_type; + typedef ptrdiff_t difference_type; + typedef void* pointer; diff --git a/flake.nix b/flake.nix index 2caef3f7d..92d7b6b05 100644 --- a/flake.nix +++ b/flake.nix @@ -228,6 +228,9 @@ }).overrideAttrs(o: { patches = (o.patches or []) ++ [ ./boehmgc-coroutine-sp-fallback.diff + + # https://github.com/ivmai/bdwgc/pull/586 + ./boehmgc-traceable_allocator-public.diff ]; }) ) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index f5eacd0e0..d414db79b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -13,6 +13,7 @@ #include "function-trace.hh" #include "profiles.hh" #include "print.hh" +#include "gc-small-vector.hh" #include #include @@ -28,6 +29,7 @@ #include #include +#include #if HAVE_BOEHMGC @@ -1701,7 +1703,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & /* We have all the arguments, so call the primop with the previous and new arguments. */ - Value * vArgs[arity]; + Value * vArgs[maxPrimOpArity]; auto n = argsDone; for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp.left) vArgs[--n] = arg->primOpApp.right; @@ -1764,11 +1766,11 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v) // 4: about 60 // 5: under 10 // This excluded attrset lambdas (`{...}:`). Contributions of mixed lambdas appears insignificant at ~150 total. - Value * vArgs[args.size()]; + SmallValueVector<4> vArgs(args.size()); for (size_t i = 0; i < args.size(); ++i) vArgs[i] = args[i]->maybeThunk(state, env); - state.callFunction(vFun, args.size(), vArgs, v, pos); + state.callFunction(vFun, args.size(), vArgs.data(), v, pos); } @@ -2007,8 +2009,9 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) return result; }; - Value values[es->size()]; - Value * vTmpP = values; + // List of returned strings. References to these Values must NOT be persisted. + SmallTemporaryValueVector values(es->size()); + Value * vTmpP = values.data(); for (auto & [i_pos, i] : *es) { Value & vTmp = *vTmpP++; diff --git a/src/libexpr/gc-small-vector.hh b/src/libexpr/gc-small-vector.hh new file mode 100644 index 000000000..7f4f08fc7 --- /dev/null +++ b/src/libexpr/gc-small-vector.hh @@ -0,0 +1,42 @@ +#pragma once + +#include + +#if HAVE_BOEHMGC + +#include +#include +#include + +#endif + +namespace nix { + +struct Value; + +/** + * A GC compatible vector that may used a reserved portion of `nItems` on the stack instead of allocating on the heap. + */ +#if HAVE_BOEHMGC +template +using SmallVector = boost::container::small_vector>; +#else +template +using SmallVector = boost::container::small_vector; +#endif + +/** + * A vector of value pointers. See `SmallVector`. + */ +template +using SmallValueVector = SmallVector; + +/** + * A vector of values that must not be referenced after the vector is destroyed. + * + * See also `SmallValueVector`. + */ +template +using SmallTemporaryValueVector = SmallVector; + +} \ No newline at end of file diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index ac8dbeaf1..b906e7747 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4,6 +4,7 @@ #include "eval-inline.hh" #include "eval.hh" #include "eval-settings.hh" +#include "gc-small-vector.hh" #include "globals.hh" #include "json-to-value.hh" #include "names.hh" @@ -2726,7 +2727,7 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V auto attrName = state.symbols.create(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.catAttrs")); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.catAttrs"); - Value * res[args[1]->listSize()]; + SmallValueVector res(args[1]->listSize()); size_t found = 0; for (auto v2 : args[1]->listItems()) { @@ -3061,8 +3062,7 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); - // FIXME: putting this on the stack is risky. - Value * vs[args[1]->listSize()]; + SmallValueVector vs(args[1]->listSize()); size_t k = 0; bool same = true; @@ -3451,7 +3451,8 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args, state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap"); auto nrLists = args[1]->listSize(); - Value lists[nrLists]; + // List of returned lists before concatenation. References to these Values must NOT be persisted. + SmallTemporaryValueVector lists(nrLists); size_t len = 0; for (unsigned int n = 0; n < nrLists; ++n) {