Skip to content

Commit ac4431e

Browse files
thufschmittthufschmitt
authored
Merge pull request NixOS#7348 from thufschmitt/dont-use-vlas
Remove the usage of VLAs in the code
2 parents 57de482 + 4e27f19 commit ac4431e

8 files changed

Lines changed: 78 additions & 33 deletions

File tree

‎src/libexpr/eval.cc‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#include "eval.hh"
22
#include "eval-settings.hh"
33
#include "hash.hh"
4+
#include "primops.hh"
45
#include "types.hh"
56
#include "util.hh"
67
#include "store-api.hh"
@@ -41,6 +42,7 @@
4142
#include <boost/coroutine2/coroutine.hpp>
4243
#include <boost/coroutine2/protected_fixedsize_stack.hpp>
4344
#include <boost/context/stack_context.hpp>
45+
#include <boost/container/small_vector.hpp>
4446

4547
#endif
4648

@@ -722,6 +724,23 @@ void EvalState::addConstant(const std::string & name, Value * v, Constant info)
722724
}
723725

724726

727+
void PrimOp::check()
728+
{
729+
if (arity > maxPrimOpArity) {
730+
throw Error("primop arity must not exceed %1%", maxPrimOpArity);
731+
}
732+
}
733+
734+
735+
void Value::mkPrimOp(PrimOp * p)
736+
{
737+
p->check();
738+
clearValue();
739+
internalType = tPrimOp;
740+
primOp = p;
741+
}
742+
743+
725744
Value * EvalState::addPrimOp(PrimOp && primOp)
726745
{
727746
/* Hack to make constants lazy: turn them into a application of
@@ -1691,7 +1710,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
16911710
/* We have all the arguments, so call the primop with
16921711
the previous and new arguments. */
16931712

1694-
Value * vArgs[arity];
1713+
Value * vArgs[maxPrimOpArity];
16951714
auto n = argsDone;
16961715
for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp.left)
16971716
vArgs[--n] = arg->primOpApp.right;
@@ -1748,11 +1767,17 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v)
17481767
Value vFun;
17491768
fun->eval(state, env, vFun);
17501769

1751-
Value * vArgs[args.size()];
1770+
// Empirical arity of Nixpkgs lambdas by regex e.g. ([a-zA-Z]+:(\s|(/\*.*\/)|(#.*\n))*){5}
1771+
// 2: over 4000
1772+
// 3: about 300
1773+
// 4: about 60
1774+
// 5: under 10
1775+
// This excluded attrset lambdas (`{...}:`). Contributions of mixed lambdas appears insignificant at ~150 total.
1776+
boost::container::small_vector<Value *, 4> vArgs(args.size());
17521777
for (size_t i = 0; i < args.size(); ++i)
17531778
vArgs[i] = args[i]->maybeThunk(state, env);
17541779

1755-
state.callFunction(vFun, args.size(), vArgs, v, pos);
1780+
state.callFunction(vFun, args.size(), vArgs.data(), v, pos);
17561781
}
17571782

17581783

@@ -1991,8 +2016,8 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
19912016
return result;
19922017
};
19932018

1994-
Value values[es->size()];
1995-
Value * vTmpP = values;
2019+
boost::container::small_vector<Value, conservativeStackReservation> values(es->size());
2020+
Value * vTmpP = values.data();
19962021

