Crash [@ js::jit::LinearScanAllocator::populateSafepoints] or Assertion failure: IsCompatibleLIRCoercion(def->type(), as->type()), at jit/shared/Lowering-shared-inl.h

VERIFIED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
JavaScript Engine: JIT
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla33
x86_64
Mac OS X
assertion, crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32+ verified, firefox33+ verified, firefox-esr24 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8436364 [details]
stack

function f() {
    (Math.ceil(0 || x && y) | 0)()
}
try {
    f()
} catch (e) {}
f()

asserts js debug shell on m-c changeset 62d33e3ba514 with --ion-eager --ion-parallel-compile=off at Assertion failure: IsCompatibleLIRCoercion(def->type(), as->type()), at jit/shared/Lowering-shared-inl.h

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140521074416" and the hash "c61733f749ec".
The "bad" changeset has the timestamp "20140521075322" and the hash "59d8d82211f2".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c61733f749ec&tochange=59d8d82211f2

Setting s-s and assuming sec-critical pending further analysis, because this seems to involve LIR. I have another upcoming testcase that crashes 32-bit opt shells at js::jit::LinearScanAllocator::populateSafepoints.

Nicolas, is bug 1000605 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Comment 1

4 years ago
Created attachment 8436370 [details]
stack for testcase that crashes 32-bit opt shell

function f() {
    (Math.ceil(y | 0 || x && y) | 0)(y);
}
try {
    f();
} catch (e) {}
f();

crashes 32-bit js opt shell on m-c rev 62d33e3ba514 at js::jit::LinearScanAllocator::populateSafepoints

$ ./js-opt-32-dm-ts-darwin-62d33e3ba514 --ion-eager --ion-parallel-compile=off testcase.js
Bus error: 10

Configuration parameters:
CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Nicolas, is bug 1000605 a likely regressor?

I am not completely sure how, but the fact that Math.ceil is used seems to relate to this patch.
I will check later today.
After some quick investigation, it appears that the expression 0 || x && y is specialized as Int32 during IonBuilder, and then de-specialized as Value later in the pipeline (which makes sense, as x and y are unknown). Since bug 1000605, there is a MLimitedTruncate in this case, to prevent further optimization, which sets its type as the type of its input. As a matter of fact, the LimitedTruncate is specialized as Int32 but its input is specialized as Value, and redefine() doesn't like that.

As far as I can remember, we shouldn't rely on inputs types during IonBuilder to compute types of newly created instructions, so we need to find another way to do this. A type policy doesn't seem appropriate as we precisely don't want to change the input type. What about making a kind of infer() function, as we have for MBinaryArithInstruction?
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> A type policy doesn't seem appropriate as we
> precisely don't want to change the input type. What about making a kind of
> infer() function, as we have for MBinaryArithInstruction?

I tihnk a type policy makes sense as MLimitedTruncate is used as a substitute of MFloor / MCeil which are both expecting to see Numbers as arguments.
Created attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

This patch is a WIP, I still need to test it properly before asking for review.
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

I checked with the test case from comment 0, and this patch is indeed fixing this issue, as well as being green on try.

https://tbpl.mozilla.org/?tree=Try&rev=0f5fcb30a5e1
Attachment #8437914 - Attachment description: WIP - Add ArithPolicy to MLimitedTruncate. → Add ArithPolicy to MLimitedTruncate.
Attachment #8437914 - Flags: review?(benj)
Flags: needinfo?(nicolas.b.pierron)
status-firefox32: --- → affected
status-firefox33: --- → affected
(Reporter)

Comment 7

4 years ago
Seems like :nbp is working on this.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

Review of attachment 8437914 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. That works only because MLimitedTruncate is used as an Int32, right? Would there be a way to make it very clear that it's only used by Ceil / Floor and such? Either a check during lowering (on the MUse of the MLimitedTruncate), or just by renaming the MIR node to MLimitedTruncateInt32?

::: js/src/jit/MIR.h
@@ +1021,5 @@
>  // No-op instruction. This cannot be moved or eliminated, and is intended for
>  // protecting the input against follow-up optimization.
> +class MLimitedTruncate
> +  : public MUnaryInstruction,
> +    public ArithPolicy

nit: You can actually use ConvertToInt32Policy<0> and avoid the specialization_ member.
Attachment #8437914 - Flags: review?(benj)
Oh, and don't forget to add the test case, please.
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Looks good to me. That works only because MLimitedTruncate is used as an
> Int32, right?

Right, but at the same time the "truncate" suffix is only meaningful if the output is in the Int32 range, so, as the intent of this instruction is to be a no-op, it makes some sense to have it ensure that its input is also an Int32.

> Would there be a way to make it very clear that it's only used
> by Ceil / Floor and such? Either a check during lowering (on the MUse of the
> MLimitedTruncate), or just by renaming the MIR node to MLimitedTruncateInt32?

I do not think a check on the lowering would be nice, as I would expect us to be able to remove the Ceil / Floor / Round operations with some additional range analysis optimizations.

On the other hand the limited truncate is introduced because we expect the Int32 in the first place.  Maybe I should make a separate patch rename the class to MInt32LimitedTruncate?
(Reporter)

Comment 11

4 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Oh, and don't forget to add the test case, please.

Erm, note that the merge window has passed and this might affect more than just nightly. *hint hint* sec-approval.
(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> On the other hand the limited truncate is introduced because we expect the
> Int32 in the first place.  Maybe I should make a separate patch rename the
> class to MInt32LimitedTruncate?

I'm fine with that renaming in a follow up bug. For what it's worth, I prefer MLimitedTruncateInt32 to MInt32LimitedTruncate, but just having Int32 in the name sounds good.
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

r=me with using the ConvertToInt32 policy rather than the ArithPolicy.
Attachment #8437914 - Flags: review+
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

This would be hard on 32 bits (as accesses are controlled by LinearScan).  I guess, this is doable on 64 bits platform (as we might be able to inject a fake pointer with a bailout).

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

The patch it-self highlights what kind of security issue it might be if we managed to compile it (x64).  The subject of the patch only paraphrase the content of the patch.

> Which older supported branches are affected by this flaw?
Gecko 31

> If not all supported branches, which bug introduced the flaw?
Bug 1000605

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

They should be identical to this patch.

> How likely is this patch to cause regressions; how much testing does it need?

This patch is adding a bailout when an input is not an Integer as we would have expected.  This bailout was already present before Bug 1000605, as it was an input of a math expression.
Attachment #8437914 - Flags: sec-approval?
(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> [Security approval request comment]
> > Which older supported branches are affected by this flaw?
> Gecko 31

I meant, Gecko 32, based on Bug 1000605 target milestone.
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

Sec-approval+ for trunk. We should get an Aurora patch to fix it on 32 so we don't ship the issue anywhere.
Attachment #8437914 - Flags: sec-approval? → sec-approval+
status-firefox31: --- → unaffected
tracking-firefox32: --- → +
tracking-firefox33: --- → +
Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

This patch applies cleanly on top of aurora without merge conflicts.
I will land it on inbound.
Attachment #8437914 - Flags: approval-mozilla-aurora?
Attachment #8437914 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/d2152031a5d1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
JSBugMon: This bug has been automatically verified fixed.
status-firefox32: affected → fixed
status-firefox32: fixed → verified
JSBugMon: This bug has been automatically verified fixed on Fx32
status-b2g-v1.3: --- → unaffected
status-b2g-v1.3T: --- → unaffected
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox-esr24: --- → unaffected
Group: javascript-core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.