Skip to content

Commit

Permalink
src: allocate Buffer memory using ArrayBuffer allocator
Browse files Browse the repository at this point in the history
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@f61bae3440e. 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 84e02b1
Show file tree
Hide file tree
Showing 16 changed files with 256 additions and 319 deletions.
98 changes: 40 additions & 58 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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));
}


Expand All @@ -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());
}


Expand All @@ -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());
}


Expand Down Expand Up @@ -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);
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 84e02b1

Please sign in to comment.