Propagate exceptions thrown by access check interceptors.

Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org

Committed: https://crrev.com/ebe94192284e5f4437929a3256749fef7cb53329
Cr-Commit-Position: refs/heads/master@{#41556}

[dcheng, 2016-12-06 09:55:15]

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

[dcheng, 2016-12-06 09:56:29]

Description was changed from

==========
Allow cross-origin property setter interceptors to throw exceptions.

SetPropertyWithInterceptorInternal() promotes a scheduled exception to a
pending exception with the RETURN_VALUE_IF_SCHEDULED_EXCEPTION helper.
However, SetPropertyWithFailedAccessCheck() also tries to check for a
scheduled exception; otherwise, if the result is nothing, it dispatches
a failed access check callback.

To allow interceptors to throw exceptions without triggering the failed
access check callback, SetPropertyWithFailedAccessCheck() now just
directly returns the result. This mirrors the normal property access
path via SetPropertyWithAccessor().

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

to

==========
Allow cross-origin property setter interceptors to throw exceptions.

SetPropertyWithInterceptorInternal() promotes a scheduled exception to a
pending exception with the RETURN_VALUE_IF_SCHEDULED_EXCEPTION helper.
However, its caller SetPropertyWithFailedAccessCheck() also tries to
check for a scheduled exception. If there is no scheduled exception and
the result is nothing, it dispatches a failed access check callback.

To allow interceptors to throw exceptions without triggering the failed
access check callback, SetPropertyWithFailedAccessCheck() now just
directly returns the result. This mirrors the normal property access
path via SetPropertyWithAccessor().

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

[dcheng, 2016-12-06 09:56:37]

Does this make sense?

[dcheng, 2016-12-06 09:59:34]

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

[commit-bot: I haz the power, 2016-12-06 09:59:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2016-12-06 10:05:22]

do we need a similar change for the getter case?

https://codereview.chromium.org/2550423002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2550423002/diff/1/src/objects.cc#newcode1892
src/objects.cc:1892: isolate->ReportFailedAccessCheck(checked);
maybe move these lines into the if () { } block, so it doesn't look like there
was a way to reach them from the else {} block?

[commit-bot: I haz the power, 2016-12-06 10:24:45]

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

[commit-bot: I haz the power, 2016-12-06 10:24:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-06 10:29:33]

On 2016/12/06 10:05:22, jochen wrote:
> do we need a similar change for the getter case?

Yes, we do. I haven't quite figured out the proper tweaks needed in that logic
yet: the getter version has an extra bool* that's set to true iff there's no
exception and the getter didn't return null. I'm not yet sure if it's OK to just
ignore this bool completely in the failed access check path.

(I need to rest for now, I'll revisit this tomorrow unless someone else wants to
adopt the patch)

> 
> https://codereview.chromium.org/2550423002/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/2550423002/diff/1/src/objects.cc#newcode1892
> src/objects.cc:1892: isolate->ReportFailedAccessCheck(checked);
> maybe move these lines into the if () { } block, so it doesn't look like there
> was a way to reach them from the else {} block?

[jochen (gone - plz use gerrit), 2016-12-06 10:31:54]

ok, lgtm if you address my comment. Fixing the getter separately is fine

[dcheng, 2016-12-06 10:44:13]

On 2016/12/06 10:29:33, dcheng wrote:
> On 2016/12/06 10:05:22, jochen wrote:
> > do we need a similar change for the getter case?
> 
> Yes, we do. I haven't quite figured out the proper tweaks needed in that logic
> yet: the getter version has an extra bool* that's set to true iff there's no
> exception and the getter didn't return null. I'm not yet sure if it's OK to
just
> ignore this bool completely in the failed access check path.
> 
> (I need to rest for now, I'll revisit this tomorrow unless someone else wants
to
> adopt the patch)

I need to think more about how this should work.

Right now, I think the code is structured this way so that if an intercept is
omitted, we get the 'failed access check' callback like before. With this
change, I think that might no longer be true: the setter path will now *never*
call the interceptor checks.

So an alternative is to tweak the internal API a bit further:
- SetPropertyWithInternalInterceptor should just return false if no interceptor
is set. In that case, we'll fall back to the failed access check callback.
- Otherwise, it should return true, even if there are exceptions thrown by the
interceptor: it's already been handled, and calling the failed access check
callback just isn't safe.

For the getter version:
- GetPropertyWithInternalInterceptor uses |*done| to signal whether not a getter
is set.
- If no getter is set, we fall back to the failed access check callback.
- Otherwise, if we have a getter, we mark *done as true and never fall back.

That being said, I'm not sure if this API change is safe for getters... it looks
like we might rely on the current behavior to fallback between getters if a
previous getter doesn't return anything...

> 
> > 
> > https://codereview.chromium.org/2550423002/diff/1/src/objects.cc
> > File src/objects.cc (right):
> > 
> > https://codereview.chromium.org/2550423002/diff/1/src/objects.cc#newcode1892
> > src/objects.cc:1892: isolate->ReportFailedAccessCheck(checked);
> > maybe move these lines into the if () { } block, so it doesn't look like
there
> > was a way to reach them from the else {} block?

[dcheng, 2016-12-06 10:46:16]

On 2016/12/06 10:44:13, dcheng wrote:
> On 2016/12/06 10:29:33, dcheng wrote:
> > On 2016/12/06 10:05:22, jochen wrote:
> > > do we need a similar change for the getter case?
> > 
> > Yes, we do. I haven't quite figured out the proper tweaks needed in that
logic
> > yet: the getter version has an extra bool* that's set to true iff there's no
> > exception and the getter didn't return null. I'm not yet sure if it's OK to
> just
> > ignore this bool completely in the failed access check path.
> > 
> > (I need to rest for now, I'll revisit this tomorrow unless someone else
wants
> to
> > adopt the patch)
> 
> I need to think more about how this should work.
> 
> Right now, I think the code is structured this way so that if an intercept is
> omitted, we get the 'failed access check' callback like before. With this
> change, I think that might no longer be true: the setter path will now *never*
> call the interceptor checks.

Argh: I mean that the setter path will now never call the *failed access check
callback* if there's an interceptor.
While this probably doesn't matter as much for setters, I think it does matter
for getters, for the reasons listed below.

> 
> So an alternative is to tweak the internal API a bit further:
> - SetPropertyWithInternalInterceptor should just return false if no
interceptor
> is set. In that case, we'll fall back to the failed access check callback.
> - Otherwise, it should return true, even if there are exceptions thrown by the
> interceptor: it's already been handled, and calling the failed access check
> callback just isn't safe.
> 
> For the getter version:
> - GetPropertyWithInternalInterceptor uses |*done| to signal whether not a
getter
> is set.
> - If no getter is set, we fall back to the failed access check callback.
> - Otherwise, if we have a getter, we mark *done as true and never fall back.
> 
> That being said, I'm not sure if this API change is safe for getters... it
looks
> like we might rely on the current behavior to fallback between getters if a
> previous getter doesn't return anything...
> 
> > 
> > > 
> > > https://codereview.chromium.org/2550423002/diff/1/src/objects.cc
> > > File src/objects.cc (right):
> > > 
> > >
https://codereview.chromium.org/2550423002/diff/1/src/objects.cc#newcode1892
> > > src/objects.cc:1892: isolate->ReportFailedAccessCheck(checked);
> > > maybe move these lines into the if () { } block, so it doesn't look like
> there
> > > was a way to reach them from the else {} block?

[dcheng, 2016-12-06 10:59:23]

On 2016/12/06 10:46:16, dcheng wrote:
> On 2016/12/06 10:44:13, dcheng wrote:
> > On 2016/12/06 10:29:33, dcheng wrote:
> > > On 2016/12/06 10:05:22, jochen wrote:
> > > > do we need a similar change for the getter case?
> > > 
> > > Yes, we do. I haven't quite figured out the proper tweaks needed in that
> logic
> > > yet: the getter version has an extra bool* that's set to true iff there's
no
> > > exception and the getter didn't return null. I'm not yet sure if it's OK
to
> > just
> > > ignore this bool completely in the failed access check path.
> > > 
> > > (I need to rest for now, I'll revisit this tomorrow unless someone else
> wants
> > to
> > > adopt the patch)
> > 
> > I need to think more about how this should work.
> > 
> > Right now, I think the code is structured this way so that if an intercept
is
> > omitted, we get the 'failed access check' callback like before. With this
> > change, I think that might no longer be true: the setter path will now
*never*
> > call the interceptor checks.
> 
> Argh: I mean that the setter path will now never call the *failed access check
> callback* if there's an interceptor.
> While this probably doesn't matter as much for setters, I think it does matter
> for getters, for the reasons listed below.
> 
> > 
> > So an alternative is to tweak the internal API a bit further:
> > - SetPropertyWithInternalInterceptor should just return false if no
> interceptor
> > is set. In that case, we'll fall back to the failed access check callback.
> > - Otherwise, it should return true, even if there are exceptions thrown by
the
> > interceptor: it's already been handled, and calling the failed access check
> > callback just isn't safe.
> > 
> > For the getter version:
> > - GetPropertyWithInternalInterceptor uses |*done| to signal whether not a
> getter
> > is set.
> > - If no getter is set, we fall back to the failed access check callback.
> > - Otherwise, if we have a getter, we mark *done as true and never fall back.
> > 
> > That being said, I'm not sure if this API change is safe for getters... it
> looks
> > like we might rely on the current behavior to fallback between getters if a
> > previous getter doesn't return anything...
> > 
> > > 
> > > > 
> > > > https://codereview.chromium.org/2550423002/diff/1/src/objects.cc
> > > > File src/objects.cc (right):
> > > > 
> > > >
> https://codereview.chromium.org/2550423002/diff/1/src/objects.cc#newcode1892
> > > > src/objects.cc:1892: isolate->ReportFailedAccessCheck(checked);
> > > > maybe move these lines into the if () { } block, so it doesn't look like
> > there
> > > > was a way to reach them from the else {} block?

OK, I'll upload a patch tomorrow: I think it makes sense that if you set an
access check interceptor, you're responsible for throwing all the exceptions. If
you don't, then you'll still get the old behavior.

[dcheng, 2016-12-07 06:14:34]

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

[commit-bot: I haz the power, 2016-12-07 06:14:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-12-07 06:29:58]

Description was changed from

==========
Allow cross-origin property setter interceptors to throw exceptions.

SetPropertyWithInterceptorInternal() promotes a scheduled exception to a
pending exception with the RETURN_VALUE_IF_SCHEDULED_EXCEPTION helper.
However, its caller SetPropertyWithFailedAccessCheck() also tries to
check for a scheduled exception. If there is no scheduled exception and
the result is nothing, it dispatches a failed access check callback.

To allow interceptors to throw exceptions without triggering the failed
access check callback, SetPropertyWithFailedAccessCheck() now just
directly returns the result. This mirrors the normal property access
path via SetPropertyWithAccessor().

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

to

==========
Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

[dcheng, 2016-12-07 06:31:11]

I did a bit more reading, and I think this is a more correct fix now: it
propagates the exceptions upwards correctly now.

https://codereview.chromium.org/2550423002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2550423002/diff/20001/src/objects.cc#newcode1850
src/objects.cc:1850: if (isolate->has_scheduled_exception()) break;
Note: I'm not convinced this is correct, but I haven't managed to hit cases that
break this either. So I'm just going to leave it alone…

[commit-bot: I haz the power, 2016-12-07 07:01:32]

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

[commit-bot: I haz the power, 2016-12-07 07:01:36]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-07 15:37:13]

still lgtm

[dcheng, 2016-12-07 15:52:18]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-12-07 15:52:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 15:54:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481125938824530,
"parent_rev": "75f2f17b80ac03407cd6711dbec9101b8aa830ca", "commit_rev":
"9c9186a64c33b86929052f9d28e25c03872c1c0d"}

[commit-bot: I haz the power, 2016-12-07 15:54:09]

Description was changed from

==========
Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

to

==========
Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

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

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-07 15:54:50]

Description was changed from

==========
Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org
==========

to

==========
Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org

Committed: https://crrev.com/ebe94192284e5f4437929a3256749fef7cb53329
Cr-Commit-Position: refs/heads/master@{#41556}
==========

[commit-bot: I haz the power, 2016-12-07 15:54:51]

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