buffer: throw exception when creating from non-Node.js Context#23938
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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.
nodejs-github-bot added buffer 
Uh oh!
There was an error while loading. Please reload this page.
vsemozhetbyt added the errors 
danbev commented Oct 31, 2018
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: nodejs#23938 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
Trott commented Nov 4, 2018
Landed in 9c7d191 (with @vsemozhetbyt's an/a nit). |
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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
codebytere commented Nov 29, 2018
@addaleax do you think this should be backported to |
addaleax commented Nov 29, 2018
@codebytere It might be nice to backport it in order to avoid conflicts, but it’s not super important. It sounds like maybe another commit should be backported first? Can you show the compilation error, that might help identify it? |
@addaleax so the throw helper initially took a ptr to an Environment, and was later changed to take an isolate; if i change the commit such that we have |
addaleax commented Nov 29, 2018
@codebytere I’ll try to backport #23879 (tomorrow), I think this sounds like that would solve the issue? :) |
codebytere commented Nov 29, 2018
i think so! thanks @addaleax ✨ |
| Environment* env = Environment::GetCurrent(isolate); | ||
| CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. | ||
| if (env == nullptr){ | ||
| callback(data, hint); |
There was a problem hiding this comment.
Belated review: this callback() call and the free() call on line 417 look a bit incongruent because they're not executed when the Buffer::New() calls below them fail.
Performing the action only sometimes is confusing, IMO. A caller that diligently checks the return value and frees the memory when it's empty will now (sometimes) double-free.
There was a problem hiding this comment.
@bnoordhuis I think it was more or less the idea to be more consistent here and always take ownership, because in other cases we already did free the data when failing (e.g. in the New(Isolate*, Local<String>, encoding) variant)…
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: nodejs/node#23938 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
8 participants
Throw an exception instead of crashing when attempting to create
Bufferobjects from a Context that is not associated witha Node.js
Environment.Possible alternatives for the future might be just returning
a plain
Uint8Array, or working on providingBufferfor allContexts.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes