Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: throw exception when creating from non-Node.js Context #23938

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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
Contexts.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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 nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 28, 2018
@vsemozhetbyt vsemozhetbyt added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Oct 28, 2018
@danbev
Copy link
Contributor

danbev commented Oct 31, 2018

@Trott Trott self-assigned this Nov 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 4, 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
Copy link
Member

Trott commented Nov 4, 2018

Landed in 9c7d191 (with @vsemozhetbyt's an/a nit).

@Trott Trott closed this Nov 4, 2018
targos pushed a commit that referenced this pull request Nov 5, 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: #23938
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@codebytere
Copy link
Member

@addaleax do you think this should be backported to v10.x? it applies cleanly but there's drift in the error arguments that causes a crash if change the isolate arg to env so that it compiles.

@addaleax addaleax deleted the buffer-context-exception branch November 29, 2018 16:50
@addaleax
Copy link
Member Author

@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?

@codebytere
Copy link
Member

codebytere commented Nov 29, 2018

@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 THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(env); then it compiles but crashes in test; i'll paste the test here imminently

@addaleax
Copy link
Member Author

@codebytere I’ll try to backport #23879 (tomorrow), I think this sounds like that would solve the issue? :)

@codebytere
Copy link
Member

i think so! thanks @addaleax

@@ -364,7 +370,11 @@ MaybeLocal<Object> 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)…

deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants