From 5196613e8290a9ee81f1b9d88e7bc61cc3f64d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 18 Nov 2022 11:13:32 +0100 Subject: [PATCH 01/11] Use boost small vectors instead of VLAs in the primops VLAs are a dangerous feature, and their usage triggers an undefined behavior since theire size can be zero in some cases. So replace them with `boost::small_vector`s which fit the same goal but are safer. It's also incidentally consistently 1% faster on the benchmarks. --- src/libexpr/primops.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8d3a18526..e7587506a 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -29,7 +29,6 @@ #include - namespace nix { @@ -2729,8 +2728,8 @@ 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()]; - unsigned int found = 0; + boost::container::small_vector res(args[1]->listSize()); + size_t found = 0; for (auto v2 : args[1]->listItems()) { state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs"); @@ -3064,9 +3063,8 @@ 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()]; - unsigned int k = 0; + boost::container::small_vector vs(args[1]->listSize()); + size_t k = 0; bool same = true; for (unsigned int n = 0; n < args[1]->listSize(); ++n) { @@ -3450,7 +3448,7 @@ 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]; + boost::container::small_vector lists(nrLists); size_t len = 0; for (unsigned int n = 0; n < nrLists; ++n) { From ba3cb4a04949e043669299da5497bea27b944598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 18 Nov 2022 13:02:06 +0100 Subject: [PATCH 02/11] Remove all the occurences of VLAs There's generally no strict reason for using them, and they are somewhat fishy, so let's avoid them. --- src/libexpr/eval.cc | 13 ++++++++----- src/libstore/derivations.cc | 9 ++++----- src/libstore/gc.cc | 9 ++------- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index dfe81cbf7..d853b104b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -41,6 +41,7 @@ #include #include #include +#include #endif @@ -1691,7 +1692,8 @@ 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]; + assert(arity < 64); + Value * vArgs[64]; auto n = argsDone; for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp.left) vArgs[--n] = arg->primOpApp.right; @@ -1748,11 +1750,12 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v) Value vFun; fun->eval(state, env, vFun); - Value * vArgs[args.size()]; + + boost::container::small_vector 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); } @@ -1991,8 +1994,8 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) return result; }; - Value values[es->size()]; - Value * vTmpP = values; + boost::container::small_vector values(es->size()); + Value * vTmpP = values.data(); for (auto & [i_pos, i] : *es) { Value & vTmp = *vTmpP++; diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 1fecd1c97..6d9c8b9d6 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -151,11 +151,10 @@ StorePath writeDerivation(Store & store, /* Read string `s' from stream `str'. */ static void expect(std::istream & str, std::string_view s) { - char s2[s.size()]; - str.read(s2, s.size()); - std::string_view s2View { s2, s.size() }; - if (s2View != s) - throw FormatError("expected string '%s', got '%s'", s, s2View); + for (auto & c : s) { + if (str.get() != c) + throw FormatError("expected string '%1%'", s); + } } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 8d05ae4bd..ddec43fdc 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -330,9 +330,7 @@ typedef std::unordered_map> UncheckedRoots static void readProcLink(const std::string & file, UncheckedRoots & roots) { - /* 64 is the starting buffer size gnu readlink uses... */ - auto bufsiz = ssize_t{64}; -try_again: + constexpr auto bufsiz = PATH_MAX; char buf[bufsiz]; auto res = readlink(file.c_str(), buf, bufsiz); if (res == -1) { @@ -341,10 +339,7 @@ try_again: throw SysError("reading symlink"); } if (res == bufsiz) { - if (SSIZE_MAX / 2 < bufsiz) - throw Error("stupidly long symlink"); - bufsiz *= 2; - goto try_again; + throw Error("stupidly long symlink"); } if (res > 0 && buf[0] == '/') roots[std::string(static_cast(buf), res)] From 0daccb1121dfd5e98db3e41ba992b1b2c413dfc8 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 11:10:25 +0100 Subject: [PATCH 03/11] libexpr: Check primop arity earlier --- src/libexpr/eval.cc | 20 ++++++++++++++++++-- src/libexpr/eval.hh | 12 ++++++++++++ src/libexpr/tests/value/print.cc | 3 ++- src/libexpr/value.hh | 8 +------- 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d853b104b..1425eab97 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -723,6 +723,23 @@ void EvalState::addConstant(const std::string & name, Value * v, Constant info) } +void PrimOp::check() +{ + if (arity > maxPrimOpArity) { + throw Error("primop arity must not exceed %1%", maxPrimOpArity); + } +} + + +void Value::mkPrimOp(PrimOp * p) +{ + p->check(); + clearValue(); + internalType = tPrimOp; + primOp = p; +} + + Value * EvalState::addPrimOp(PrimOp && primOp) { /* Hack to make constants lazy: turn them into a application of @@ -1692,8 +1709,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. */ - assert(arity < 64); - Value * vArgs[64]; + Value * vArgs[maxPrimOpArity]; auto n = argsDone; for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp.left) vArgs[--n] = arg->primOpApp.right; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 048dff42b..5ee6359a8 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -18,6 +18,12 @@ namespace nix { +/** + * We put a limit on primop arity because it lets us use a fixed size array on + * the stack. 16 is already an impractical number of arguments. Use an attrset + * argument for such overly complicated functions. + */ +constexpr size_t maxPrimOpArity = 64; class Store; class EvalState; @@ -71,6 +77,12 @@ struct PrimOp * Optional experimental for this to be gated on. */ std::optional experimentalFeature; + + /** + * Validity check to be performed by functions that introduce primops, + * such as RegisterPrimOp() and Value::mkPrimOp(). + */ + void check(); }; /** diff --git a/src/libexpr/tests/value/print.cc b/src/libexpr/tests/value/print.cc index 5e96e12ec..a4f6fc014 100644 --- a/src/libexpr/tests/value/print.cc +++ b/src/libexpr/tests/value/print.cc @@ -114,7 +114,8 @@ TEST_F(ValuePrintingTests, vLambda) TEST_F(ValuePrintingTests, vPrimOp) { Value vPrimOp; - vPrimOp.mkPrimOp(nullptr); + PrimOp primOp{}; + vPrimOp.mkPrimOp(&primOp); test(vPrimOp, ""); } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 622e613ea..191cc30ba 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -354,13 +354,7 @@ public: // Value will be overridden anyways } - inline void mkPrimOp(PrimOp * p) - { - clearValue(); - internalType = tPrimOp; - primOp = p; - } - + void mkPrimOp(PrimOp * p); inline void mkPrimOpApp(Value * l, Value * r) { From 12c91a823e80b5e0a14a0abb0f34a6633b14bbfe Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 11:27:31 +0100 Subject: [PATCH 04/11] maxPrimOpArity: 64 -> 8 This makes stack usage significantly more compact, allowing larger amounts of data to be processed on the same stack. PrimOp functions with more than 8 positional (curried) arguments should use an attrset instead. --- src/libexpr/eval.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 5ee6359a8..ce798ed96 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -20,10 +20,10 @@ namespace nix { /** * We put a limit on primop arity because it lets us use a fixed size array on - * the stack. 16 is already an impractical number of arguments. Use an attrset + * the stack. 8 is already an impractical number of arguments. Use an attrset * argument for such overly complicated functions. */ -constexpr size_t maxPrimOpArity = 64; +constexpr size_t maxPrimOpArity = 8; class Store; class EvalState; From 9fa133dde5610dfb0605399ffea83081bda1c6fc Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 11:33:10 +0100 Subject: [PATCH 05/11] readProcLink: Replace unnecessary value judgement by actual info --- src/libstore/gc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index ddec43fdc..93fa60682 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -339,7 +339,7 @@ static void readProcLink(const std::string & file, UncheckedRoots & roots) throw SysError("reading symlink"); } if (res == bufsiz) { - throw Error("stupidly long symlink"); + throw Error("overly long symlink starting with '%1%'", std::string_view(buf, bufsiz)); } if (res > 0 && buf[0] == '/') roots[std::string(static_cast(buf), res)] From 206ece0f41142536a856c62c49bd202282f12db8 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 12:18:37 +0100 Subject: [PATCH 06/11] builtins.{any,all}: Use constant errorCtx Clang warned that the expanded code used to have a buffer overflow. Very strange, but also very avoidable. --- src/libexpr/primops.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index e7587506a..d104b7180 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3189,10 +3189,14 @@ static void anyOrAll(bool any, EvalState & state, const PosIdx pos, Value * * ar state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.") + (any ? "any" : "all")); state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.") + (any ? "any" : "all")); + std::string_view errorCtx = any + ? "while evaluating the return value of the function passed to builtins.any" + : "while evaluating the return value of the function passed to builtins.all"; + Value vTmp; for (auto elem : args[1]->listItems()) { state.callFunction(*args[0], *elem, vTmp, pos); - bool res = state.forceBool(vTmp, pos, std::string("while evaluating the return value of the function passed to builtins.") + (any ? "any" : "all")); + bool res = state.forceBool(vTmp, pos, errorCtx); if (res == any) { v.mkBool(any); return; From 91114a6fa48e2eb9399c23938eb12fdbd4fcda42 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 12:44:10 +0100 Subject: [PATCH 07/11] ExprCall::eval: Heap allocate at arity 5+ --- src/libexpr/eval.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 1425eab97..bfbda52ef 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1766,8 +1766,13 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v) Value vFun; fun->eval(state, env, vFun); - - boost::container::small_vector vArgs(args.size()); + // Empirical arity of Nixpkgs lambdas by regex e.g. ([a-zA-Z]+:(\s|(/\*.*\/)|(#.*\n))*){5} + // 2: over 4000 + // 3: about 300 + // 4: about 60 + // 5: under 10 + // This excluded attrset lambdas (`{...}:`). Contributions of mixed lambdas appears insignificant at ~150 total. + boost::container::small_vector vArgs(args.size()); for (size_t i = 0; i < args.size(); ++i) vArgs[i] = args[i]->maybeThunk(state, env); From 898c47384f651f51b3e4b63c271da274db8fca2e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 12:48:37 +0100 Subject: [PATCH 08/11] primops: Err on the side of less stack usage Try to stay away from stack overflows. These small vectors use stack space. Most instances will not need to allocate because in general most things are small, and large things are worth heap allocating. 16 * 3 * word = 384 bytes is still quite a bit, but these functions tend not to be part of deep recursions. --- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bfbda52ef..2fcbf3311 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2015,7 +2015,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) return result; }; - boost::container::small_vector values(es->size()); + boost::container::small_vector values(es->size()); Value * vTmpP = values.data(); for (auto & [i_pos, i] : *es) { diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index d104b7180..7aa212281 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2549,7 +2549,7 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args /* Get the attribute names to be removed. We keep them as Attrs instead of Symbols so std::set_difference can be used to remove them from attrs[0]. */ - boost::container::small_vector names; + boost::container::small_vector names; names.reserve(args[1]->listSize()); for (auto elem : args[1]->listItems()) { state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); @@ -3452,7 +3452,7 @@ 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(); - boost::container::small_vector lists(nrLists); + boost::container::small_vector lists(nrLists); size_t len = 0; for (unsigned int n = 0; n < nrLists; ++n) { From 1b9813e4e60836ddb1467efd50c572e7579ac945 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 13:04:27 +0100 Subject: [PATCH 09/11] primops: Name stack reservation limits --- src/libexpr/eval.cc | 3 ++- src/libexpr/primops.cc | 6 +++--- src/libexpr/primops.hh | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 2fcbf3311..8b0ada517 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1,6 +1,7 @@ #include "eval.hh" #include "eval-settings.hh" #include "hash.hh" +#include "primops.hh" #include "types.hh" #include "util.hh" #include "store-api.hh" @@ -2015,7 +2016,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) return result; }; - boost::container::small_vector values(es->size()); + boost::container::small_vector values(es->size()); Value * vTmpP = values.data(); for (auto & [i_pos, i] : *es) { diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7aa212281..adce95bed 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2728,7 +2728,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"); - boost::container::small_vector res(args[1]->listSize()); + boost::container::small_vector res(args[1]->listSize()); size_t found = 0; for (auto v2 : args[1]->listItems()) { @@ -3063,7 +3063,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"); - boost::container::small_vector vs(args[1]->listSize()); + boost::container::small_vector vs(args[1]->listSize()); size_t k = 0; bool same = true; @@ -3452,7 +3452,7 @@ 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(); - boost::container::small_vector lists(nrLists); + boost::container::small_vector lists(nrLists); size_t len = 0; for (unsigned int n = 0; n < nrLists; ++n) { diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 930e7f32a..1d5d5710d 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -8,6 +8,22 @@ namespace nix { +/** + * For functions where we do not expect deep recursion, we can use a sizable + * part of the stack a free allocation space. + * + * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. + */ +constexpr size_t nonRecursiveStackReservation = 256; + +/** + * Functions that maybe applied to self-similar inputs, such as concatMap on a + * tree, should reserve a smaller part of the stack for allocation. + * + * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. + */ +constexpr size_t conservativeStackReservation = 16; + struct RegisterPrimOp { typedef std::vector PrimOps; From a96be29db536177fdc284b51a3b2af44a70496e0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 13:13:01 +0100 Subject: [PATCH 10/11] removeAttrs: increase stack reservation to 64 --- src/libexpr/primops.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index adce95bed..e274c3c0c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2549,7 +2549,8 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args /* Get the attribute names to be removed. We keep them as Attrs instead of Symbols so std::set_difference can be used to remove them from attrs[0]. */ - boost::container::small_vector names; + // 64: large enough to fit the attributes of a derivation + boost::container::small_vector names; names.reserve(args[1]->listSize()); for (auto elem : args[1]->listItems()) { state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); From 4e27f1947a444a36d6a85f41cbf1afdc70ac6c4c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 16 Nov 2023 13:23:17 +0100 Subject: [PATCH 11/11] libexpr: Reduce nonRecursiveStackReservation 128 is still beyond the point where the allocation overhead is insignificant, but we don't anticipate to overflow for these use cases, so it's fine. --- src/libexpr/primops.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 1d5d5710d..45486608f 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -14,7 +14,7 @@ namespace nix { * * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. */ -constexpr size_t nonRecursiveStackReservation = 256; +constexpr size_t nonRecursiveStackReservation = 128; /** * Functions that maybe applied to self-similar inputs, such as concatMap on a