Closed
Bug 995607
Opened 11 years ago
Closed 11 years ago
AutoDebugModeInvalidation can discard baseline scripts that are on the stack
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla31
| Tracking | Status | |
|---|---|---|
| firefox28 | --- | wontfix |
| firefox29 | + | fixed |
| firefox30 | + | fixed |
| firefox31 | + | fixed |
| firefox-esr24 | --- | unaffected |
| b2g18 | --- | unaffected |
| b2g-v1.1hd | --- | unaffected |
| b2g-v1.2 | --- | unaffected |
| b2g-v1.3 | --- | fixed |
| b2g-v1.3T | --- | fixed |
| b2g-v1.4 | --- | fixed |
| b2g-v2.0 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main29+][qa-])
Attachments
(1 file, 1 obsolete file)
|
2.92 KB,
patch
|
jandem
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
We've seen a number of weird JIT crashes related to the debugger. Bug 982675 is one that affects b2g debugging.
I just pushed a patch to try that much more eagerly JIT-compiles scripts, and we're lucky: bc1 is now crashing reliably:
https://tbpl.mozilla.org/?tree=Try&rev=d7897a97a4cc
It looks like Debugger::addAllGlobalsAsDebuggees has an AutoDebugModeInvalidation for some Zone. Then in ~AutoDebugModeInvalidation, invalidateStack is |false| (because we're turning on the debugger), and we discard baseline scripts that are on the stack.
Although the debugger can't be enabled for compartments with scripts on the stack, the Zone can have compartments that are ignored by the debugger: the debugger's compartment itself or compartments that are invisibleToDebugger(). We don't want to throw away their active scripts.
| Assignee | ||
Comment 1•11 years ago
|
||
This patch removes the invalidateStack condition. Walking the list of activations is pretty fast, especially compared to the CellIter that follows it, so I think this is the safest fix.
| Assignee | ||
Comment 2•11 years ago
|
||
Oh and I wonder why the fuzzers didn't catch this... I'll try to write a shell testcase.
status-b2g18:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox-esr24:
--- → unaffected
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
Keywords: sec-high
| Assignee | ||
Comment 3•11 years ago
|
||
This is hard to repro in the shell, because the shell's default compartment has no principals, and AutoDebugModeInvalidation ignores compartments that have no principals :(
There is newGlobal({principal: foo}), but it's hard/impossible to use that without having scripts of the original global on the stack (the latter will throw).
Anyway, if I allocate ShellPrincipals for the default global, the following crashes:
---
newGlobal({sameZoneAs:{}});
var dbg = new Debugger;
dbg.addAllGlobalsAsDebuggees();
---
Shu, can we remove the principals check from AutoDebugModeInvalidation? As the shell's global has no principals atm, I wonder if it's really valid to ignore these compartments.
| Assignee | ||
Comment 4•11 years ago
|
||
jryans, can you check if the patch here fixes bug 982675?
Flags: needinfo?(jryans)
Comment 5•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3)
> This is hard to repro in the shell, because the shell's default compartment
> has no principals, and AutoDebugModeInvalidation ignores compartments that
> have no principals :(
>
> There is newGlobal({principal: foo}), but it's hard/impossible to use that
> without having scripts of the original global on the stack (the latter will
> throw).
>
> Anyway, if I allocate ShellPrincipals for the default global, the following
> crashes:
> ---
> newGlobal({sameZoneAs:{}});
> var dbg = new Debugger;
> dbg.addAllGlobalsAsDebuggees();
> ---
>
> Shu, can we remove the principals check from AutoDebugModeInvalidation? As
> the shell's global has no principals atm, I wonder if it's really valid to
> ignore these compartments.
The OSR patches happen to get rid of the invalidateStack check too: https://github.com/syg/gecko-dev/blob/debug-mode-osr/js/src/jit/Ion.cpp#L3053
Either way, that check is going away, whether here or once reviews are done for the OSR patches.
As for the principals check, that's copied from previous code. I would check with Jim.
Flags: needinfo?(jimb)
Comment 6•11 years ago
|
||
That said, I don't see why it would be unsafe to always invalidate and just have it recompiled either. Barring really strange requirements here, I believe it is fine to remove the principals check.
Comment 7•11 years ago
|
||
Comment on attachment 8405761 [details] [diff] [review]
Patch
Review of attachment 8405761 [details] [diff] [review]:
-----------------------------------------------------------------
Good catch on the Zone-wide invalidation case!
Attachment #8405761 -
Flags: review?(shu) → review+
Comment 8•11 years ago
|
||
In light of my own comment #6, and since Jim is swamped with stuff to do already, I'm going to flip-flop on my decision to NI Jim and say just remove the principals check.
Flags: needinfo?(jimb)
(In reply to Jan de Mooij [:jandem] from comment #4)
> jryans, can you check if the patch here fixes bug 982675?
Yes, this patch appears to fix bug 982675 on Fennec. Thanks for the quick work!
Flags: needinfo?(jryans)
| Assignee | ||
Comment 10•11 years ago
|
||
Shu, thanks for the quick review.
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily. You have to use the debugger and it's not exposed to content.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.
Which older supported branches are affected by this flaw? 27+.
If not all supported branches, which bug introduced the flaw? Bug 933882.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply; else it will be very easy.
How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8405761 -
Attachment is obsolete: true
Attachment #8406027 -
Flags: sec-approval?
Attachment #8406027 -
Flags: review+
Updated•11 years ago
|
status-b2g-v1.2:
--- → unaffected
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
Comment 11•11 years ago
|
||
It would be great if we could get that for 29 beta 9 (gtb next Thursday). Thanks
Comment 12•11 years ago
|
||
Comment on attachment 8406027 [details] [diff] [review]
Patch
sec-approval+ to get this into trunk. We should get this into Aurora and also Beta, if we can get Beta approval. We should loop in release management for that given the date.
Attachment #8406027 -
Flags: sec-approval? → sec-approval+
Updated•11 years ago
|
Flags: needinfo?(sledru)
Comment 13•11 years ago
|
||
Sure, as said in comment 11, we can accept it in beta9 (next Thursday).
Jan, can we get an uplift request for both aurora & beta? Thanks
Flags: needinfo?(sledru) → needinfo?(jdemooij)
| Assignee | ||
Comment 14•11 years ago
|
||
Flags: needinfo?(jdemooij)
| Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8406027 [details] [diff] [review]
Patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 933882.
User impact if declined: Crashes, sec issues maybe.
Testing completed (on m-c, etc.): Just landed on m-i.
Risk to taking this patch (and alternatives if risky): Pretty low risk; it makes some code unconditional but doesn't add a lot of new code.
String or IDL/UUID changes made by this patch: None.
Attachment #8406027 -
Flags: approval-mozilla-beta?
Attachment #8406027 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8406027 -
Flags: approval-mozilla-beta?
Attachment #8406027 -
Flags: approval-mozilla-beta+
Attachment #8406027 -
Flags: approval-mozilla-aurora?
Attachment #8406027 -
Flags: approval-mozilla-aurora+
Comment 16•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 17•11 years ago
|
||
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
Updated•11 years ago
|
Whiteboard: [adv-main29+]
Comment 18•11 years ago
|
||
Marking [qa-] due to lack of test case, feel free to provide one if you'd like verification. Thanks.
Whiteboard: [adv-main29+] → [adv-main29+][qa-]
Updated•11 years ago
|
Group: javascript-core-security → core-security
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•