[proxies] fix access issue when having proxies on the prototype-chain of global objects.

[proxies] fix access issue when having proxies on the prototype-chain of global objects.

We can no longer just walk the prototype chain without doing proper access-checks. When installing a proxy as the __proto__ of the global object we might accidentally end up invoking cross-realm code without access-checks (see proxies-cross-realm-ecxeption.js).

Committed: https://crrev.com/2c75e3d2abc3eba3337a09afcd0dfa3e653ff006
Cr-Commit-Position: refs/heads/master@{#32903}

[Camillo Bruni, 2015-12-14 08:47:17]

Description was changed from

==========
Merge branch 'master' into 2015-12-11_JSProxy_getPrototypeOf_Error


formatting


formatting


more


doing stuff


enabling tests again


need the one constant for some reason... going to investigate later


formatting


updating platforms


Merge branch 'test' into 2015-12-10_JSProxy_more_tests_1516843002


[WIP] Proxy fix for for-in.

R=cbruni@chromium.org
BUG=

patch from issue 1512143005 at patchset 1 (http://crrev.com/1512143005#ps1)

enable enumerate test


fixing last test


formatting


fixing tests


fixing with tests since they now trigger the [[Has]] trap


fixing typo


formatting


formatting


fixing for-in + tests


copying for-in-opt test again


Merge branch 'master' into 2015-12-10_JSProxy_more_tests_1516843002


enabling tests


formatting


formatting


adding more tests


fixing proxy tests


removing dcheck


more test refactoring


formatting


improving error messages and proxy tests


Adapt/delete/reenable proxy tests.  WIP, not to be landed as is.

R=cbruni@chromium.org
BUG=

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)

updating tests
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects
==========

[Camillo Bruni, 2015-12-14 08:52:35]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

[Camillo Bruni, 2015-12-14 17:09:07]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-14 17:09:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/100001

[commit-bot: I haz the power, 2015-12-14 17:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-14 17:10:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/13185)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11473)
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/12076)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/13058)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/13180)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/8942)

[Camillo Bruni, 2015-12-14 17:17:46]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-14 17:17:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/140001

[commit-bot: I haz the power, 2015-12-14 17:56:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-14 17:56:24]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/12078)

[Camillo Bruni, 2015-12-14 19:01:42]

cbruni@chromium.org changed reviewers:
+ bmeurer@chromium.org, jkummerow@chromium.org
- cbruni@chromium.org

[Camillo Bruni, 2015-12-14 19:01:43]

PTAL,
bmeurer: mostly check the native code pieces :)

mips/pcc team: could you check the CL and port it if necessary

[Benedikt Meurer, 2015-12-14 19:13:09]

native bits LGTM. But don't we need to do the same in Crankshaft?

[Toon Verwaest, 2015-12-14 19:53:05]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2015-12-14 19:53:05]

I'd prefer having the Advance methods automatically enforce HasAccess, reaching
"null" if access isn't given. Otherwise users are going to forget.

There could be a single method that skips the access check if really necessary.

[Camillo Bruni, 2015-12-14 19:57:11]

On 2015/12/14 at 19:53:05, verwaest wrote:
> I'd prefer having the Advance methods automatically enforce HasAccess,
reaching "null" if access isn't given. Otherwise users are going to forget.
> 
> There could be a single method that skips the access check if really
necessary.

Right, so we would do that only for AdvanceFollowingProxies since the access
check won't work with non-handlified code.
In the other case we will defer the Proxy handling to the client (which is
probably a bad idea...).

[Camillo Bruni, 2015-12-14 19:58:57]

On 2015/12/14 at 19:13:09, bmeurer wrote:
> native bits LGTM. But don't we need to do the same in Crankshaft?

From what I've seen we have a deopt point in
LCodeGen::DoHasInPrototypeChainAndBranch when we find a Proxy.
Since we restart the lookup in the code stub I presume that we're fine, or did I
miss something?

[Camillo Bruni, 2015-12-14 19:59:21]

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
src/crankshaft/ia32/lithium-codegen-ia32.cc:2547: // without performing the
necessary access checks first.
probably should add this comment to all platforms.

[Toon Verwaest, 2015-12-14 20:06:48]

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
src/crankshaft/ia32/lithium-codegen-ia32.cc:2547: // without performing the
necessary access checks first.
On 2015/12/14 19:59:20, cbruni wrote:
> probably should add this comment to all platforms.

There's still a silly issue. If you have:
window2.__proto__ = window1
then in window1: window2 instanceof Object -> true

That's an information leak. It's not high-priority, but it's wrong nonetheless.

[Camillo Bruni, 2015-12-15 11:38:45]

On 2015/12/14 at 20:06:48, verwaest wrote:
>
https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
> File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
> src/crankshaft/ia32/lithium-codegen-ia32.cc:2547: // without performing the
necessary access checks first.
> On 2015/12/14 19:59:20, cbruni wrote:
> > probably should add this comment to all platforms.
> 
> There's still a silly issue. If you have:
> window2.__proto__ = window1
> then in window1: window2 instanceof Object -> true
> 
> That's an information leak. It's not high-priority, but it's wrong
nonetheless.

