From 9c7d19104d1f03f70658a6834a7d0baac7d38724 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 28 Oct 2018 15:59:20 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/23938 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- doc/api/errors.md | 13 +++++++++++ src/node_buffer.cc | 22 +++++++++++++++---- src/node_errors.h | 3 +++ test/addons/non-node-context/binding.cc | 21 ++++++++++++------ .../non-node-context/test-make-buffer.js | 22 +++++++++++++++++++ 5 files changed, 70 insertions(+), 11 deletions(-) create mode 100644 test/addons/non-node-context/test-make-buffer.js diff --git a/doc/api/errors.md b/doc/api/errors.md index bdabc48a7ddd72..aecb5282e79808 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -613,6 +613,19 @@ An attempt was made to register something that is not a function as an The type of an asynchronous resource was invalid. Note that users are also able to define their own types if using the public embedder API. + +### ERR_BUFFER_CONTEXT_NOT_AVAILABLE + +An attempt was made to create a Node.js `Buffer` instance from addon or embedder +code, while in a JS engine Context that is not associated with a Node.js +instance. The data passed to the `Buffer` method will have been released +by the time the method returns. + +When encountering this error, a possible alternative to creating a `Buffer` +instance is to create a normal `Uint8Array`, which only differs in the +prototype of the resulting object. `Uint8Array`s are generally accepted in all +Node.js core APIs where `Buffer`s are; they are available in all Contexts. + ### ERR_BUFFER_OUT_OF_BOUNDS diff --git a/src/node_buffer.cc b/src/node_buffer.cc index d4f7c751634b4d..2a2fb45e7abf15 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -271,7 +271,10 @@ MaybeLocal New(Isolate* isolate, size_t length) { EscapableHandleScope handle_scope(isolate); Local obj; Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } if (Buffer::New(env, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); @@ -314,7 +317,10 @@ MaybeLocal New(Environment* env, size_t length) { MaybeLocal Copy(Isolate* isolate, const char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::Copy(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -364,7 +370,11 @@ MaybeLocal New(Isolate* isolate, void* hint) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + callback(data, hint); + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -403,7 +413,11 @@ MaybeLocal New(Environment* env, MaybeLocal New(Isolate* isolate, char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + free(data); + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::New(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); diff --git a/src/node_errors.h b/src/node_errors.h index a958eccf8af4b9..a435695693ba18 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -21,6 +21,7 @@ namespace node { // a `Local` containing the TypeError with proper code and message #define ERRORS_WITH_CODE(V) \ + V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, Error) \ V(ERR_BUFFER_OUT_OF_BOUNDS, RangeError) \ V(ERR_BUFFER_TOO_LARGE, Error) \ V(ERR_CANNOT_TRANSFER_OBJECT, TypeError) \ @@ -64,6 +65,8 @@ namespace node { // Errors with predefined static messages #define PREDEFINED_ERROR_MESSAGES(V) \ + V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, \ + "Buffer is not available for the current Context") \ V(ERR_CANNOT_TRANSFER_OBJECT, "Cannot transfer object of unsupported type")\ V(ERR_CLOSED_MESSAGE_PORT, "Cannot send data on closed MessagePort") \ V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`") \ diff --git a/test/addons/non-node-context/binding.cc b/test/addons/non-node-context/binding.cc index 324f5c5a1ef16f..776786cef5180f 100644 --- a/test/addons/non-node-context/binding.cc +++ b/test/addons/non-node-context/binding.cc @@ -1,4 +1,5 @@ #include +#include #include namespace { @@ -15,6 +16,17 @@ using v8::Script; using v8::String; using v8::Value; +inline void MakeBufferInNewContext( + const v8::FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = Context::New(isolate); + Context::Scope context_scope(context); + + // This should throw an exception, rather than actually do anything. + MaybeLocal buf = node::Buffer::Copy(isolate, "foo", 3); + assert(buf.IsEmpty()); +} + inline void RunInNewContext( const v8::FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); @@ -41,13 +53,8 @@ inline void RunInNewContext( inline void Initialize(Local exports, Local module, Local context) { - Isolate* isolate = context->GetIsolate(); - Local key = String::NewFromUtf8( - isolate, "runInNewContext", NewStringType::kNormal).ToLocalChecked(); - Local value = FunctionTemplate::New(isolate, RunInNewContext) - ->GetFunction(context) - .ToLocalChecked(); - assert(exports->Set(context, key, value).IsJust()); + NODE_SET_METHOD(exports, "runInNewContext", RunInNewContext); + NODE_SET_METHOD(exports, "makeBufferInNewContext", MakeBufferInNewContext); } } // anonymous namespace diff --git a/test/addons/non-node-context/test-make-buffer.js b/test/addons/non-node-context/test-make-buffer.js new file mode 100644 index 00000000000000..9b17fa4ee930ae --- /dev/null +++ b/test/addons/non-node-context/test-make-buffer.js @@ -0,0 +1,22 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const { + makeBufferInNewContext +} = require(`./build/${common.buildType}/binding`); + +// Because the `Buffer` function and its protoype property only (currently) +// exist in a Node.js instance’s main context, trying to create buffers from +// another context throws an exception. + +try { + makeBufferInNewContext(); +} catch (exception) { + assert.strictEqual(exception.constructor.name, 'Error'); + assert(!(exception.constructor instanceof Error)); + + assert.strictEqual(exception.code, 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE'); + assert.strictEqual(exception.message, + 'Buffer is not available for the current Context'); +}