Closed Bug 995607 Opened 6 years ago Closed 6 years ago

AutoDebugModeInvalidation can discard baseline scripts that are on the stack

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

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)

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.
Attached patch Patch (obsolete) — Splinter Review
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: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8405761 - Flags: review?(shu)
Oh and I wonder why the fuzzers didn't catch this... I'll try to write a shell testcase.
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.
jryans, can you check if the patch here fixes bug 982675?
Flags: needinfo?(jryans)
(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)
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 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+
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)
Attached patch PatchSplinter Review
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+
It would be great if we could get that for 29 beta 9 (gtb next Thursday). Thanks
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+
Flags: needinfo?(sledru)
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)
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?
Attachment #8406027 - Flags: approval-mozilla-beta?
Attachment #8406027 - Flags: approval-mozilla-beta+
Attachment #8406027 - Flags: approval-mozilla-aurora?
Attachment #8406027 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/eb8e31011371
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Whiteboard: [adv-main29+]
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-]
Group: javascript-core-security → core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.