Closed Bug 1473830 Opened 6 years ago Closed 5 years ago

Differential Testing: Different output message involving forEach

Categories

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

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: gkw, Assigned: mgaudet)

References

Details

(Keywords: sec-other, testcase, Whiteboard: [fuzzblocker][post-critsmash-triage][adv-main66-])

Attachments

(2 files)

Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review
Bug 1473830 - Add testcase r?jandem
47 bytes, text/x-phabricator-request
Details | Review 
x = [];
y = [];
for (var i = 0; i < 99; ++i) {
    x.push(null, 1);
}
x.sort();
y.forEach(function() {});
Array.prototype.reverse.call(x);
x.forEach(function(j) {
    if (j) {
        Array.prototype.forEach.call(x, function() {});
    }
    this.t = Int8Array;
}, 0);
print(uneval(this.t));


$ ./js-dbg-64-dm-linux-afdeb0288690 --fuzzing-safe --no-threads testcase.js 
function Int8Array() {
    [native code]
}

$ ./js-dbg-64-dm-linux-afdeb0288690 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js 
(void 0)

Tested this on m-c rev afdeb0288690.

My configure flags are:

'AR=ar' 'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--enable-debug' '--enable-more-deterministic' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic" -r afdeb0288690

Bisection underway...

Setting s-s as a start because TypedArrays are involved.
This is quite complex (and probably old):

https://hg.mozilla.org/mozilla-central/rev/dc3ea89ac257
in Dec 2015 did not show any mismatches.

https://hg.mozilla.org/mozilla-central/rev/97b38c3a1063
is an approximate area where the testcase causes:
Assertion failure: !isFloat(), at jit/RegisterSets.h

https://hg.mozilla.org/mozilla-central/rev/be2f6cb7251c
causes the assertion failure to mutate to:
Assertion failure: i < Codes::Total, at jit/x86-shared/Architecture-x86-shared.h

https://hg.mozilla.org/mozilla-central/rev/87c6479a51c6
fixes this assertion failure but the mismatch shows now.

Thus, setting needinfo? from Jan as a start (he fixed the last assertion failure).
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #1)
> https://hg.mozilla.org/mozilla-central/rev/97b38c3a1063
> is an approximate area where the testcase causes:
> Assertion failure: !isFloat(), at jit/RegisterSets.h

fwiw, this assertion failure shows up at:

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/33d2af1ba94e
user:        Jan de Mooij
date:        Tue Dec 01 11:00:22 2015 +0100
summary:     Bug 922406 - Ion-compile global scripts that use 'this'. r=shu
Curious, looks like a very old bug. I'll take a look today.
Regression from bug 1246229.

We're DCE'ing a TypeBarrier in the OSR block (for the T variable in ArrayForEach - we expect it's |undefined| but it's an object when we OSR). Type barriers for undefined/null don't have uses and are always followed by MConstant - I think this means we always DCE type barriers for null/undefined in the OSR block? :/

Simpler testcase, fails with --no-threads:

x = [];
y = [];
for (var i = 0; i < 100; ++i) {
    x.push(null, 1);
}
x.sort();
y.forEach(function() {});
x.reverse();
x.forEach(function(j) {
    "use strict";
    assertEq(this, 4);
    if (j) {
        x.forEach(function(z) { try {} finally {}; });
    }
}, 4);
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
One idea would be to special case for any replacement which is made for an optimized out variable by adding a flag on the replaced instruction to record that it got optimized out.  In which case the TI guards added by Ion builder would not have such a flag and would not be removed by DCE.

Honestly I am surprised this kind of bug appears that late after we introduce this optimization.
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P1
This sounds like it isn't a security issue. If that's wrong, please delete the sec-other keyword or change it to an appropriate rating.
Keywords: sec-other
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
This blocks other differential testing bugs involving TypedArrays, setting [fuzzblocker] as per :nbp recommendation in-person as well...
Whiteboard: [fuzzblocker]
status-firefox63: affected → wontfix
status-firefox64: --- → wontfix
status-firefox65: --- → fix-optional
Flags: needinfo?(nicolas.b.pierron)

Nicolas, what's next here?

Summary: Differential Testing: Different output message involving TypedArrays → Differential Testing: Different output message involving forEach
Flags: needinfo?(nicolas.b.pierron)

Removing my-self from the assignee, it is unlikely that I would be looking at it in the upcoming months, as I am focusing on ARM64 deadline.

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)

Nicolas, what's next here?

Do what is suggested in comment 5.

Assignee: nicolas.b.pierron → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(nicolas.b.pierron)

Ted/Iain/Matthew, any takers? The clues are in comment 5.

Flags: needinfo?(tcampbell)
Flags: needinfo?(mgaudet)
Flags: needinfo?(iireland)

I'm going to take a look at this for the next little bit.

Assignee: nobody → mgaudet
Flags: needinfo?(tcampbell)
Flags: needinfo?(mgaudet)
Flags: needinfo?(iireland)

