From d038a67bd3c6ed0d6452d595cf0af3115e14c48f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 01:20:12 +0100 Subject: [PATCH 1/6] Fix segfault or stack overflow caused by large derivation fields This removes a dynamic stack allocation, making the derivation unparsing logic robust against overflows when large strings are added to a derivation. Overflow behavior depends on the platform and stack configuration. For instance, x86_64-linux/glibc behaves as (somewhat) expected: $ (ulimit -s 20000; nix-instantiate tests/lang/eval-okay-big-derivation-attr.nix) error: stack overflow (possible infinite recursion) $ (ulimit -s 40000; nix-instantiate tests/lang/eval-okay-big-derivation-attr.nix) error: expression does not evaluate to a derivation (or a set or list of those) However, on aarch64-darwin: $ nix-instantiate big-attr.nix ~ zsh: segmentation fault nix-instantiate big-attr.nix This indicates a slight flaw in the single stack protection page approach that is not encountered with normal stack frames. --- src/libstore/derivations.cc | 10 +++++++++- tests/big-derivation-attr.nix | 13 +++++++++++++ tests/simple.sh | 6 ++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 tests/big-derivation-attr.nix diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index b926bb711..616e78076 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -272,7 +272,15 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi static void printString(string & res, std::string_view s) { - char buf[s.size() * 2 + 2]; + char * buf; + size_t bufSize = s.size() * 2 + 2; + std::unique_ptr dynBuf; + if (bufSize < 0x10000) { + buf = (char *)alloca(bufSize); + } else { + dynBuf = decltype(dynBuf)(new char[bufSize]); + buf = dynBuf.get(); + } char * p = buf; *p++ = '"'; for (auto c : s) diff --git a/tests/big-derivation-attr.nix b/tests/big-derivation-attr.nix new file mode 100644 index 000000000..35c1187f6 --- /dev/null +++ b/tests/big-derivation-attr.nix @@ -0,0 +1,13 @@ +let + sixteenBytes = "0123456789abcdef"; + times16 = s: builtins.concatStringsSep "" [s s s s s s s s s s s s s s s s]; + exp = n: x: if n == 1 then x else times16 (exp (n - 1) x); + sixteenMegabyte = exp 6 sixteenBytes; +in +assert builtins.stringLength sixteenMegabyte == 16777216; +derivation { + name = "big-derivation-attr"; + builder = "/x"; + system = "y"; + bigAttr = sixteenMegabyte; +} diff --git a/tests/simple.sh b/tests/simple.sh index 15bd2bd16..50d44f93f 100644 --- a/tests/simple.sh +++ b/tests/simple.sh @@ -25,3 +25,9 @@ if test "$outPath" != "/foo/lfy1s6ca46rm5r6w4gg9hc0axiakjcnm-dependencies.drv"; echo "hashDerivationModulo appears broken, got $outPath" exit 1 fi + +outPath="$(NIX_REMOTE=local?store=/foo\&real=$TEST_ROOT/real-store nix-instantiate --readonly-mode big-derivation-attr.nix)" +if test "$outPath" != "/foo/xxiwa5zlaajv6xdjynf9yym9g319d6mn-big-derivation-attr.drv"; then + echo "big-derivation-attr.nix hash appears broken, got $outPath. Memory corruption in large drv attr?" + exit 1 +fi From 55c58580be89e0f7ec6519bd7a7f2cded4fab382 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 14:31:23 +0100 Subject: [PATCH 2/6] Add withBuffer ... to avoid non-standard, unidiomatic alloca. --- src/libstore/derivations.cc | 30 ++++++++++++------------------ src/libutil/util.hh | 9 +++++++++ 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 616e78076..21ea84dbf 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -272,25 +272,19 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi static void printString(string & res, std::string_view s) { - char * buf; size_t bufSize = s.size() * 2 + 2; - std::unique_ptr dynBuf; - if (bufSize < 0x10000) { - buf = (char *)alloca(bufSize); - } else { - dynBuf = decltype(dynBuf)(new char[bufSize]); - buf = dynBuf.get(); - } - char * p = buf; - *p++ = '"'; - for (auto c : s) - if (c == '\"' || c == '\\') { *p++ = '\\'; *p++ = c; } - else if (c == '\n') { *p++ = '\\'; *p++ = 'n'; } - else if (c == '\r') { *p++ = '\\'; *p++ = 'r'; } - else if (c == '\t') { *p++ = '\\'; *p++ = 't'; } - else *p++ = c; - *p++ = '"'; - res.append(buf, p - buf); + withBuffer(bufSize, [&](char buf[]) { + char * p = buf; + *p++ = '"'; + for (auto c : s) + if (c == '\"' || c == '\\') { *p++ = '\\'; *p++ = c; } + else if (c == '\n') { *p++ = '\\'; *p++ = 'n'; } + else if (c == '\r') { *p++ = '\\'; *p++ = 'r'; } + else if (c == '\t') { *p++ = '\\'; *p++ = 't'; } + else *p++ = c; + *p++ = '"'; + res.append(buf, p - buf); + }); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 369c44f78..840acd67b 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -671,5 +671,14 @@ template overloaded(Ts...) -> overloaded; std::string showBytes(uint64_t bytes); +template inline R withBuffer(size_t size, std::function fun) { + if (size < 0x10000) { + T buf[size]; + return fun(buf); + } else { + auto buf = std::unique_ptr(new T[size]); + return fun(buf.get()); + } +} } From 6dd271b7b4a679ce3c2a8d84a39909bb9b60bf9f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 15:11:37 +0100 Subject: [PATCH 3/6] withBuffer: avoid allocating a std::function --- src/libstore/derivations.cc | 2 +- src/libutil/util.hh | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 21ea84dbf..f8fe6a59c 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -273,7 +273,7 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi static void printString(string & res, std::string_view s) { size_t bufSize = s.size() * 2 + 2; - withBuffer(bufSize, [&](char buf[]) { + withBuffer(bufSize, [&](char *buf) { char * p = buf; *p++ = '"'; for (auto c : s) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 840acd67b..407505be9 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -671,7 +671,10 @@ template overloaded(Ts...) -> overloaded; std::string showBytes(uint64_t bytes); -template inline R withBuffer(size_t size, std::function fun) { +template +inline auto withBuffer(size_t size, Fn fun) + -> std::invoke_result_t +{ if (size < 0x10000) { T buf[size]; return fun(buf); From 624f18ad90e3784878e6d303858f59651af68f61 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 10 Jan 2022 21:17:17 +0100 Subject: [PATCH 4/6] withBuffer: Make sure to hit the stack protector --- src/libutil/util.hh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 407505be9..e2192987c 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -671,15 +671,28 @@ template overloaded(Ts...) -> overloaded; std::string showBytes(uint64_t bytes); +/** + `withBuffer(size, fun)` applies `fun` to a temporary `char` array of size `size`. + The array will be allocated either on the stack or on the heap depending on its size +*/ template -inline auto withBuffer(size_t size, Fn fun) +inline auto withBuffer(size_t n, Fn fun) -> std::invoke_result_t { - if (size < 0x10000) { - T buf[size]; + // Large stack allocations can skip past the stack protection page. + const size_t stack_protection_size = 4096; + // We reduce the max stack allocated buffer by an extra amount to increase + // the chance of hitting it, even when `fun`'s first access is some distance + // into its *further* stack frame, particularly if the call was inlined and + // therefore not writing a frame pointer. + const size_t play = 64 * sizeof(char *); // 512B on 64b archs + size_t size_bytes = n * sizeof(T); + + if (size_bytes < stack_protection_size - play) { + T buf[n]; return fun(buf); } else { - auto buf = std::unique_ptr(new T[size]); + auto buf = std::unique_ptr(new T[n]); return fun(buf.get()); } } From dec774811922d7ee220bb8e2488bc2f7abf61844 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 19 Jan 2022 15:20:46 +0100 Subject: [PATCH 5/6] Replace withBuffer by boost small_vector Although this will leave gaps in the stack, the performance impact of those should be insignificant and we get a simpler solution this way. --- src/libstore/derivations.cc | 35 ++++++++++++++++++++++------------- src/libutil/util.hh | 25 ------------------------- 2 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index f8fe6a59c..5233cfe67 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -4,6 +4,7 @@ #include "util.hh" #include "worker-protocol.hh" #include "fs-accessor.hh" +#include namespace nix { @@ -272,19 +273,27 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi static void printString(string & res, std::string_view s) { - size_t bufSize = s.size() * 2 + 2; - withBuffer(bufSize, [&](char *buf) { - char * p = buf; - *p++ = '"'; - for (auto c : s) - if (c == '\"' || c == '\\') { *p++ = '\\'; *p++ = c; } - else if (c == '\n') { *p++ = '\\'; *p++ = 'n'; } - else if (c == '\r') { *p++ = '\\'; *p++ = 'r'; } - else if (c == '\t') { *p++ = '\\'; *p++ = 't'; } - else *p++ = c; - *p++ = '"'; - res.append(buf, p - buf); - }); + // Large stack allocations can skip past the stack protection page. + const size_t stack_protection_size = 4096; + // We reduce the max stack allocated buffer by an extra amount to increase + // the chance of hitting it, even when `fun`'s first access is some distance + // into its *further* stack frame, particularly if the call was inlined and + // therefore not writing a frame pointer. + const size_t play = 64 * sizeof(char *); // 512B on 64b archs + + boost::container::small_vector buffer; + buffer.reserve(s.size() * 2 + 2); + char * buf = buffer.data(); + char * p = buf; + *p++ = '"'; + for (auto c : s) + if (c == '\"' || c == '\\') { *p++ = '\\'; *p++ = c; } + else if (c == '\n') { *p++ = '\\'; *p++ = 'n'; } + else if (c == '\r') { *p++ = '\\'; *p++ = 'r'; } + else if (c == '\t') { *p++ = '\\'; *p++ = 't'; } + else *p++ = c; + *p++ = '"'; + res.append(buf, p - buf); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index e2192987c..369c44f78 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -671,30 +671,5 @@ template overloaded(Ts...) -> overloaded; std::string showBytes(uint64_t bytes); -/** - `withBuffer(size, fun)` applies `fun` to a temporary `char` array of size `size`. - The array will be allocated either on the stack or on the heap depending on its size -*/ -template -inline auto withBuffer(size_t n, Fn fun) - -> std::invoke_result_t -{ - // Large stack allocations can skip past the stack protection page. - const size_t stack_protection_size = 4096; - // We reduce the max stack allocated buffer by an extra amount to increase - // the chance of hitting it, even when `fun`'s first access is some distance - // into its *further* stack frame, particularly if the call was inlined and - // therefore not writing a frame pointer. - const size_t play = 64 * sizeof(char *); // 512B on 64b archs - size_t size_bytes = n * sizeof(T); - - if (size_bytes < stack_protection_size - play) { - T buf[n]; - return fun(buf); - } else { - auto buf = std::unique_ptr(new T[n]); - return fun(buf.get()); - } -} } From 0407436b0f15900399d11da43178cd09fddba0af Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 21 Jan 2022 17:25:37 +0100 Subject: [PATCH 6/6] derivations.cc: Use larger buffer in printString If we want to be careful about hitting the stack protector page, we should use `-fstack-check` instead. Co-authored-by: Eelco Dolstra --- src/libstore/derivations.cc | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 5233cfe67..3e3d50144 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -273,15 +273,7 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi static void printString(string & res, std::string_view s) { - // Large stack allocations can skip past the stack protection page. - const size_t stack_protection_size = 4096; - // We reduce the max stack allocated buffer by an extra amount to increase - // the chance of hitting it, even when `fun`'s first access is some distance - // into its *further* stack frame, particularly if the call was inlined and - // therefore not writing a frame pointer. - const size_t play = 64 * sizeof(char *); // 512B on 64b archs - - boost::container::small_vector buffer; + boost::container::small_vector buffer; buffer.reserve(s.size() * 2 + 2); char * buf = buffer.data(); char * p = buf;