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: cherry-pick 34880eb3dc from V8 upstream #8673

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
Affected core subsystem(s)

deps

Description of change

Regex on frozen objects is currently broken in our version of V8.
This patch reverts changes to regex while the V8 team figures out
what they are going to do for web compat.

Revert of Put RegExp js code in strict mode (patchset #2 id:20001
of https://codereview.chromium.org/1776883005/ )

Reason for revert:
Found to break SAP Web IDE, and these semantics are not shipped
in any other browser.
Revert to legacy semantics while assessing web compatibility.

BUG=chromium:624318

Original issue's description:
> Put RegExp js code in strict mode
>
> src/js/regexp.js was one of the few files that was left in sloppy
> mode. The ES2017 draft specification requires that writes to
> lastIndex throw when the property is non-writable, and test262
> tests enforce this behavior. This patch puts that file in strict
> mode.
>
> BUG=v8:4504
> R=yangguo@chromium.org
> LOG=Y
>
> Committed: https://crrev.com/80b1b2a45bbd9bf3d08e4e6516acfaaa8f438213
> Cr-Commit-Position: refs/heads/master@{#34801}

TBR=yangguo@chromium.org,adamk@chromium.org

Review-Url: https://codereview.chromium.org/2112713003
Cr-Commit-Position: refs/heads/master@{#37449}

@MylesBorins MylesBorins added v8 engine Issues and PRs related to the V8 dependency. v6.x labels Sep 20, 2016
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Sep 20, 2016
@MylesBorins
Copy link
Member Author

@MylesBorins
Copy link
Member Author

/cc @ofrobots and @littledan to make sure that this is the right code to be backporting

@addaleax
Copy link
Member

Since you labelled this v6.x, should this target v6.x-staging?

@MylesBorins
Copy link
Member Author

it is currently broken in master and needs to be backported to v6.x

On Tue, Sep 20, 2016, 6:29 PM Anna Henningsen notifications@github.com
wrote:

Since you labelled this v6.x, should this target v6.x-staging?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8673 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecVyIoOHJRMByHJO6LWWQ07ZEOl7XOks5qsF4rgaJpZM4KCMu-
.

@ofrobots
Copy link
Contributor

Link to chromium issue: http://crbug.com/624318. This was one of the fixes back-ported to V8 5.2 and 5.3 that is missing from V8 5.1 (and hence from Node v6.x and master).

This is indeed the correct change that you're back-porting. Thanks for putting this back-port together @thealphanerd.

The wording in the commit message suggest (to me at least) that this may not be the final form and may change in v6.x. I don't think that is the case; v6.x should behave compatibly with current browsers. In the future, through the spec, or otherwise, the browsers might decide to change their bahaviour and future versions of node will match that. v6.x will continue to behave the same.

@littledan
Copy link

LGTM. Not sure if this is helpful, but an alternative would be to backport the later fix towards full spec compliance, at https://codereview.chromium.org/2339443002

@MylesBorins
Copy link
Member Author

@littledan I think we should land this and likely look at that other commit once it has had a bit more time to bake in V8

@MylesBorins
Copy link
Member Author

It looks like the arm failures are infra related. @nodejs/platform-windows any insight into the windows failures?

@littledan
Copy link

@thealphanerd Sounds good to me. Bake time is a good idea, as the new semantics have never been deployed until now. Note that the later fix obviates this one, and we have reverted the patch that you're backporting in master.

@targos targos changed the base branch from master to v6.x-staging September 22, 2016 12:23
@targos
Copy link
Member

targos commented Sep 22, 2016

@thealphanerd I changed the base branch to v6.x-staging because this revert is already included in V8 5.4. Can you rebase ?

@MylesBorins
Copy link
Member Author

will do. thanks for being pro active!

On Thu, Sep 22, 2016, 8:25 AM Michaël Zasso notifications@github.com
wrote:

@thealphanerd https://github.com/TheAlphaNerd I changed the base branch
to v6.x-staging because this revert is already included in V8 5.4. Can
you rebase ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8673 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV6PIra0NZDkK4gz-k0mlBp5_dN7Gks5qsnPAgaJpZM4KCMu-
.

    Revert of Put RegExp js code in strict mode (patchset nodejs#2 id:20001
    of https://codereview.chromium.org/1776883005/ )

    Reason for revert:
    Found to break SAP Web IDE, and these semantics are not shipped
    in any other browser.
    Revert to legacy semantics while assessing web compatibility.

    BUG=chromium:624318

    Original issue's description:
    > Put RegExp js code in strict mode
    >
    > src/js/regexp.js was one of the few files that was left in sloppy
    > mode. The ES2017 draft specification requires that writes to
    > lastIndex throw when the property is non-writable, and test262
    > tests enforce this behavior. This patch puts that file in strict
    > mode.
    >
    > BUG=v8:4504
    > R=yangguo@chromium.org
    > LOG=Y
    >
    > Committed: https://crrev.com/80b1b2a45bbd9bf3d08e4e6516acfaaa8f438213
    > Cr-Commit-Position: refs/heads/master@{nodejs#34801}

    TBR=yangguo@chromium.org,adamk@chromium.org

    Review-Url: https://codereview.chromium.org/2112713003
    Cr-Commit-Position: refs/heads/master@{nodejs#37449}
@MylesBorins
Copy link
Member Author

@joaocgreis
Copy link
Member

joaocgreis commented Sep 22, 2016

@thealphanerd

With this we'll be able to see if the test is indeed failing or if it was some kind of once only, and we'll also know if it is already flaky in master. Note that the test that failed was with node compiled with VCBT2015 and this job is compilig with VS2015, because there is no easy way to do a stress test run with VCBT2015. I'll be happy if it doesn't fail with VS2015, but if there are concerns I can test it locally.

@fhinkel
Copy link
Member

fhinkel commented Sep 22, 2016

LGTM Thanks!

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Member Author

see as how windows was green on the last run I'm going to go ahead and land this on v6.x. Thanks for the review everyone

MylesBorins pushed a commit that referenced this pull request Sep 23, 2016
Original commit message:

    Revert of Put RegExp js code in strict mode (patchset #2 id:20001
    of https://codereview.chromium.org/1776883005/ )

    Reason for revert:
    Found to break SAP Web IDE, and these semantics are not shipped
    in any other browser.
    Revert to legacy semantics while assessing web compatibility.

    BUG=chromium:624318

    Original issue's description:
    > Put RegExp js code in strict mode
    >
    > src/js/regexp.js was one of the few files that was left in sloppy
    > mode. The ES2017 draft specification requires that writes to
    > lastIndex throw when the property is non-writable, and test262
    > tests enforce this behavior. This patch puts that file in strict
    > mode.
    >
    > BUG=v8:4504
    > R=yangguo@chromium.org
    > LOG=Y
    >
    > Committed: https://crrev.com/80b1b2a45bbd9bf3d08e4e6516acfaaa8f438213
    > Cr-Commit-Position: refs/heads/master@{#34801}

    TBR=yangguo@chromium.org,adamk@chromium.org

    Review-Url: https://codereview.chromium.org/2112713003
    Cr-Commit-Position: refs/heads/master@{#37449}

PR-URL: #8673
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins
Copy link
Member Author

landed in aafc314

MylesBorins pushed a commit that referenced this pull request Sep 26, 2016
Original commit message:

    Revert of Put RegExp js code in strict mode (patchset #2 id:20001
    of https://codereview.chromium.org/1776883005/ )

    Reason for revert:
    Found to break SAP Web IDE, and these semantics are not shipped
    in any other browser.
    Revert to legacy semantics while assessing web compatibility.

    BUG=chromium:624318

    Original issue's description:
    > Put RegExp js code in strict mode
    >
    > src/js/regexp.js was one of the few files that was left in sloppy
    > mode. The ES2017 draft specification requires that writes to
    > lastIndex throw when the property is non-writable, and test262
    > tests enforce this behavior. This patch puts that file in strict
    > mode.
    >
    > BUG=v8:4504
    > R=yangguo@chromium.org
    > LOG=Y
    >
    > Committed: https://crrev.com/80b1b2a45bbd9bf3d08e4e6516acfaaa8f438213
    > Cr-Commit-Position: refs/heads/master@{#34801}

    TBR=yangguo@chromium.org,adamk@chromium.org

    Review-Url: https://codereview.chromium.org/2112713003
    Cr-Commit-Position: refs/heads/master@{#37449}

PR-URL: #8673
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
evanlucas added a commit that referenced this pull request Sep 28, 2016
This is a security release. All Node.js users should consult the
security release summary at
https://nodejs.org/en/blog/vulnerability/september-2016-security-releases/
for details on patched vulnerabilities.

Notable Changes

Semver Minor:

* openssl:
  - Upgrade to 1.0.2i, fixes a number of defects impacting Node.js:
    CVE-2016-6304 ("OCSP Status Request extension unbounded memory
    growth", high severity), CVE-2016-2183, CVE-2016-2178, and CVE-2016-6306.
    (Shigeki Ohtsu) #8714
  - Upgrade to 1.0.2j, fixes a defect included in 1.0.2i resulting in
    a crash when using CRLs, CVE-2016-7052.
    (Shigeki Ohtsu) #8786
  - Remove support for loading dynamic third-party engine modules.
    An attacker may be able to hide malicious code to be inserted
    into Node.js at runtime by masquerading as one of the dynamic
    engine modules. Originally reported by Ahmed Zaki (Skype).
    (Ben Noordhuis) https://github.com/nodejs/node-private/pull/73
* http: CVE-2016-5325 - Properly validate for allowable characters in
  the `reason` argument in `ServerResponse#writeHead()`. Fixes a
  possible response splitting attack vector. This introduces a new
  case where `throw` may occur when configuring HTTP responses, users
  should already be adopting try/catch here. Originally reported
  independently by Evan Lucas and Romain Gaucher.
  (Evan Lucas) https://github.com/nodejs/node-private/pull/60

Semver Patch:

* buffer: Zero-fill excess bytes in new `Buffer` objects created with
  `Buffer.concat()` while providing a `totalLength` parameter that
  exceeds the total length of the original `Buffer` objects being
  concatenated.
  (Сковорода Никита Андреевич) https://github.com/nodejs/node-private/pull/64
* src: Fix regression where passing an empty password and/or salt to
  crypto.pbkdf2() would cause a fatal error
  (Rich Trott) #8572
* tls: CVE-2016-7099 - Fix invalid wildcard certificate validation
  check whereby a TLS server may be able to serve an invalid wildcard
  certificate for its hostname due to improper validation of `*.` in the
  wildcard string. Originally reported by Alexander Minozhenko and
  James Bunton (Atlassian).
  (Ben Noordhuis) https://github.com/nodejs/node-private/pull/75
* v8: Fix regression where a regex on a frozen object was broken
  (Myles Borins) #8673
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 28, 2016
    This is a security release. All Node.js users should consult the
    security release summary at
    https://nodejs.org/en/blog/vulnerability/september-2016-security-releases/
    for details on patched vulnerabilities.

    Notable Changes

    Semver Minor:

    * openssl:
      - Upgrade to 1.0.2i, fixes a number of defects impacting Node.js:
        CVE-2016-6304 ("OCSP Status Request extension unbounded memory
        growth", high severity), CVE-2016-2183, CVE-2016-2178, and CVE-2016-6306.
        (Shigeki Ohtsu) nodejs/node#8714
      - Upgrade to 1.0.2j, fixes a defect included in 1.0.2i resulting in
        a crash when using CRLs, CVE-2016-7052.
        (Shigeki Ohtsu) nodejs/node#8786
      - Remove support for loading dynamic third-party engine modules.
        An attacker may be able to hide malicious code to be inserted
        into Node.js at runtime by masquerading as one of the dynamic
        engine modules. Originally reported by Ahmed Zaki (Skype).
        (Ben Noordhuis) https://github.com/nodejs/node-private/pull/73
    * http: CVE-2016-5325 - Properly validate for allowable characters in
      the `reason` argument in `ServerResponse#writeHead()`. Fixes a
      possible response splitting attack vector. This introduces a new
      case where `throw` may occur when configuring HTTP responses, users
      should already be adopting try/catch here. Originally reported
      independently by Evan Lucas and Romain Gaucher.
      (Evan Lucas) https://github.com/nodejs/node-private/pull/60

    Semver Patch:

    * buffer: Zero-fill excess bytes in new `Buffer` objects created with
      `Buffer.concat()` while providing a `totalLength` parameter that
      exceeds the total length of the original `Buffer` objects being
      concatenated.
      https://github.com/nodejs/node-private/pull/64
    * src: Fix regression where passing an empty password and/or salt to
      crypto.pbkdf2() would cause a fatal error
      (Rich Trott) nodejs/node#8572
    * tls: CVE-2016-7099 - Fix invalid wildcard certificate validation
      check whereby a TLS server may be able to serve an invalid wildcard
      certificate for its hostname due to improper validation of `*.` in the
      wildcard string. Originally reported by Alexander Minozhenko and
      James Bunton (Atlassian).
      (Ben Noordhuis) https://github.com/nodejs/node-private/pull/75
    * v8: Fix regression where a regex on a frozen object was broken
      (Myles Borins) nodejs/node#8673

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 28, 2016
    This is a security release. All Node.js users should consult the
    security release summary at
    https://nodejs.org/en/blog/vulnerability/september-2016-security-releases/
    for details on patched vulnerabilities.

    Notable Changes

    Semver Minor:

    * openssl:
      - Upgrade to 1.0.2i, fixes a number of defects impacting Node.js:
        CVE-2016-6304 ("OCSP Status Request extension unbounded memory
        growth", high severity), CVE-2016-2183, CVE-2016-2178, and CVE-2016-6306.
        (Shigeki Ohtsu) nodejs/node#8714
      - Upgrade to 1.0.2j, fixes a defect included in 1.0.2i resulting in
        a crash when using CRLs, CVE-2016-7052.
        (Shigeki Ohtsu) nodejs/node#8786
      - Remove support for loading dynamic third-party engine modules.
        An attacker may be able to hide malicious code to be inserted
        into Node.js at runtime by masquerading as one of the dynamic
        engine modules. Originally reported by Ahmed Zaki (Skype).
        (Ben Noordhuis) https://github.com/nodejs/node-private/pull/73
    * http: CVE-2016-5325 - Properly validate for allowable characters in
      the `reason` argument in `ServerResponse#writeHead()`. Fixes a
      possible response splitting attack vector. This introduces a new
      case where `throw` may occur when configuring HTTP responses, users
      should already be adopting try/catch here. Originally reported
      independently by Evan Lucas and Romain Gaucher.
      (Evan Lucas) https://github.com/nodejs/node-private/pull/60

    Semver Patch:

    * buffer: Zero-fill excess bytes in new `Buffer` objects created with
      `Buffer.concat()` while providing a `totalLength` parameter that
      exceeds the total length of the original `Buffer` objects being
      concatenated.
      https://github.com/nodejs/node-private/pull/64
    * src: Fix regression where passing an empty password and/or salt to
      crypto.pbkdf2() would cause a fatal error
      (Rich Trott) nodejs/node#8572
    * tls: CVE-2016-7099 - Fix invalid wildcard certificate validation
      check whereby a TLS server may be able to serve an invalid wildcard
      certificate for its hostname due to improper validation of `*.` in the
      wildcard string. Originally reported by Alexander Minozhenko and
      James Bunton (Atlassian).
      (Ben Noordhuis) https://github.com/nodejs/node-private/pull/75
    * v8: Fix regression where a regex on a frozen object was broken
      (Myles Borins) nodejs/node#8673

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@richardlau
Copy link
Member

Reading nodejs/Release#137, shouldn't this change have also bumped the V8 patch level?

@MylesBorins
Copy link
Member Author

@richardlau it should have... and I could have sworn it did. Must have missed it. can send a PR bump it, is that the best course of action @nodejs/v8

@ofrobots
Copy link
Contributor

A follow on change to bump the V8 patch level would be fine. It is obviously better if the bump happened at the landing time, but doing it afterwards still has benefits (and no clear downsides that I see).

@MylesBorins MylesBorins mentioned this pull request Sep 29, 2016
2 tasks
@MylesBorins
Copy link
Member Author

Sent in a PR to bump to patch number #8851

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Oct 4, 2016
The patch should have been bumped in aafc314 but it was missed.

Ref: nodejs@aafc314
Ref: nodejs#8673

PR-URL: nodejs#8851
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell pushed a commit that referenced this pull request Oct 10, 2016
This is a security release. All Node.js users should consult the
security release summary at
https://nodejs.org/en/blog/vulnerability/september-2016-security-releases/
for details on patched vulnerabilities.

Notable Changes

Semver Minor:

* openssl:
  - Upgrade to 1.0.2i, fixes a number of defects impacting Node.js:
    CVE-2016-6304 ("OCSP Status Request extension unbounded memory
    growth", high severity), CVE-2016-2183, CVE-2016-2178, and CVE-2016-6306.
    (Shigeki Ohtsu) #8714
  - Upgrade to 1.0.2j, fixes a defect included in 1.0.2i resulting in
    a crash when using CRLs, CVE-2016-7052.
    (Shigeki Ohtsu) #8786
  - Remove support for loading dynamic third-party engine modules.
    An attacker may be able to hide malicious code to be inserted
    into Node.js at runtime by masquerading as one of the dynamic
    engine modules. Originally reported by Ahmed Zaki (Skype).
    (Ben Noordhuis) nodejs-private/node-private#73
* http: CVE-2016-5325 - Properly validate for allowable characters in
  the `reason` argument in `ServerResponse#writeHead()`. Fixes a
  possible response splitting attack vector. This introduces a new
  case where `throw` may occur when configuring HTTP responses, users
  should already be adopting try/catch here. Originally reported
  independently by Evan Lucas and Romain Gaucher.
  (Evan Lucas) nodejs-private/node-private#60

Semver Patch:

* buffer: Zero-fill excess bytes in new `Buffer` objects created with
  `Buffer.concat()` while providing a `totalLength` parameter that
  exceeds the total length of the original `Buffer` objects being
  concatenated.
  (Сковорода Никита Андреевич) nodejs-private/node-private#64
* src: Fix regression where passing an empty password and/or salt to
  crypto.pbkdf2() would cause a fatal error
  (Rich Trott) #8572
* tls: CVE-2016-7099 - Fix invalid wildcard certificate validation
  check whereby a TLS server may be able to serve an invalid wildcard
  certificate for its hostname due to improper validation of `*.` in the
  wildcard string. Originally reported by Alexander Minozhenko and
  James Bunton (Atlassian).
  (Ben Noordhuis) nodejs-private/node-private#75
* v8: Fix regression where a regex on a frozen object was broken
  (Myles Borins) #8673
@MylesBorins MylesBorins deleted the no-strict-regex branch November 14, 2017 17:43
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.

None yet

9 participants