Closed Bug 831846 Opened 7 years ago Closed 7 years ago

Compartment mismatch with evalcx and watch

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 --- unaffected
firefox21 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: Benjamin)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Attachments

(2 files, 1 obsolete file)

Attached file stack
evalcx('').watch("", /()/)

crashes js debug shell on m-c changeset ce9cdd801a73 without any CLI arguments with a compartment mismatch message:

*** Compartment mismatch 0x101845600 vs. 0x10184d400
Segmentation fault: 11

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   118999:39885ae3a597
user:        Brendan Eich
date:        Tue Jan 15 18:17:50 2013 -0800
summary:     Bug 810525 - unregress DecompileValueGenerator change to handle object literal reference bases (r=jandem).
s-s because this involves a compartment mismatch.

Brendan, is bug 810525 a likely regressor?
Bug 830389 is another compartment mismatch (without a testcase) involving js_ValueToSource (though likely unrelated).
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Bug 830389 is another compartment mismatch (without a testcase) involving
> js_ValueToSource (though likely unrelated).

That seems unlikely to be related. Scoobidiver in bug 821733 comment 11 says that crashes "have almost completely stopped since 21.0a1/20130111".

autoBisect points to changeset 39885ae3a597 which landed yesterday around noon.

Although I won't exactly discount a latent bug appearing occasionally.
Right, crashes with that signature were mostly due to a different print preview issue which has been fixed, but the bug I linked to also involved a compartment mismatch with js_ValueToSource. I've only seen that once, though, so it isn't a common issue.
Assuming sec-critical pending further analysis, feel free to change if this is not the case.
Keywords: sec-critical
Attached patch enter some compartments (obsolete) — Splinter Review
We properly enter sandbox's compartment through the CCW over Object.wrap. We then try to generate an error about the regular expression not being callable in the expression decompiler. The expression decompiler starts playing around with objects in the outer global script creating the compartment violation.

This patch makes the expression decompiler be in the compartment of the script's global.
Attachment #703566 - Flags: feedback?(wmccloskey)
No more info to add, comment 6 diagnoses. Hoping this can get fast r+ from a compartment expert (cc'ed gal since he is around).

/be
Flags: needinfo?(brendan)
Comment on attachment 703566 [details] [diff] [review]
enter some compartments

Can you please check that there is no other frame walking path that might have the same problem?
Attachment #703566 - Flags: feedback?(wmccloskey) → feedback+
Attached patch v2Splinter Review
I think I got all the paths. Anyway, here's a patch with an assertion that will go off next time.
Attachment #703566 - Attachment is obsolete: true
Attachment #704358 - Flags: review?(gal)
Assigning to benjamin since this has a patch from him awaiting review from gal.
Assignee: general → benjamin
Status: NEW → ASSIGNED
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
Attachment #704358 - Flags: review?(gal) → review?(wmccloskey)
I invited gal to do an r+. One peer should be enough. Is there a reason for two? Bill seems busy enough for real, also in that he didn't get to the original r? yet.

/be
gal only f+ last time. I asked him by email if he wanted to finish, but he should I should switch defer to someone else.
Comment on attachment 704358 [details] [diff] [review]
v2

Sorry, this got taken off my list when Andreas added the f+. It looks fine.

It does make sense to audit all the FrameIter uses for compartment mismatches, but we should land this patch now.
Attachment #704358 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #14)
> It does make sense to audit all the FrameIter uses for compartment
> mismatches, but we should land this patch now.

File bug 835170 for that.
https://hg.mozilla.org/mozilla-central/rev/10a03f096499
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e589bf878934
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.