From c4d903ddb009aa6472530699e154d85a24eac51d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 30 Oct 2020 20:55:53 +0100 Subject: [PATCH 1/2] Fix memory corruption caused by GC-invisible coroutine stacks Crucially this introduces BoehmGCStackAllocator, but it also adds a bunch of wiring to avoid making libutil depend on bdw-gc. Part of the solutions for #4178, #4200 --- src/libexpr/eval.cc | 26 ++++++++++++++++++++++++++ src/libexpr/local.mk | 2 +- src/libutil/serialise.cc | 35 ++++++++++++++++++++++++++++++++++- src/libutil/serialise.hh | 14 ++++++++++++++ 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 4de87d647..ea7ba0a6a 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -27,6 +27,10 @@ #include #include +#include +#include +#include + #endif namespace nix { @@ -220,6 +224,26 @@ static void * oomHandler(size_t requested) /* Convert this to a proper C++ exception. */ throw std::bad_alloc(); } + +class BoehmGCStackAllocator : public StackAllocator { + boost::coroutines2::protected_fixedsize_stack stack; + + public: + boost::context::stack_context allocate() override { + auto sctx = stack.allocate(); + GC_add_roots(static_cast(sctx.sp) - sctx.size, sctx.sp); + return sctx; + } + + void deallocate(boost::context::stack_context sctx) override { + GC_remove_roots(static_cast(sctx.sp) - sctx.size, sctx.sp); + stack.deallocate(sctx); + } + +}; + +static BoehmGCStackAllocator boehmGCStackAllocator; + #endif @@ -257,6 +281,8 @@ void initGC() GC_set_oom_fn(oomHandler); + StackAllocator::defaultAllocator = &boehmGCStackAllocator; + /* Set the initial heap size to something fairly big (25% of physical RAM, up to a maximum of 384 MiB) so that in most cases we don't need to garbage collect at all. (Collection has a diff --git a/src/libexpr/local.mk b/src/libexpr/local.mk index 687a8ccda..a5422169d 100644 --- a/src/libexpr/local.mk +++ b/src/libexpr/local.mk @@ -15,7 +15,7 @@ libexpr_CXXFLAGS += -I src/libutil -I src/libstore -I src/libfetchers -I src/lib libexpr_LIBS = libutil libstore libfetchers -libexpr_LDFLAGS = +libexpr_LDFLAGS = -lboost_context ifneq ($(OS), FreeBSD) libexpr_LDFLAGS += -ldl endif diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 5c9f6f901..28f6968d0 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -171,6 +171,39 @@ size_t StringSource::read(unsigned char * data, size_t len) #error Coroutines are broken in this version of Boost! #endif +/* A concrete datatype allow virtual dispatch of stack allocation methods. */ +struct VirtualStackAllocator { + StackAllocator *allocator = StackAllocator::defaultAllocator; + + boost::context::stack_context allocate() { + return allocator->allocate(); + } + + void deallocate(boost::context::stack_context sctx) { + allocator->deallocate(sctx); + } +}; + + +/* This class reifies the default boost coroutine stack allocation strategy with + a virtual interface. */ +class DefaultStackAllocator : public StackAllocator { + boost::coroutines2::default_stack stack; + + boost::context::stack_context allocate() { + return stack.allocate(); + } + + void deallocate(boost::context::stack_context sctx) { + deallocate(sctx); + } +}; + +static DefaultStackAllocator defaultAllocatorSingleton; + +StackAllocator *StackAllocator::defaultAllocator = &defaultAllocatorSingleton; + + std::unique_ptr sinkToSource( std::function fun, std::function eof) @@ -195,7 +228,7 @@ std::unique_ptr sinkToSource( size_t read(unsigned char * data, size_t len) override { if (!coro) - coro = coro_t::pull_type([&](coro_t::push_type & yield) { + coro = coro_t::pull_type(VirtualStackAllocator{}, [&](coro_t::push_type & yield) { LambdaSink sink([&](const unsigned char * data, size_t len) { if (len) yield(std::string((const char *) data, len)); }); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index d7fe0b81e..5c7d3ce76 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -5,6 +5,7 @@ #include "types.hh" #include "util.hh" +namespace boost::context { struct stack_context; } namespace nix { @@ -497,5 +498,18 @@ struct FramedSink : nix::BufferedSink }; }; +/* Stack allocation strategy for sinkToSource. + Mutable to avoid a boehm gc dependency in libutil. + + boost::context doesn't provide a virtual class, so we define our own. + */ +struct StackAllocator { + virtual boost::context::stack_context allocate() = 0; + virtual void deallocate(boost::context::stack_context sctx) = 0; + + /* The stack allocator to use in sinkToSource and potentially elsewhere. + It is reassigned by the initGC() method in libexpr. */ + static StackAllocator *defaultAllocator; +}; } From b43c13a9161daf1801188e61104debafa5243fe1 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 30 Oct 2020 23:18:24 +0100 Subject: [PATCH 2/2] BoehmGCStackAllocator: increase stack size to 8MB The default stack size was not based on the normal stack size and was too small. --- src/libexpr/eval.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index ea7ba0a6a..486a9fc1a 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -226,7 +226,12 @@ static void * oomHandler(size_t requested) } class BoehmGCStackAllocator : public StackAllocator { - boost::coroutines2::protected_fixedsize_stack stack; + boost::coroutines2::protected_fixedsize_stack stack { + // We allocate 8 MB, the default max stack size on NixOS. + // A smaller stack might be quicker to allocate but reduces the stack + // depth available for source filter expressions etc. + std::max(boost::context::stack_traits::default_size(), static_cast(8 * 1024 * 1024)) + }; public: boost::context::stack_context allocate() override {