Closed Bug 960768 Opened 6 years ago Closed 6 years ago
Compartment mismatch in js::Unwind
Iterator For Exception
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.
This appears to be the only call to JSContext::getException that does not check the return value.
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.
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 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+
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.
You need to log in before you can comment on or make changes to this bug.