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 0ba513f05 from V8 upstream #11712

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@fhinkel
Member

fhinkel commented Mar 6, 2017

Original commit message:

[api] Fix DescriptorInterceptor with access check.

The DescriptorInterceptor should intercept all
Object.getOwnPropertyDescriptor calls. This CL fixes
the interceptor's behavior if the iterator state is
ACCESS_CHECK.

BUG=

Review-Url: https://codereview.chromium.org/2707263002
Cr-Commit-Position: refs/heads/master@{#43417}
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
Affected core subsystem(s)

deps V8

This fix is needed to use the GenericNamedPropertyDescriptorCallback on the global object in node_contextify.cc.

5.6 is not maintained anymore, therefore not backmerged upstream.

cc/ @nodejs/v8

deps: cherry-pick 0ba513f05 from V8 upstream
Original commit message:
  [api] Fix DescriptorInterceptor with access check.

  The DescriptorInterceptor should intercept all
  Object.getOwnPropertyDescriptor calls. This CL fixes
  the interceptor's behavior if the iterator state is
  ACCESS_CHECK.

  BUG=

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

This comment has been minimized.

Show comment
Hide comment
bool has_access = true;
if (it->state() == LookupIterator::ACCESS_CHECK) {
has_access = it->HasAccess() || JSObject::AllCanRead(it);
it->Next();

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Is this sound? I see LookupIterator::Next() has a DCHECK_NE(JSPROXY, state_) but if I read JSObject::AllCanRead() right, it can stop at a JSPROXY instance. It seems like the logic should be:

if (has_access) it->Next();
@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Is this sound? I see LookupIterator::Next() has a DCHECK_NE(JSPROXY, state_) but if I read JSObject::AllCanRead() right, it can stop at a JSPROXY instance. It seems like the logic should be:

if (has_access) it->Next();

This comment has been minimized.

@fhinkel

fhinkel Mar 6, 2017

Member

The access check is on the object_template, which is created by the embedder. We cannot configure templates as JSProxies. I'd say it's sound.

@fhinkel

fhinkel Mar 6, 2017

Member

The access check is on the object_template, which is created by the embedder. We cannot configure templates as JSProxies. I'd say it's sound.

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Okay, if you say so.

@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Okay, if you say so.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
Member

fhinkel commented Mar 6, 2017

bool has_access = true;
if (it->state() == LookupIterator::ACCESS_CHECK) {
has_access = it->HasAccess() || JSObject::AllCanRead(it);
it->Next();

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Okay, if you say so.

@bnoordhuis

bnoordhuis Mar 6, 2017

Member

Okay, if you say so.

@MylesBorins

LGTM

@jasnell

jasnell approved these changes Mar 8, 2017

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Mar 9, 2017

Member

Landed in a44aff4.

Member

fhinkel commented Mar 9, 2017

Landed in a44aff4.

@fhinkel fhinkel closed this Mar 9, 2017

fhinkel added a commit that referenced this pull request Mar 9, 2017

deps: cherry-pick 0ba513f05 from V8 upstream
Original commit message:
  [api] Fix DescriptorInterceptor with access check.

  The DescriptorInterceptor should intercept all
  Object.getOwnPropertyDescriptor calls. This CL fixes
  the interceptor's behavior if the iterator state is
  ACCESS_CHECK.

  BUG=

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

PR-URL: #11712
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 9, 2017

Member

@fhinkel Is this already in 5.7?

Member

targos commented Mar 9, 2017

@fhinkel Is this already in 5.7?

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Mar 10, 2017

Member

It's only in 5.8. Do you want to open the merge request or should I?

Member

fhinkel commented Mar 10, 2017

It's only in 5.8. Do you want to open the merge request or should I?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 10, 2017

Member

Feel free to do it.

Member

targos commented Mar 10, 2017

Feel free to do it.

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

deps: cherry-pick 0ba513f05 from V8 upstream
Original commit message:
  [api] Fix DescriptorInterceptor with access check.

  The DescriptorInterceptor should intercept all
  Object.getOwnPropertyDescriptor calls. This CL fixes
  the interceptor's behavior if the iterator state is
  ACCESS_CHECK.

  BUG=

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

PR-URL: nodejs#11712
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit to targos/node that referenced this pull request Mar 23, 2017

deps: cherry-pick 0ba513f05 from V8 upstream
Original commit message:
  [api] Fix DescriptorInterceptor with access check.

  The DescriptorInterceptor should intercept all
  Object.getOwnPropertyDescriptor calls. This CL fixes
  the interceptor's behavior if the iterator state is
  ACCESS_CHECK.

  BUG=

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

PR-URL: nodejs#11712
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@targos targos referenced this pull request Mar 23, 2017

Merged

deps: update V8 to 5.7 #11752

2 of 2 tasks complete

targos added a commit to targos/node that referenced this pull request Mar 25, 2017

deps: cherry-pick 0ba513f05 from V8 upstream
Original commit message:
  [api] Fix DescriptorInterceptor with access check.

  The DescriptorInterceptor should intercept all
  Object.getOwnPropertyDescriptor calls. This CL fixes
  the interceptor's behavior if the iterator state is
  ACCESS_CHECK.

  BUG=

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

PR-URL: nodejs#11712
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment