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

deps: backport IsValid changes from 4e8736d in V8 #6544

Merged
merged 1 commit into from
May 4, 2016

Conversation

targos
Copy link
Member

@targos targos commented May 3, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Description of change

V8 erroneously did null pointer checks on this.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from 1 that fix this issue.

Fixes: #6272

cc @nodejs/v8

@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label May 3, 2016
@bnoordhuis
Copy link
Member

LGTM

@targos
Copy link
Member Author

targos commented May 3, 2016

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/2475/

Do we have a CI job to run V8 tests ?

@jeisinger
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell commented May 3, 2016

@targos ... yes... https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/
But it is limited a bit still

@targos
Copy link
Member Author

targos commented May 3, 2016

@indutny
Copy link
Member

indutny commented May 3, 2016

Rubber-stamp LGTM

@targos
Copy link
Member Author

targos commented May 4, 2016

Labelling lts-watch-v4.x. I have the backport ready in my fork if necessary. There are other occurrences of this != NULL in the V8 version we have in v4.x, though.

V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@targos targos merged commit 96198d5 into nodejs:master May 4, 2016
@targos targos deleted the fix2-6272 branch May 4, 2016 08:23
@bnoordhuis
Copy link
Member

There are other occurrences of this != NULL in the V8 version we have in v4.x

Interesting. The compiler is only allowed to elide the first check and I don't think the initial FrameStateDescriptor can be NULL, going by the code in deps/v8/src/compiler.

@targos
Copy link
Member Author

targos commented May 4, 2016

I don't think the initial FrameStateDescriptor can be NULL, going by the code in deps/v8/src/compiler.

It apparently can. At least one of our tests crashes at FrameStateDescriptor::GetTotalSize()

@bnoordhuis
Copy link
Member

Oh, okay. I'll review your changes if you're willing to work on it.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 4, 2016
V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
targos added a commit to targos/node that referenced this pull request Jun 3, 2016
V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
targos added a commit to targos/node that referenced this pull request Jun 29, 2016
V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
V8 erroneously did null pointer checks on `this`.
It can lead to a SIGSEGV crash if node is compiled with GCC 6.
Backport relevant changes from [1] that fix this issue.

[1]: https://codereview.chromium.org/1900423002

Fixes: nodejs#6272
PR-URL: nodejs#6544
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v6 has buffer issues on Fedora 24
6 participants