buffer: neuter external nullptr buffers#3624
Conversation
|
Fixes the issue for me. 👍 |
|
Not that you need help kicking of a CI job, but I did it anyway: https://ci.nodejs.org/job/node-test-pull-request/657/ |
|
CI looks green, thank you @Trott |
|
cc @nodejs/collaborators I would like to get LGTM from either @trevnorris or @bnoordhuis before landing it. |
|
btw, other reviews are more than welcome! |
|
LGTM Would you consider this a v8 bug? |
src/node_buffer.cc
Outdated
There was a problem hiding this comment.
Can you add a comment explaining why the Neuter() call is necessary?
|
LGTM with comments. |
There was a problem hiding this comment.
Is this flag necessary here?
There was a problem hiding this comment.
Yes. Without it, v8::Isolate::RequestGarbageCollectionForTesting() aborts with a run-time error.
There was a problem hiding this comment.
Ah ok, didn't realize that
Neuter external `nullptr` buffers, otherwise their contents will be
materialized on access, and the buffer instance will be internalized.
This leads to a crash like this:
v8::ArrayBuffer::Neuter Only externalized ArrayBuffers can be
neutered
Fix: nodejs#3619
|
@trevnorris I'm not sure, maybe a documentation bug. Filed an issue: https://code.google.com/p/v8/issues/detail?id=4530 |
|
@nodejs/release how do I mark it to be landed on v5? |
|
Landed in master in 827ee49, thank you everyone! |
Neuter external `nullptr` buffers, otherwise their contents will be
materialized on access, and the buffer instance will be internalized.
This leads to a crash like this:
v8::ArrayBuffer::Neuter Only externalized ArrayBuffers can be
neutered
Fix: #3619
PR-URL: #3624
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
|
I'm unable to find anything about who can propose releases and how, but is this a sufficiently significant bug/fix that a 5.0.1 release should happen as soon as is reasonable? Breaking nativescript and ffi seems significant to me, but I don't know what the agreed-upon litmus test (if any) is. |
|
Does this affect LTS? |
|
@trevnorris nope, it just gets raw |
Neuter external `nullptr` buffers, otherwise their contents will be
materialized on access, and the buffer instance will be internalized.
This leads to a crash like this:
v8::ArrayBuffer::Neuter Only externalized ArrayBuffers can be
neutered
Fix: #3619
PR-URL: #3624
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This call was introduced in 827ee49 to avoid a crash in a later `Neuter()` call that has later been removed in ebbbc5a, rendering the original call unnecessary. Refs: nodejs#3624 Refs: nodejs#5204
This call was introduced in 827ee49 to avoid a crash in a later `Neuter()` call that has later been removed in ebbbc5a, rendering the original call unnecessary. Refs: nodejs/node#3624 Refs: nodejs/node#5204 PR-URL: nodejs/node#25479 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
8 participants
Neuter external
nullptrbuffers, otherwise their contents will be materialized on access, and the buffer instance will be internalized.This leads to a crash like this:
Fix: #3619
cc @Trott @trevnorris