From d038a67bd3c6ed0d6452d595cf0af3115e14c48f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 01:20:12 +0100 Subject: 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 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'src/libstore') 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) -- cgit v1.2.3 From 55c58580be89e0f7ec6519bd7a7f2cded4fab382 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 14:31:23 +0100 Subject: Add withBuffer ... to avoid non-standard, unidiomatic alloca. --- src/libstore/derivations.cc | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) (limited to 'src/libstore') 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); + }); } -- cgit v1.2.3 From 6dd271b7b4a679ce3c2a8d84a39909bb9b60bf9f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 6 Jan 2022 15:11:37 +0100 Subject: withBuffer: avoid allocating a std::function --- src/libstore/derivations.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libstore') 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) -- cgit v1.2.3 From dec774811922d7ee220bb8e2488bc2f7abf61844 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 19 Jan 2022 15:20:46 +0100 Subject: 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 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) (limited to 'src/libstore') 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); } -- cgit v1.2.3 From 0407436b0f15900399d11da43178cd09fddba0af Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 21 Jan 2022 17:25:37 +0100 Subject: 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(-) (limited to 'src/libstore') 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; -- cgit v1.2.3