Closed Bug 1329933 Opened 3 years ago Closed 3 years ago

Assertion failure: Double input should be equal or higher than Lowerbound., at js/src/jit/MacroAssembler.cpp:1598

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + verified
firefox53 + verified

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [fuzzblocker][jsbugmon:update])

Attachments

(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision d192a99be4b4 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis):

function g(f) {
    for (var j = 0; j < 999; ++j) {
        f(0 / 0);
    }
}
function h(x) {
    x < 1 ? 0 : Math.imul(x || 0);
}
g(h);

Backtrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	0x00000001049f1929 0 + 4372502825

For detailed crash information, see attachment.

Setting s-s as a start because this assertion talks about ranges, bounds and so seems scary, also there is no stack.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/01d621c2dbe3
user:        Johannes Schulte
date:        Fri Jun 24 19:02:23 2016 +0200
summary:     Bug 1176230 - Try to fold ternary's with double-argument to NaNToZero. r=nbp

Nicolas, is bug 1176230 a likely regressor?
Blocks: 1176230
Flags: needinfo?(nicolas.b.pierron)
Also setting [fuzzblocker] because I also see testcases crashing with no stack, involving instructions like xorpd.
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Hannes, do you have time to look at the following?
Flags: needinfo?(nicolas.b.pierron) → needinfo?(hv1989)
[Tracking Requested - why for this release]:
Added in FF52. Possibly exploitable since we can do bogus optimization based on wrong range information.
Flags: needinfo?(hv1989)
Assignee: nobody → hv1989
Attachment #8826195 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8826195 [details] [diff] [review]
Patch: add 0 to the range if NaN is in the input range.

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

r=me once fixed (if you agree with the following fix)

::: js/src/jit/RangeAnalysis.cpp
@@ +1222,3 @@
>          copy->max_exponent_ = Range::IncludesInfinity;
> +        if (!copy->canBeZero())
> +            copy->setLowerInit(0);

The range could have a negative upper limit and/or reach the NaN exponent from the negative.  This might lead to invariant failures or unexpected shrinkage of the Range of the instruction.

The following might add extra work, but is surely less error-prone:

    if (!copy->canBeZero()) {
         Range zero;
         zero.setDoubleSingleton(0);
         copy->unionWith(&zero);
    }


Nice catch.
Attachment #8826195 - Flags: review?(nicolas.b.pierron) → review+
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I assume quite hard. But not impossible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They give the same information as the patch itself.

Which older supported branches are affected by this flaw?
ff52

If not all supported branches, which bug introduced the flaw?
bug 1176230

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

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.
Attachment #8826526 - Flags: sec-approval?
Attachment #8826526 - Flags: review+
Attachment #8826195 - Attachment is obsolete: true
Sec-approval+ for trunk since this doesn't affect 51, which has had its last beta.
We'll want this nominated for aurora as well.
Attachment #8826526 - Flags: sec-approval? → sec-approval+
Comment on attachment 8826526 [details] [diff] [review]
Patch: add 0 to the range if NaN is in the input range.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1176230

[User impact if declined]:
Possibly exploitable

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
The testcase doesn't reproduce anymore and confirmed looking at the graphs ion now creates.

[Needs manual test from QE? If yes, steps to reproduce]: 
/

[List of other uplifts needed for the feature/fix]:
/

[Is the change risky?]:
[Why is the change risky/not risky?]:
Should very contained. We increase the range of this opcode as a result could lead to less optimizations kicking. Thought that should also be very contained. I think this is quite save.


[String changes made/needed]:
Attachment #8826526 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1ec74b000805
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8826526 [details] [diff] [review]
Patch: add 0 to the range if NaN is in the input range.

sec-high jit fix for aurora52
Attachment #8826526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3cfe2b9f1e94

We should be fine to land the test now, yes?
Flags: needinfo?(hv1989)
Flags: in-testsuite?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3cfe2b9f1e94
> 
> We should be fine to land the test now, yes?

Yes: https://hg.mozilla.org/integration/mozilla-inbound/rev/72cfa0d3be9f189bdf73a3e873c5869b7f649cdd
Flags: needinfo?(hv1989)
Flags: in-testsuite? → in-testsuite+
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx52
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.