Skip to content

Commit e4cd255

Browse files
addaleaxtargos
authored andcommitted
buffer: throw exception when creating from non-Node.js Context
Throw an exception instead of crashing when attempting to create `Buffer` objects from a Context that is not associated with a Node.js `Environment`. Possible alternatives for the future might be just returning a plain `Uint8Array`, or working on providing `Buffer` for all `Context`s. PR-URL: #23938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
1 parent e27f432 commit e4cd255

File tree

5 files changed

+70
-11
lines changed

5 files changed

+70
-11
lines changed

doc/api/errors.md

+13
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,19 @@ An attempt was made to register something that is not a function as an
613613
The type of an asynchronous resource was invalid. Note that users are also able
614614
to define their own types if using the public embedder API.
615615

616+
<a id="ERR_BUFFER_CONTEXT_NOT_AVAILABLE"></a>
617+
### ERR_BUFFER_CONTEXT_NOT_AVAILABLE
618+
619+
An attempt was made to create a Node.js `Buffer` instance from addon or embedder
620+
code, while in a JS engine Context that is not associated with a Node.js
621+
instance. The data passed to the `Buffer` method will have been released
622+
by the time the method returns.
623+
624+
When encountering this error, a possible alternative to creating a `Buffer`
625+
instance is to create a normal `Uint8Array`, which only differs in the
626+
prototype of the resulting object. `Uint8Array`s are generally accepted in all
627+
Node.js core APIs where `Buffer`s are; they are available in all Contexts.
628+
616629
<a id="ERR_BUFFER_OUT_OF_BOUNDS"></a>
617630
### ERR_BUFFER_OUT_OF_BOUNDS
618631

src/node_buffer.cc

+18-4
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,10 @@ MaybeLocal<Object> New(Isolate* isolate, size_t length) {
271271
EscapableHandleScope handle_scope(isolate);
272272
Local<Object> obj;
273273
Environment* env = Environment::GetCurrent(isolate);
274-
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
274+
if (env == nullptr) {
275+
THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
276+
return MaybeLocal<Object>();
277+
}
275278
if (Buffer::New(env, length).ToLocal(&obj))
276279
return handle_scope.Escape(obj);
277280
return Local<Object>();
@@ -314,7 +317,10 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
314317
MaybeLocal<Object> Copy(Isolate* isolate, const char* data, size_t length) {
315318
EscapableHandleScope handle_scope(isolate);
316319
Environment* env = Environment::GetCurrent(isolate);
317-
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
320+
if (env == nullptr) {
321+
THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
322+
return MaybeLocal<Object>();
323+
}
318324
Local<Object> obj;
319325
if (Buffer::Copy(env, data, length).ToLocal(&obj))
320326
return handle_scope.Escape(obj);
@@ -364,7 +370,11 @@ MaybeLocal<Object> New(Isolate* isolate,
364370
void* hint) {
365371
EscapableHandleScope handle_scope(isolate);
366372
Environment* env = Environment::GetCurrent(isolate);
367-
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
373+
if (env == nullptr) {
374+
callback(data, hint);
375+
THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
376+
return MaybeLocal<Object>();
377+
}
368378
Local<Object> obj;
369379
if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
370380
return handle_scope.Escape(obj);
@@ -403,7 +413,11 @@ MaybeLocal<Object> New(Environment* env,
403413
MaybeLocal<Object> New(Isolate* isolate, char* data, size_t length) {
404414
EscapableHandleScope handle_scope(isolate);
405415
Environment* env = Environment::GetCurrent(isolate);
406-
CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here.
416+
if (env == nullptr) {
417+
free(data);
418+
THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
419+
return MaybeLocal<Object>();
420+
}
407421
Local<Object> obj;
408422
if (Buffer::New(env, data, length).ToLocal(&obj))
409423
return handle_scope.Escape(obj);

src/node_errors.h

+3
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ namespace node {
2121
// a `Local<Value>` containing the TypeError with proper code and message
2222

2323
#define ERRORS_WITH_CODE(V) \
24+
V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, Error) \
2425
V(ERR_BUFFER_OUT_OF_BOUNDS, RangeError) \
2526
V(ERR_BUFFER_TOO_LARGE, Error) \
2627
V(ERR_CANNOT_TRANSFER_OBJECT, TypeError) \
@@ -64,6 +65,8 @@ namespace node {
6465
// Errors with predefined static messages
6566

6667
#define PREDEFINED_ERROR_MESSAGES(V) \
68+
V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, \
69+
"Buffer is not available for the current Context") \
6770
V(ERR_CANNOT_TRANSFER_OBJECT, "Cannot transfer object of unsupported type")\
6871
V(ERR_CLOSED_MESSAGE_PORT, "Cannot send data on closed MessagePort") \
6972
V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`") \

test/addons/non-node-context/binding.cc

+14-7
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include <node.h>
2+
#include <node_buffer.h>
23
#include <assert.h>
34

45
namespace {
@@ -15,6 +16,17 @@ using v8::Script;
1516
using v8::String;
1617
using v8::Value;
1718

19+
inline void MakeBufferInNewContext(
20+
const v8::FunctionCallbackInfo<v8::Value>& args) {
21+
Isolate* isolate = args.GetIsolate();
22+
Local<Context> context = Context::New(isolate);
23+
Context::Scope context_scope(context);
24+
25+
// This should throw an exception, rather than actually do anything.
26+
MaybeLocal<Object> buf = node::Buffer::Copy(isolate, "foo", 3);
27+
assert(buf.IsEmpty());
28+
}
29+
1830
inline void RunInNewContext(
1931
const v8::FunctionCallbackInfo<v8::Value>& args) {
2032
Isolate* isolate = args.GetIsolate();
@@ -41,13 +53,8 @@ inline void RunInNewContext(
4153
inline void Initialize(Local<Object> exports,
4254
Local<Value> module,
4355
Local<Context> context) {
44-
Isolate* isolate = context->GetIsolate();
45-
Local<String> key = String::NewFromUtf8(
46-
isolate, "runInNewContext", NewStringType::kNormal).ToLocalChecked();
47-
Local<Function> value = FunctionTemplate::New(isolate, RunInNewContext)
48-
->GetFunction(context)
49-
.ToLocalChecked();
50-
assert(exports->Set(context, key, value).IsJust());
56+
NODE_SET_METHOD(exports, "runInNewContext", RunInNewContext);
57+
NODE_SET_METHOD(exports, "makeBufferInNewContext", MakeBufferInNewContext);
5158
}
5259

5360
} // anonymous namespace
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
3+
const common = require('../../common');
4+
const assert = require('assert');
5+
const {
6+
makeBufferInNewContext
7+
} = require(`./build/${common.buildType}/binding`);
8+
9+
// Because the `Buffer` function and its protoype property only (currently)
10+
// exist in a Node.js instance’s main context, trying to create buffers from
11+
// another context throws an exception.
12+
13+
try {
14+
makeBufferInNewContext();
15+
} catch (exception) {
16+
assert.strictEqual(exception.constructor.name, 'Error');
17+
assert(!(exception.constructor instanceof Error));
18+
19+
assert.strictEqual(exception.code, 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE');
20+
assert.strictEqual(exception.message,
21+
'Buffer is not available for the current Context');
22+
}

0 commit comments

Comments
 (0)