Closed
Bug 1118996
Opened 6 years ago
Closed 6 years ago
Compartment mismatch / security boundary violation involving gcparam
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: gkw, Assigned: jonco)
Details
(Keywords: crash, regression, testcase)
Attachments
(1 file, 1 obsolete file)
2.52 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
g = newGlobal(); gcparam('maxBytes', gcparam('gcBytes')); evaluate("return 0", ({ global: g, newContext: true })); crashes js debug shell on m-c changeset 2a193b7f395c with --fuzzing-safe --no-threads --no-ion at: $ 2015-01-06/js --fuzzing-safe --no-threads --no-ion 40.js out of memory *** Compartment mismatch 0x101c77000 vs. 0x101c81800 Hit MOZ_CRASH() at /builds/slave/m-cen-osx64-d-0000000000000000/build/src/js/src/jscntxtinlines.h:52 Segmentation fault: 11 The shell was obtained from: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-01-06-mozilla-central-debug/jsshell-mac64.zip I cannot get a stack from the FTP builds as the builds from FTP do not have symbols. Not sure exactly what issue this is, so setting needinfo? from Terrence and Jon as a start, and setting s-s because compartment mismatches seem bad.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 1•6 years ago
|
||
I think AutoNewContext just needs to wrap the exception into the old compartment. This is shell-only code so I don't think it's a security issue.
Assignee: nobody → jcoppeard
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Attachment #8545896 -
Flags: review?(sphink)
Updated•6 years ago
|
Group: core-security
Comment 2•6 years ago
|
||
Comment on attachment 8545896 [details] [diff] [review] bug1118996-AutoNewContext Review of attachment 8545896 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +990,5 @@ > if (throwing) > JS_GetPendingException(newcx, &exc); > newCompartment.reset(); > newRequest.reset(); > + if (throwing && JS_WrapValue(oldcx, &exc)) So if JS_WrapValue fails, this will throw its failure exception if it sets one, otherwise it seems like it will leave oldcx with an uncatchable exception. Uh, I guess that's about as good as it can do.
Attachment #8545896 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 3•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d425a16fe7c
Assignee | ||
Comment 4•6 years ago
|
||
Backed out because the test case doesn't always fail with OOM - sometimes it fails with SyntaxError. https://hg.mozilla.org/integration/mozilla-inbound/rev/936382a9e919
Assignee | ||
Comment 5•6 years ago
|
||
Requesting re-review for the following changes: - catch the syntax error in case zeal or something else makes the test case not trigger the OOM - strengthen the jit-tests harness' definition of OOM to make this testcase fail without the fix applied - also add the correct options to make the testcase fail without the fix
Attachment #8545896 -
Attachment is obsolete: true
Attachment #8546731 -
Flags: review?(sphink)
Comment 6•6 years ago
|
||
Comment on attachment 8546731 [details] [diff] [review] bug1118996-AutoNewContext-v2 Review of attachment 8546731 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to be a sleazeball and mark this r+ even though I don't really understand what's going on. If this fixes the problem, then it seems like it's a nice minimal fix and I'd rather it just get in the tree. But: ::: js/src/jit-test/tests/basic/bug1118996.js @@ +4,5 @@ > +try { > + evaluate("return 0", ({ > + global: g, > + newContext: true > + })); Can you comment here or in the catch() that this is expected to OOM, but if it doesn't due to zeal settings, then it will throw a "SyntaxError: return not in function"? (It took me a while to figure out why this would throw a SyntaxError.)
Attachment #8546731 -
Flags: review?(sphink) → review+
Comment 7•6 years ago
|
||
https://hg.mozilla.org/projects/cypress/rev/55b18ee8ffb7 https://hg.mozilla.org/projects/cypress/rev/1f8566481a95
Assignee | ||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf0de7bbc4c
https://hg.mozilla.org/mozilla-central/rev/cdf0de7bbc4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•