I fixed that now as well (turbofan still doesn't work properly though)

[Camillo Bruni, 2015-12-15 11:38:48]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-15 11:38:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/220001

[commit-bot: I haz the power, 2015-12-15 11:41:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 11:41:46]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Camillo Bruni, 2015-12-15 11:56:01]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-15 11:56:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/240001

[commit-bot: I haz the power, 2015-12-15 12:10:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 12:10:54]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Camillo Bruni, 2015-12-15 13:13:06]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-15 13:13:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/260001

[commit-bot: I haz the power, 2015-12-15 13:32:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 13:32:12]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-12-15 13:33:57]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=chr:399951
LOG=y
==========

[Camillo Bruni, 2015-12-15 13:34:08]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=chr:399951
LOG=y
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=cr:399951
LOG=y
==========

[Camillo Bruni, 2015-12-15 13:34:20]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=cr:399951
LOG=y
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=chromium:399951
LOG=y
==========

[Camillo Bruni, 2015-12-15 13:37:19]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

BUG=chromium:399951
LOG=y
==========

to

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

[Camillo Bruni, 2015-12-15 14:09:27]

PTAL again:
- included access check in AdvanceFollowingProxies (in the other case we behave
as is, otherwise we need to handlify all callers).
- updated turbofan JSTypedLowering::ReduceJSInstanceOf with a lot of help from
mstarzinger to perform the same checks as CodeStub (bmeureer, please have a look
at it)

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1521953002/diff/180001/src/crankshaft/ia32/li...
src/crankshaft/ia32/lithium-codegen-ia32.cc:2547: // without performing the
necessary access checks first.
On 2015/12/14 at 20:06:48, Toon Verwaest wrote:
> On 2015/12/14 19:59:20, cbruni wrote:
> > probably should add this comment to all platforms.
> 
> There's still a silly issue. If you have:
> window2.__proto__ = window1
> then in window1: window2 instanceof Object -> true
> 
> That's an information leak. It's not high-priority, but it's wrong
nonetheless.

fixed that now as well...

[Camillo Bruni, 2015-12-15 14:16:35]

Description was changed from

==========
[runtime] [proxy] fix access issue when having proxies on the prototype-chain of
global objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

to

==========
[proxies] fix access issue when having proxies on the prototype-chain of global
objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

[paul.l..., 2015-12-16 02:14:33]

On 2015/12/14 19:01:43, cbruni wrote:
> mips/pcc team: could you check the CL and port it if necessary

 MIPS ports look good, thanks!

[Toon Verwaest, 2015-12-16 13:31:17]

lgtm

[Camillo Bruni, 2015-12-16 13:40:50]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-12-16 13:40:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1521953002/#ps320001
(title: "ppc code mess")

[commit-bot: I haz the power, 2015-12-16 13:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521953002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521953002/320001

[commit-bot: I haz the power, 2015-12-16 14:31:27]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-12-16 14:31:50]

Description was changed from

==========
[proxies] fix access issue when having proxies on the prototype-chain of global
objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).
==========

to

==========
[proxies] fix access issue when having proxies on the prototype-chain of global
objects.

We can no longer just walk the prototype chain without doing proper
access-checks. When installing a proxy as the __proto__ of the global object we
might accidentally end up invoking cross-realm code without access-checks (see
proxies-cross-realm-ecxeption.js).

Committed: https://crrev.com/2c75e3d2abc3eba3337a09afcd0dfa3e653ff006
Cr-Commit-Position: refs/heads/master@{#32903}
==========

[commit-bot: I haz the power, 2015-12-16 14:31:51]

Patchset 17 (id:??) landed as
https://crrev.com/2c75e3d2abc3eba3337a09afcd0dfa3e653ff006
Cr-Commit-Position: refs/heads/master@{#32903}

[Lei Zhang, 2015-12-23 00:25:16]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-12-23 00:25:17]

https://codereview.chromium.org/1521953002/diff/320001/src/prototype.h
File src/prototype.h (right):

https://codereview.chromium.org/1521953002/diff/320001/src/prototype.h#newcode66
src/prototype.h:66: const bool HasAccess() {
BTW, using V8 with PDFium, I get this warning:

../../v8/src/prototype.h:70:3: warning: 'const' type qualifier on return type
has no effect [-Wignored-qualifiers]
  const bool HasAccess() {
  ^~~~~~

[Jakob Kummerow, 2016-01-04 15:55:29]

On 2015/12/23 00:25:17, Lei Zhang wrote:
> https://codereview.chromium.org/1521953002/diff/320001/src/prototype.h
> File src/prototype.h (right):
> 
>
https://codereview.chromium.org/1521953002/diff/320001/src/prototype.h#newcode66
> src/prototype.h:66: const bool HasAccess() {
> BTW, using V8 with PDFium, I get this warning:
> 
> ../../v8/src/prototype.h:70:3: warning: 'const' type qualifier on return type
> has no effect [-Wignored-qualifiers]
>   const bool HasAccess() {
>   ^~~~~~

Thanks, here's a fix: https://codereview.chromium.org/1559923002/