Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: NixOS/nix
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 5e6eabe1551f
Choose a base ref
...
head repository: NixOS/nix
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 387f824cab50
Choose a head ref
  • 3 commits
  • 4 files changed
  • 2 contributors

Commits on Oct 30, 2020

  1. 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
    roberth committed Oct 30, 2020
    Copy the full SHA
    c4d903d View commit details
  2. BoehmGCStackAllocator: increase stack size to 8MB

    The default stack size was not based on the normal stack size and
    was too small.
    roberth committed Oct 30, 2020
    Copy the full SHA
    b43c13a View commit details

Commits on Nov 5, 2020

  1. Merge pull request #4206 from hercules-ci/fix-coroutine-gc

    Fix memory corruption caused by GC-invisible coroutine stacks
    edolstra authored Nov 5, 2020
    Copy the full SHA
    387f824 View commit details
Showing with 80 additions and 2 deletions.
  1. +31 −0 src/libexpr/eval.cc
  2. +1 −1 src/libexpr/local.mk
  3. +34 −1 src/libutil/serialise.cc
  4. +14 −0 src/libutil/serialise.hh
31 changes: 31 additions & 0 deletions src/libexpr/eval.cc
Original file line number Diff line number Diff line change
@@ -27,6 +27,10 @@
#include <gc/gc.h>
#include <gc/gc_cpp.h>

#include <boost/coroutine2/coroutine.hpp>
#include <boost/coroutine2/protected_fixedsize_stack.hpp>
#include <boost/context/stack_context.hpp>

#endif

namespace nix {
@@ -220,6 +224,31 @@ 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 {
// 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<std::size_t>(8 * 1024 * 1024))
};

public:
boost::context::stack_context allocate() override {
auto sctx = stack.allocate();
GC_add_roots(static_cast<char *>(sctx.sp) - sctx.size, sctx.sp);
return sctx;
}

void deallocate(boost::context::stack_context sctx) override {
GC_remove_roots(static_cast<char *>(sctx.sp) - sctx.size, sctx.sp);
stack.deallocate(sctx);
}

};

static BoehmGCStackAllocator boehmGCStackAllocator;

#endif


@@ -257,6 +286,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
2 changes: 1 addition & 1 deletion src/libexpr/local.mk
Original file line number Diff line number Diff line change
@@ -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
35 changes: 34 additions & 1 deletion src/libutil/serialise.cc
Original file line number Diff line number Diff line change
@@ -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<Source> sinkToSource(
std::function<void(Sink &)> fun,
std::function<void()> eof)
@@ -195,7 +228,7 @@ std::unique_ptr<Source> 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));
});
14 changes: 14 additions & 0 deletions src/libutil/serialise.hh
Original file line number Diff line number Diff line change
@@ -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;
};

}