Closed Bug 928625 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: sunfish)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, sec-other, testcase, Whiteboard: [fuzzblocker][qa-])

Attachments

(1 file, 1 obsolete file)

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;');
CCing sunfish and nbp since this is a range analysis bug. This triggers really easy so it would be great to fix it quickly.
Whiteboard: [fuzzblocker]
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.
Keywords: helpwanted
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.
Blocks: 349611
Flags: needinfo?(jdemooij)
Attached patch dont-check-unused-values.patch (obsolete) — Splinter Review
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.
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.
Assignee: general → sunfish
Attachment #819956 - Flags: review?(bhackett1024)
Attachment #819956 - Flags: review?(bhackett1024) → review+
Attachment #819354 - Attachment is obsolete: true
Keywords: helpwanted
https://hg.mozilla.org/integration/mozilla-inbound/rev/47797798a205

Also clearing the needinfo, since I think we have everything we need here.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/47797798a205
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
Keywords: verifyme
Whiteboard: [fuzzblocker] → [fuzzblocker][qa-]
How did this get checked in unrated with no affected range?

Does this *only* affect 27?
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.
Thanks for the information!
Group: core-security
You need to log in before you can comment on or make changes to this bug.