Closed Bug 850000 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

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 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?
Attachment #723654 - Flags: review?(mrbkap)
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.
Attachment #723654 - Flags: review?(mrbkap)
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...
Attachment #723654 - Flags: review?(mrbkap) → review+
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.
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
(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
https://hg.mozilla.org/mozilla-central/rev/b8e357cea6f6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.