Skip to content
Permalink
Browse files
src: allocate Buffer memory using ArrayBuffer allocator
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s electron/node@f61bae3. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
addaleax committed Feb 25, 2019
1 parent 6c257cd commit 84e02b178ad14fae0df2a514e8a39bfa50ffdc2d
Showing with 256 additions and 319 deletions.
  1. +40 −58 src/node_buffer.cc
  2. +129 −154 src/node_crypto.cc
  3. +7 −8 src/node_crypto.h
  4. +11 −12 src/node_http2.cc
  5. +1 −0 src/node_http2.h
  6. +2 −3 src/node_http_parser_impl.h
  7. +7 −5 src/node_internals.h
  8. +8 −0 src/node_messaging.cc
  9. +2 −8 src/node_native_module.cc
  10. +4 −1 src/node_serdes.cc
  11. +3 −12 src/stream_base-inl.h
  12. +21 −26 src/stream_base.cc
  13. +4 −10 src/stream_base.h
  14. +7 −8 src/stream_pipe.cc
  15. +1 −1 src/stream_pipe.h
  16. +9 −13 src/udp_wrap.cc
@@ -54,17 +54,6 @@
size_t length = end - start;

namespace node {

namespace {

inline void* BufferMalloc(size_t length) {
return per_process::cli_options->zero_fill_all_buffers ?
node::UncheckedCalloc(length) :
node::UncheckedMalloc(length);
}

} // namespace

namespace Buffer {

using v8::ArrayBuffer;
@@ -260,7 +249,7 @@ MaybeLocal<Object> New(Isolate* isolate,
char* data = nullptr;

if (length > 0) {
data = static_cast<char*>(BufferMalloc(length));
data = UncheckedMalloc(length);

if (data == nullptr) {
THROW_ERR_MEMORY_ALLOCATION_FAILED(isolate);
@@ -278,13 +267,7 @@ MaybeLocal<Object> New(Isolate* isolate,
}
}

Local<Object> buf;
if (New(isolate, data, actual).ToLocal(&buf))
return scope.Escape(buf);

// Object failed to be created. Clean up resources.
free(data);
return Local<Object>();
return scope.EscapeMaybe(New(isolate, data, actual));
}


@@ -311,26 +294,16 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
return Local<Object>();
}

void* data;
AllocatedBuffer ret(env);
if (length > 0) {
data = BufferMalloc(length);
if (data == nullptr) {
ret = env->AllocateManaged(length, false);
if (ret.data() == nullptr) {
THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
return Local<Object>();
}
} else {
data = nullptr;
}

Local<ArrayBuffer> ab =
ArrayBuffer::New(env->isolate(),
data,
length,
ArrayBufferCreationMode::kInternalized);
Local<Object> obj;
if (Buffer::New(env, ab, 0, length).ToLocal(&obj))
return scope.Escape(obj);
return Local<Object>();
return scope.EscapeMaybe(ret.ToBuffer());
}


@@ -357,28 +330,18 @@ MaybeLocal<Object> Copy(Environment* env, const char* data, size_t length) {
return Local<Object>();
}

void* new_data;
AllocatedBuffer ret(env);
if (length > 0) {
CHECK_NOT_NULL(data);
new_data = node::UncheckedMalloc(length);
if (new_data == nullptr) {
ret = env->AllocateManaged(length, false);
if (ret.data() == nullptr) {
THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
return Local<Object>();
}
memcpy(new_data, data, length);
} else {
new_data = nullptr;
memcpy(ret.data(), data, length);
}

Local<ArrayBuffer> ab =
ArrayBuffer::New(env->isolate(),
new_data,
length,
ArrayBufferCreationMode::kInternalized);
Local<Object> obj;
if (Buffer::New(env, ab, 0, length).ToLocal(&obj))
return scope.Escape(obj);
return Local<Object>();
return scope.EscapeMaybe(ret.ToBuffer());
}


@@ -425,7 +388,8 @@ MaybeLocal<Object> New(Environment* env,
return scope.Escape(ui.ToLocalChecked());
}


// Warning: This function needs `data` to be allocated with malloc() and not
// necessarily isolate's ArrayBuffer::Allocator.
MaybeLocal<Object> New(Isolate* isolate, char* data, size_t length) {
EscapableHandleScope handle_scope(isolate);
Environment* env = Environment::GetCurrent(isolate);
@@ -435,18 +399,37 @@ MaybeLocal<Object> New(Isolate* isolate, char* data, size_t length) {
return MaybeLocal<Object>();
}
Local<Object> obj;
if (Buffer::New(env, data, length).ToLocal(&obj))
if (Buffer::New(env, data, length, true).ToLocal(&obj))
return handle_scope.Escape(obj);
return Local<Object>();
}


MaybeLocal<Object> New(Environment* env, char* data, size_t length) {
// Warning: If this call comes through the public node_buffer.h API,
// the contract for this function is that `data` is allocated with malloc()
// and not necessarily isolate's ArrayBuffer::Allocator.
MaybeLocal<Object> New(Environment* env,
char* data,
size_t length,
bool uses_malloc) {
if (length > 0) {
CHECK_NOT_NULL(data);
CHECK(length <= kMaxLength);
}

if (uses_malloc) {
if (env->isolate_data()->uses_node_allocator()) {

This comment has been minimized.

Copy link
@zcbenz

zcbenz Apr 10, 2019

Contributor

@addaleax It seems that this condition was meant to be not uses_node_allocator? I just encountered a crash caused by the CHECK in the else code, but I'm not sure if reverting the condition would be the correct fix.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 10, 2019

Author Member

@zcbenz Yes, that’s exactly it. Thanks for noticing! I’ve opened a PR @ #27174

// We don't know for sure that the allocator is malloc()-based, so we need
// to fall back to the FreeCallback variant.
auto free_callback = [](char* data, void* hint) { free(data); };
return New(env, data, length, free_callback, nullptr);
} else {
// This is malloc()-based, so we can acquire it into our own
// ArrayBufferAllocator.
CHECK_NOT_NULL(env->isolate_data()->node_allocator());
env->isolate_data()->node_allocator()->RegisterPointer(data, length);
}
}

Local<ArrayBuffer> ab =
ArrayBuffer::New(env->isolate(),
data,
@@ -1053,15 +1036,13 @@ static void EncodeUtf8String(const FunctionCallbackInfo<Value>& args) {

Local<String> str = args[0].As<String>();
size_t length = str->Utf8Length(isolate);
char* data = node::UncheckedMalloc(length);
AllocatedBuffer buf = env->AllocateManaged(length);
str->WriteUtf8(isolate,
data,
buf.data(),
-1, // We are certain that `data` is sufficiently large
nullptr,
String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8);
auto array_buf = ArrayBuffer::New(
isolate, data, length, ArrayBufferCreationMode::kInternalized);
auto array = Uint8Array::New(array_buf, 0, length);
auto array = Uint8Array::New(buf.ToArrayBuffer(), 0, length);
args.GetReturnValue().Set(array);
}

@@ -1123,7 +1104,8 @@ void Initialize(Local<Object> target,

// It can be a nullptr when running inside an isolate where we
// do not own the ArrayBuffer allocator.
if (uint32_t* zero_fill_field = env->isolate_data()->zero_fill_field()) {
if (ArrayBufferAllocator* allocator = env->isolate_data()->node_allocator()) {
uint32_t* zero_fill_field = allocator->zero_fill_field();
Local<ArrayBuffer> array_buffer = ArrayBuffer::New(
env->isolate(), zero_fill_field, sizeof(*zero_fill_field));
CHECK(target

0 comments on commit 84e02b1

Please sign in to comment.