1118996 - Compartment mismatch / security boundary violation involving gcparam

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, critical)

Description

[Gary Kwong [:gkw] [:nth10sd]]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1118996-AutoNewContext

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.

Comment 2

[Steve Fink [:sfink] [:s:]]

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.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d425a16fe7c

Comment 4

[Jon Coppeard (:jonco)]

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

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1118996-AutoNewContext-v2

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

Comment 6

[Steve Fink [:sfink] [:s:]]

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.)

Comment 7

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/55b18ee8ffb7
https://hg.mozilla.org/projects/cypress/rev/1f8566481a95

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf0de7bbc4c

Comment 9

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/cdf0de7bbc4c