FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: neuter external `nullptr` buffers by indutny · Pull Request #3624 · nodejs/node · GitHub

/ node Public

buffer: neuter external nullptr buffers - #3624

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-3619
ConversationCommits (1)ChecksFiles changed
Closed

buffer: neuter external nullptr buffers#3624
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-3619

Conversation

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member

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

cc @Trott @trevnorris

Trott commented Nov 2, 2015

Copy link
Copy Markdown
Member

Fixes the issue for me. 👍

Trott mentioned this pull request Nov 2, 2015
Closed

Trott commented Nov 2, 2015

Copy link
Copy Markdown
Member

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/

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Nov 2, 2015

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

CI looks green, thank you @Trott

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

cc @nodejs/collaborators

I would like to get LGTM from either @trevnorris or @bnoordhuis before landing it.

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

btw, other reviews are more than welcome!

trevnorris commented Nov 2, 2015

Copy link
Copy Markdown
Contributor

LGTM

Would you consider this a v8 bug?

bnoordhuis reviewed Nov 2, 2015
View reviewed changes
Comment thread src/node_buffer.cc Outdated

bnoordhuis Nov 2, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a comment explaining why the Neuter() call is necessary?

indutny Nov 2, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep, will do.

bnoordhuis commented Nov 2, 2015

Copy link
Copy Markdown
Member

LGTM with comments.

evanlucas reviewed Nov 2, 2015
View reviewed changes
Comment thread test/addons/null-buffer-neuter/test.js

evanlucas Nov 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this flag necessary here?

bnoordhuis Nov 2, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. Without it, v8::Isolate::RequestGarbageCollectionForTesting() aborts with a run-time error.

evanlucas Nov 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah ok, didn't realize that

buffer: neuter external nullptr buffers
0eeb375 
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

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

@trevnorris I'm not sure, maybe a documentation bug. Filed an issue: https://code.google.com/p/v8/issues/detail?id=4530

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

@nodejs/release how do I mark it to be landed on v5?

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

Landed in master in 827ee49, thank you everyone!

indutny closed this Nov 2, 2015
indutny deleted the fix/gh-3619 branch November 2, 2015 13:38
indutny added a commit that referenced this pull request Nov 2, 2015
buffer: neuter external nullptr buffers
827ee49 
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>

Trott commented Nov 2, 2015

Copy link
Copy Markdown
Member

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.

trevnorris commented Nov 2, 2015

Copy link
Copy Markdown
Contributor

Does this affect LTS?

indutny commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

@trevnorris nope, it just gets raw backing_store from the ArrayBuffer without materializing it. LTS is safe.

Fishrock123 mentioned this pull request Nov 6, 2015
Closed
indutny added a commit that referenced this pull request Nov 7, 2015
buffer: neuter external nullptr buffers
5667369 
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>
Fishrock123 mentioned this pull request Nov 11, 2015
Merged
rvagg mentioned this pull request Dec 17, 2015
Closed
MylesBorins mentioned this pull request Jan 14, 2016
Closed
addaleax added a commit to addaleax/node that referenced this pull request Jan 13, 2019
src: remove outdated Neuter() call in node_buffer.cc
98da4cc 
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
addaleax mentioned this pull request Jan 13, 2019
Closed
2 tasks
addaleax added a commit that referenced this pull request Jan 21, 2019
src: remove outdated Neuter() call in node_buffer.cc
30f4568 
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: #3624
Refs: #5204

PR-URL: #25479
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos pushed a commit that referenced this pull request Jan 24, 2019
src: remove outdated Neuter() call in node_buffer.cc
dbecc82 
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: #3624
Refs: #5204

PR-URL: #25479
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
src: remove outdated Neuter() call in node_buffer.cc
1d2d3f3 
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: #3624
Refs: #5204

PR-URL: #25479
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BethGriggs pushed a commit that referenced this pull request May 10, 2019
src: remove outdated Neuter() call in node_buffer.cc
f71c55b 
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: #3624
Refs: #5204

PR-URL: #25479
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
src: remove outdated Neuter() call in node_buffer.cc
cd4a932 
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: #3624
Refs: #5204

PR-URL: #25479
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
src: remove outdated Neuter() call in node_buffer.cc
e13e8fe 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL