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

@@ -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<void> {
++public:
+   typedef size_t      size_type;
+   typedef ptrdiff_t   difference_type;
+   typedef void*       pointer;

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
               ];
             })
             )

src/libexpr/eval.cc

@@ -13,6 +13,7 @@
 #include "function-trace.hh"
 #include "profiles.hh"
 #include "print.hh"
+#include "gc-small-vector.hh"
 
 #include <algorithm>
 #include <chrono>
@@ -28,6 +29,7 @@
 
 #include <sys/resource.h>
 #include <nlohmann/json.hpp>
+#include <boost/container/small_vector.hpp>
 
 #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()];
+    // List of returned strings. References to these Values must NOT be persisted.
-    Value * vTmpP = values;
+    SmallTemporaryValueVector<conservativeStackReservation> values(es->size());
+    Value * vTmpP = values.data();
 
     for (auto & [i_pos, i] : *es) {
         Value & vTmp = *vTmpP++;

src/libexpr/gc-small-vector.hh

@@ -0,0 +1,42 @@
+#pragma once
+
+#include <boost/container/small_vector.hpp>
+
+#if HAVE_BOEHMGC
+
+#include <gc/gc.h>
+#include <gc/gc_cpp.h>
+#include <gc/gc_allocator.h>
+
+#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 <typename T, size_t nItems>
+using SmallVector = boost::container::small_vector<T, nItems, traceable_allocator<T>>;
+#else
+template <typename T, size_t nItems>
+using SmallVector = boost::container::small_vector<T, nItems>;
+#endif
+
+/**
+ * A vector of value pointers. See `SmallVector`.
+ */
+template <size_t nItems>
+using SmallValueVector = SmallVector<Value *, nItems>;
+
+/**
+ * A vector of values that must not be referenced after the vector is destroyed.
+ *
+ * See also `SmallValueVector`.
+ */
+template <size_t nItems>
+using SmallTemporaryValueVector = SmallVector<Value, nItems>;
+
+}

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<nonRecursiveStackReservation> 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.
+    SmallValueVector<nonRecursiveStackReservation> vs(args[1]->listSize());
-    Value * 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<conservativeStackReservation> lists(nrLists);
     size_t len = 0;
 
     for (unsigned int n = 0; n < nrLists; ++n) {