Closed Bug 1118996 Opened 6 years ago Closed 6 years ago

Compartment mismatch / security boundary violation involving gcparam


(Core :: JavaScript: GC, defect)

Not set





(Reporter: gkw, Assigned: jonco)


(Keywords: crash, regression, testcase)


(1 file, 1 obsolete file)

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:

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)
Attached patch bug1118996-AutoNewContext (obsolete) — Splinter Review
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)
Group: core-security
Comment on attachment 8545896 [details] [diff] [review]

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+
Backed out because the test case doesn't always fail with OOM - sometimes it fails with SyntaxError.
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 on attachment 8546731 [details] [diff] [review]

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