Open Bug 1846777 Opened 1 year ago Updated 1 year ago

Indices optimization produces inconsistencies in differential testing with property enumeration

Categories

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

Firefox 118
defect

Tracking

()

UNCONFIRMED

People

(Reporter: 2628388509, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36

Steps to reproduce:

test.js:

function foo(a) {
  //print(Object.keys(a))
  a.sort()
  fuzzilli_hash(a)
  print(Object.keys(a))
  a.push(a[18], 11, a[20], a[1])
}
var array = [];
for (var i = 0; i < 48; ++i) {
  foo(array);
  Array.prototype.some.call(array, function () {
    i += i;
    array = String.prototype.match.call("Tests.", /\s*\W/);
  });
  array[i] = i;
}
for (var i = 0; i < 3; ++i) {
  foo(array);
}

$./js --no-threads --differential-testing --blinterp-warmup-threshold=0 --baseline-warmup-threshold=0 --trial-inlining-warmup-threshold=0 --ion-warmup-threshold=3 test.js

output:
0,index,input,groups
0,1,index,input,groups
0,1,index,input,groups
0,1,2,3,4,5,index,input,groups
0,1,2,3,4,5,6,7,8,9,index,input,groups
executionHash is 0x155cb294 with 6 inputs

$./js --no-threads --differential-testing --blinterp-warmup-threshold=50 --baseline-warmup-threshold=60 --trial-inlining-warmup-threshold=70 --ion-warmup-threshold=80 test.js

output:
0,index,input,groups
0,1,index,input,groups
0,1,index,input,groups
0,1,2,3,4,5,groups,index,input
0,1,2,3,4,5,6,7,8,9,groups,index,input
executionHash is 0x216fffc5 with 6 inputs

Actual results:

Differential testing generates different hash values.

Expected results:

Differential testing should generate the same hash value.

The Bugbug bot thinks this bug should belong to the 'Core::JavaScript Engine: JIT' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core

Ah, this is a sneaky one. It's not a user-facing problem, though, it's just a bug in the tweaks we make to support differential testing.

The order of enumerated properties for Object.keys and for-in loops is not strictly specified. Our tiers have some subtle differences when resolve hooks are involved. When doing differential testing, we therefore sort the keys alphabetically to avoid inconsistencies (see bug 707017). However, when we're using the for-in indices optimization (see here), we can't simply sort the keys, because there's a separate array of indices whose order must correspond to the order of the keys.

Whether we can use the for-in indices optimization depends on a variety of things, in particular whether an object has any indexed properties / sparse elements. It turns out that the capacity of the elements array of a regexp match result depends on where we allocate it. In C++, we base the size on the actual number of capture groups, leading in this case to AllocKind::OBJECT4_BACKGROUND and a capacity of 2. In the regexp stub, we use a single template object, so it has to be large enough to hold the maximum number of matches we can allocate in jitcode, leading to AllocKind::OBJECT16_BACKGROUND and a capacity of 14. In ArrayNativeSortImpl, the capacity affects what happens whether the elements we add for FillWithUndefined are dense or sparse, which in turn affects whether we hit this line in enumerateNativeProperties. The indices optimization doesn't support sparse elements, so the initial difference in capacity ends up deciding whether or not we sort the property keys, leading to this mismatch.

This doesn't affect anything other than fuzzing. If it becomes a fuzz-blocker, we will have to rewrite the sorting code so that it simultaneously sorts the indices array to keep the two arrays in sync.

Thanks for the report!

Severity: -- → S4
Priority: -- → P3
Summary: Array.prototype.sort produces inconsistencies when sorting → Indices optimization produces inconsistencies in differential testing with property enumeration
You need to log in before you can comment on or make changes to this bug.