19972022
for (auto & [i_pos, i] : *es) {
19982023
Value & vTmp = *vTmpP++;

‎src/libexpr/eval.hh‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@
1818

1919
namespace nix {
2020

21+
/**
22+
* We put a limit on primop arity because it lets us use a fixed size array on
23+
* the stack. 8 is already an impractical number of arguments. Use an attrset
24+
* argument for such overly complicated functions.
25+
*/
26+
constexpr size_t maxPrimOpArity = 8;
2127

2228
class Store;
2329
class EvalState;
@@ -71,6 +77,12 @@ struct PrimOp
7177
* Optional experimental for this to be gated on.
7278
*/
7379
std::optional<ExperimentalFeature> experimentalFeature;
80+
81+
/**
82+
* Validity check to be performed by functions that introduce primops,
83+
* such as RegisterPrimOp() and Value::mkPrimOp().
84+
*/
85+
void check();
7486
};
7587

7688
/**

‎src/libexpr/primops.cc‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929

3030
#include <cmath>
3131

32-
3332
namespace nix {
3433

3534

@@ -2550,6 +2549,7 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args
25502549
/* Get the attribute names to be removed.
25512550
We keep them as Attrs instead of Symbols so std::set_difference
25522551
can be used to remove them from attrs[0]. */
2552+
// 64: large enough to fit the attributes of a derivation
25532553
boost::container::small_vector<Attr, 64> names;
25542554
names.reserve(args[1]->listSize());
25552555
for (auto elem : args[1]->listItems()) {
@@ -2729,8 +2729,8 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V
27292729
auto attrName = state.symbols.create(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.catAttrs"));
27302730
state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.catAttrs");
27312731

2732-
Value * res[args[1]->listSize()];
2733-
unsigned int found = 0;
2732+
boost::container::small_vector<Value *, nonRecursiveStackReservation> res(args[1]->listSize());
2733+
size_t found = 0;
27342734

27352735
for (auto v2 : args[1]->listItems()) {
27362736
state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs");
@@ -3064,9 +3064,8 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val
30643064

30653065
state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter");
30663066

3067-
// FIXME: putting this on the stack is risky.
3068-
Value * vs[args[1]->listSize()];
3069-
unsigned int k = 0;
3067+
boost::container::small_vector<Value *, nonRecursiveStackReservation> vs(args[1]->listSize());
3068+
size_t k = 0;
30703069

30713070
bool same = true;
30723071
for (unsigned int n = 0; n < args[1]->listSize(); ++n) {
@@ -3191,10 +3190,14 @@ static void anyOrAll(bool any, EvalState & state, const PosIdx pos, Value * * ar
31913190
state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.") + (any ? "any" : "all"));
31923191
state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.") + (any ? "any" : "all"));
31933192

3193+
std::string_view errorCtx = any
3194+
? "while evaluating the return value of the function passed to builtins.any"
3195+
: "while evaluating the return value of the function passed to builtins.all";
3196+
31943197
Value vTmp;
31953198
for (auto elem : args[1]->listItems()) {
31963199
state.callFunction(*args[0], *elem, vTmp, pos);
3197-
bool res = state.forceBool(vTmp, pos, std::string("while evaluating the return value of the function passed to builtins.") + (any ? "any" : "all"));
3200+
bool res = state.forceBool(vTmp, pos, errorCtx);
31983201
if (res == any) {
31993202
v.mkBool(any);
32003203
return;
@@ -3450,7 +3453,7 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args,
34503453
state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap");
34513454
auto nrLists = args[1]->listSize();
34523455

3453-
Value lists[nrLists];
3456+
boost::container::small_vector<Value, conservativeStackReservation> lists(nrLists);
34543457
size_t len = 0;
34553458

34563459
for (unsigned int n = 0; n < nrLists; ++n) {

‎src/libexpr/primops.hh‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,22 @@
88

99
namespace nix {
1010

11+
/**
12+
* For functions where we do not expect deep recursion, we can use a sizable
13+
* part of the stack a free allocation space.
14+
*
15+
* Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes.
16+
*/
17+
constexpr size_t nonRecursiveStackReservation = 128;
18+
19+
/**
20+
* Functions that maybe applied to self-similar inputs, such as concatMap on a
21+
* tree, should reserve a smaller part of the stack for allocation.
22+
*
23+
* Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes.
24+
*/
25+
constexpr size_t conservativeStackReservation = 16;
26+
1127
struct RegisterPrimOp
1228
{
1329
typedef std::vector<PrimOp> PrimOps;

‎src/libexpr/tests/value/print.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ TEST_F(ValuePrintingTests, vLambda)
114114
TEST_F(ValuePrintingTests, vPrimOp)
115115
{
116116
Value vPrimOp;
117-
vPrimOp.mkPrimOp(nullptr);
117+
PrimOp primOp{};
118+
vPrimOp.mkPrimOp(&primOp);
118119

119120
test(vPrimOp, "<PRIMOP>");
120121
}

‎src/libexpr/value.hh‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -354,13 +354,7 @@ public:
354354
// Value will be overridden anyways
355355
}
356356

357-
inline void mkPrimOp(PrimOp * p)
358-
{
359-
clearValue();
360-
internalType = tPrimOp;
361-
primOp = p;
362-
}
363-
357+
void mkPrimOp(PrimOp * p);
364358

365359
inline void mkPrimOpApp(Value * l, Value * r)
366360
{

‎src/libstore/derivations.cc‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,10 @@ StorePath writeDerivation(Store & store,
151151
/* Read string `s' from stream `str'. */
152152
static void expect(std::istream & str, std::string_view s)
153153
{
154-
char s2[s.size()];
155-
str.read(s2, s.size());
156-
std::string_view s2View { s2, s.size() };
157-
if (s2View != s)
158-
throw FormatError("expected string '%s', got '%s'", s, s2View);
154+
for (auto & c : s) {
155+
if (str.get() != c)
156+
throw FormatError("expected string '%1%'", s);
157+
}
159158
}
160159

161160

‎src/libstore/gc.cc‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -330,9 +330,7 @@ typedef std::unordered_map<Path, std::unordered_set<std::string>> UncheckedRoots
330330

331331
static void readProcLink(const std::string & file, UncheckedRoots & roots)
332332
{
333-
/* 64 is the starting buffer size gnu readlink uses... */
334-
auto bufsiz = ssize_t{64};
335-
try_again:
333+
constexpr auto bufsiz = PATH_MAX;
336334
char buf[bufsiz];
337335
auto res = readlink(file.c_str(), buf, bufsiz);
338336
if (res == -1) {
@@ -341,10 +339,7 @@ static void readProcLink(const std::string & file, UncheckedRoots & roots)
341339
throw SysError("reading symlink");
342340
}
343341
if (res == bufsiz) {
344-
if (SSIZE_MAX / 2 < bufsiz)
345-
throw Error("stupidly long symlink");
346-
bufsiz *= 2;
347-
goto try_again;
342+
throw Error("overly long symlink starting with '%1%'", std::string_view(buf, bufsiz));
348343
}
349344
if (res > 0 && buf[0] == '/')
350345
roots[std::string(static_cast<char *>(buf), res)]

0 commit comments

Comments
 (0)