Closed
Bug 960768
Opened 11 years ago
Closed 11 years ago
Compartment mismatch in js::UnwindIteratorForException
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | --- | unaffected |
firefox29 | --- | fixed |
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | unaffected |
b2g-v1.3 | --- | unaffected |
b2g-v1.4 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: regression, sec-high)
Attachments
(1 file)
1.46 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Seen on crash-stats:
https://crash-stats.mozilla.com/report/index/4a0c9196-12f9-4e77-953f-f2dd02140111
https://crash-stats.mozilla.com/report/index/afe8401e-8e9a-4cf0-ae10-789332131231
The stack looks like this:
JSContext::setPendingException(JS::Value)
js::UnwindIteratorForException
js::jit::HandleExceptionBaseline
The code in UnwindIteratorForException is:
cx->getPendingException(&v);
cx->clearPendingException();
if (!CloseIterator(cx, obj))
return false;
cx->setPendingException(v);
getPendingException has 3 cases:
1. the JSContext is in the atoms compartment
2. wrapping fails
3. we check that the wrapped value is in the right compartment
In case 1, the compartment check should succeed. In case 3, we've already checked, so we should be okay, also. Based on elimination, then, I'm guessing that we're failing to wrap for some reason. Notice that UnwindIteratorForException doesn't check the return value, so assuming a failing wrap leaves the argument alone, then we're ending up setting with an unwrapped v.
Maybe the solution is just to throw away the exception in that case?
I don't know how bad this is, given that we're holding the object alive on the context directly, but maybe something bad could happen later.
Assignee | ||
Comment 1•11 years ago
|
||
This appears to be the only call to JSContext::getException that does not check the return value.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d0093f2ac3ef
Well, maybe it should be high, because it can obviously be triggered somehow, and I assume once it is on the context, anything could grab it and stick it on the heap somewhere.
Keywords: sec-moderate → sec-high
Assignee | ||
Comment 3•11 years ago
|
||
I don't know this code, but I assume we want to clear and close in the case of failure.
Also, let me know if you think the sec rating is reasonable, or if it is too high.
Attachment #8361669 -
Flags: review?(luke)
![]() |
||
Comment 4•11 years ago
|
||
Comment on attachment 8361669 [details] [diff] [review]
Check the result of getPendingException in js::UnwindIteratorForException.
I think your analysis is correct.
Attachment #8361669 -
Flags: review?(luke) → review+
Assignee | ||
Comment 5•11 years ago
|
||
This is a regression from bug 951282, so it only affects 29.
Prior to that, getPendingException was infallible, and wrapping occurred in wrapPendingException. If the wrap failed in the latter, then the exception just got thrown away, so there was no problem.
Blocks: 951282
status-firefox27:
--- → unaffected
status-firefox28:
--- → unaffected
status-firefox29:
--- → affected
status-firefox-esr24:
--- → unaffected
Keywords: regression
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
status-b2g-v1.3:
--- → unaffected
status-b2g-v1.4:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•