Type barriers are marked as Guard instructions, however, in OSR blocks guards
are eligible for DCE. However, Null/Undefined/MagicOptimizedOut have no uses
associated with them, and so get optimized out. To prevent that, this patch
uses the ImplicitlyUsed flag to indicate to DCE that these barriers are not
eligible for elimination.

Attachment #9045030 - Attachment description: Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedOut as implicitly used, and don't eliminate them during DCE r?jandem → Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem

I have a couple questions here that may affect how we decide we want to fix this one.

The attached patch is a very targeted fix, preventing the exact bad case from occurring here. However, I got the impression when talking to Nicolas that he had some lingering doubts about the core optimization that caused the issue, which was the patch delivered in Bug 1246229.

First, a quick practical note: Bug 1246229 was trying to speed up jit-test/tests/basic/testTypedArrayInit.js. I will note that I can no longer see a speed difference on that test with and without the opt. I also see an equal number of bailouts with and without the opt. We may well be better served by just backing out Bug 1246229.

For my own education about Ion, I do have a question:

The comment on MTypeBarrier says Given a value, guard that the value is in a particular TypeSet, then returns that value.; I read this as a 'passthrough' style node that can optionally bail if the type isn't what's in the type set. However, through some experimentation I've done, it appears that a TypeBarrier for Null/Undefined/MagicOptimizedOut cannot have any uses -- so we cannot use a TypeBarrier for a passthrough of one of those three types. This limitation, combined with Nicolas' opt, seems to be the root cause of this bug: In IonBuilder::addOsrValueTypeBarrier we have to create new constants, rather than tie the values directly to the inputs, and it is this lack of uses that causes Nicolas' patch to remove what turns out to be a functionally required TypeBarrier.

But it feels like if we could universally use TypeBarriers as passthroughs for any types, we could then actually deploy Nicolas' opt on -any- type barrier. So long as the data flow is correct, then a guard for an unused value should be safe to remove. Right?

Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #13)

But it feels like if we could universally use TypeBarriers as passthroughs for any types, we could then actually deploy Nicolas' opt on -any- type barrier. So long as the data flow is correct, then a guard for an unused value should be safe to remove. Right?

I think your analysis is correct but IIRC passthrough might not be easy to do for all type barriers. I agree it's worth considering. We just need to make sure the known null/undefined cases are still no-ops in the new world.

I'm not sure about backing out bug 1246229: the same issue might still show up in other tests/loops. Maybe we can try to write a testcase for this to verify.

For this bug I'd suggest going with your minimal fix (because low-risk uplift) and then we can reconsider in a follow-up bug :)

Flags: needinfo?(jdemooij)

Comment on attachment 9045030 [details]
Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem

Security Approval Request

How easily could an exploit be constructed based on the patch?

Not very easily

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

No

Which older supported branches are affected by this flaw?

all

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

Bug 1246229

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

I don't have patches pre-prepared, however they should be easy to generate (there's a fair chance this patch will cleanly graft onto all supported branches).

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

This patch is a safe targeted fix, and so should not cause any regressions.

Attachment #9045030 - Flags: sec-approval?

Comment on attachment 9045030 [details]
Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem

As a sec-other, it doesn't need sec-approval to go in so I'm clearing. Feel free to land.

Attachment #9045030 - Flags: sec-approval?

After waiting all day in the autoland queue, landed as

https://hg.mozilla.org/integration/autoland/rev/6795ccfe2b9b04d9f70bc0709e6fbecd4c76ce94

Flags: needinfo?(nicolas.b.pierron)

https://hg.mozilla.org/mozilla-central/rev/6795ccfe2b9b

Since this is sec-other, can we go ahead and land a test for this? Also, is this something we should consider for Beta backport or can it ride the trains?

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox65: fix-optional → wontfix
status-firefox66: --- → affected
status-firefox67: --- → fixed
status-firefox-esr60: --- → wontfix
Flags: needinfo?(mgaudet)
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment on attachment 9045030 [details]
Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1246229
  • User impact if declined: Potential for wrong output
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Reduces the impact of a small opt to restore correctness.
  • String changes made/needed: None
Flags: needinfo?(mgaudet)
Attachment #9045030 - Flags: approval-mozilla-beta?

(I'll prepare a patch to land the test before Monday)

Comment on attachment 9045030 [details]
Bug 1473830 - Mark OSR TypeBarriers for Null/Undefined/MagicOptimizedArguments as implicitly used, and don't eliminate them during DCE r?jandem

Sounds like this might be useful for improving fuzzing results, however much I don't understand the rest of it. Let's take it for beta 11.

Attachment #9045030 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [fuzzblocker] → [fuzzblocker][post-critsmash-triage]
Whiteboard: [fuzzblocker][post-critsmash-triage] → [fuzzblocker][post-critsmash-triage][adv-main66-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: