850000 - Don't clobber exceptions set in security wrapper check() hook

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

In general, the filtering policy implementations don't throw, and rely on AutoEnterPolicy to make that decision. However, there are certain cases, such as a malformed __exposedProps__, where we do throw. If that happens, we shouldn't clobber the exception, even if *bp happens to end up set to false.

This came up while trying to land the tests from bug 813901.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Don't clobber exceptions set in security wrapper check() hooks. v1

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 723654 [details] [diff] [review]
Don't clobber exceptions set in security wrapper check() hooks. v1

Review of attachment 723654 [details] [diff] [review]:
-----------------------------------------------------------------

I'm generally OK with this, but want an answer to my question before stamping.

::: js/src/jsproxy.h
@@ +368,5 @@
> +        // * The policy disallowed access.
> +        // * The policy set rv to false, indicating that we should throw.
> +        // * The caller did not instruct us to ignore exceptions.
> +        // * The policy did not throw itself.
> +        if (!allow && !rv && mayThrow && !cx->isExceptionPending())

isExceptionPending checks scare me because they often get confused by OOM. Do our policies deal properly with that?

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 723654 [details] [diff] [review]
Don't clobber exceptions set in security wrapper check() hooks. v1

(In reply to Blake Kaplan (:mrbkap) from comment #2)

> isExceptionPending checks scare me because they often get confused by OOM.
> Do our policies deal properly with that?

Sort of. check() doesn't have regular JS semantics (that is to say, it's a predicate), so we don't have a great way of communicating OOM out of the policies. This patch is more or less orthogonal to that issue. That is to say, it doesn't introduce the classic

|if (!cx->isExceptionPending()) { /* all good! */ }|

bug, because we've already checked the return value. It's just limiting the places where we might throw to avoid clobbering a more useful exception that's there already.

Properly propagating OOM out of check() would be a larger refactoring that IMO isn't really worth it.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 723654 [details] [diff] [review]
Don't clobber exceptions set in security wrapper check() hooks. v1

In that case, let's get this in. I'm not terribly excited about the OOM situation, but it might be enough to just say that enter isn't allowed to OOM...

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19f154ee6f54

Comment 6

[Bobby Holley (:bholley)]

Doh, cx->isExceptionPending() doesn't work in jsproxy.h. I'll switch to JS_IsExceptionPending, given that this isn't a hot part of that function.

Comment 7

[Bobby Holley (:bholley)]

Let's be extra sure this builds everywhere. I don't think it needs a unittest run:

https://tbpl.mozilla.org/?tree=Try&rev=d78b00c67b7e

Comment 8

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #7)
> Let's be extra sure this builds everywhere. I don't think it needs a
> unittest run:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d78b00c67b7e

I meant to do that for all platforms, but oh well. Pushing to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e357cea6f6

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8e357cea6f6