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

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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

Comment 2

[Nicolas B. Pierron [:nbp]]

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

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 4

[Nicolas B. Pierron [:nbp]]

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

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Add ArithPolicy to MLimitedTruncate.

This patch is a WIP, I still need to test it properly before asking for review.

Comment 6

[Nicolas B. Pierron [:nbp]]

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

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Seems like :nbp is working on this.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Oh, and don't forget to add the test case, please.

Comment 10

[Nicolas B. Pierron [:nbp]]

(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?

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

(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 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8437914 [details] [diff] [review]
Add ArithPolicy to MLimitedTruncate.

r=me with using the ConvertToInt32 policy rather than the ArithPolicy.

Comment 14

[Nicolas B. Pierron [:nbp]]

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.

Comment 15

[Nicolas B. Pierron [:nbp]]

(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 16

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 17

[Nicolas B. Pierron [:nbp]]

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.

Comment 18

[Nicolas B. Pierron [:nbp]]

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

Comment 19

[Ed Morley [:emorley]]

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

Comment 20

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 21

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c95d58f66658

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32