928625 - Crash with SIGTRAP and --ion-check-range-analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e25e62d174ed (run with --fuzzing-safe --ion-eager --ion-check-range-analysis):


var summary = true;
evaluate("var summary = 'Array slice when arrays length is assigned';");
evaluate('var summary;');

Comment 1

[Christian Holler (:decoder)]

CCing sunfish and nbp since this is a range analysis bug. This triggers really easy so it would be great to fix it quickly.

Comment 2

[Dan Gohman [:sunfish]]

An Unbox with type bool is returning a value which is not 0 or 1. The newly enhanced range checking code is now capable of detecting this.

In a debugger, the value being unboxed to bool actually contains a Value of the string jsval("Array slice when arrays length is assigned").

This looks like a pre-existing bug in the JIT. The variable "summary" starts out as a boolean but is reassigned to a string inside an evaluate statement. Possibly something in the JIT is overlooking that reassignment and is assuming that "summary" is still a boolean afterwards.

I'll continue to investigate, but this is getting into parts of the JIT I'm not yet familiar with, so someone more familiar with this area may be able to find the underlying problem more quickly.

If a workaround is desired, I can prepare a patch to disable range checking for boolean values.

Comment 3

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

I may also be hitting this on jsfunfuzz, but I can't tell for sure yet other than that they all seem to involve --ion-check-range-analysis.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: dont-check-unused-values.patch

Ok, it was simpler than I thought. IonBuilder::pushTypeBarrier thinks that it's ok to omit type barriers for unboxes whose value isn't used. It just emits an unbox with the wrong type and assumes that everything will be fine. This is surprising, but not necessarily a bug.

Attached is a patch which disables range checking for values that aren't used. It fixes the testcase in this bug. I'm not sure that this is the right approach though.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: unused-unbox.patch

Here's a patch I like better. Previously, when the result of an unbox isn't used, the code would omit the type barrier and just emit an invalid unbox. With this patch, it avoids creating the unbox altogether.

This seems good in general, and is a nicer fix to this bug since it restores the invariant that all instructions conform to their type (except Ursh which is special). However, I'm not deeply familiar with this code so I don't know if there are any hidden problems.

Comment 6

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47797798a205

Also clearing the needinfo, since I think we have everything we need here.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/47797798a205

Comment 8

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

How did this get checked in unrated with no affected range?

Does this *only* affect 27?

Comment 9

[Dan Gohman [:sunfish]]

Yes, this only affects 27, my apologies for not making that explicit. The issue was introduced in the changes for bug 925586.

Also, this bug is probably not exploitable. The bug here was that bug 925586 introduced stronger consistency-checking code which detected an inconsistency. The inconsistency only happened with values that are unused, so it was harmless in practice.

Comment 10

[Andrew McCreight [:mccr8]]

Thanks for the information!