Skip to content

Commit

Permalink
src: fix check for accepting Buffers into Node’s allocator
Browse files Browse the repository at this point in the history
This condition was incorrect. We currently take the fallback
path in default Node builds, which always works, but may come with
some overhead, whereas the intention was that we use the fast path
in this condition.

This is causing issues for embedders, because we would erroneously
try to take the fast path when they don’t provide a Node.js-style
`ArrayBufferAlloactor`, and crash as a consequence of that.

This also requires us to relax the check in the debugging ArrayBuffer
allocator a bit, because since d117e41, 0-sized ArrayBuffers
may actually point to allocations of size 1. Previously, that wasn’t
caught because the fallback path circumvented our ArrayBufferAllocator.

Refs: 84e02b1#r33116006

PR-URL: #27174
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
  • Loading branch information
addaleax committed Apr 14, 2019
1 parent d4e7431 commit 427fce7
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,11 @@ void DebuggingArrayBufferAllocator::UnregisterPointerInternal(void* data,
if (data == nullptr) return;
auto it = allocations_.find(data);
CHECK_NE(it, allocations_.end());
CHECK_EQ(it->second, size);
if (size > 0) {
// We allow allocations with size 1 for 0-length buffers to avoid having
// to deal with nullptr values.
CHECK_EQ(it->second, size);
}
allocations_.erase(it);
}

Expand Down
2 changes: 1 addition & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ MaybeLocal<Object> New(Environment* env,
}

if (uses_malloc) {
if (env->isolate_data()->uses_node_allocator()) {
if (!env->isolate_data()->uses_node_allocator()) {
// 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); };
Expand Down

0 comments on commit 427fce7

Please sign in to comment.