1749460 - Differential testing: Baseline misscompute ...arguments with ELEMENT_OVERRIDDEN_BIT.

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

On git commit c4903ab7a0fa778d862731dff097c6a7980346a6 I encountered a miscomputation during differential fuzzing. This seems to be an issue within the baseline execution as ion and v8 compute the same result.

function main() {
    let v74;
    
    for (let i = 0; i < 10; i++) {
        let v48 = class V48 {
            constructor(v50) {
                function v62() {
                    const v66 = {set: () => Object()};
                    const v71 = Object.defineProperty(arguments, 0, v66);
                    v74 = Object(...arguments);
                }
                v62(1);
                return new String("");
            }
        };
        new v48();
    }
    print(v74); // in ion and v8 this is [object Object], if ion is disabled this prints 1
}
main();

Actual results:

Running the sample with ion disabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --baseline-warmup-threshold=10 --fuzzing-safe --differential-testing --no-ion diff.js) the computation in v74 is 1.

If ion is enabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing --ion-warmup-threshold=31 --baseline-warmup-threshold=5 diff.js) the computation in v74 is an object.

Comment 1

[Nicolas B. Pierron [:nbp]]

The following test case reproduce the issue:

function main() {
    let v74;    
    for (let i = 0; i < 10; i++) {
        function v62() {
             const v71 = Object.defineProperty(arguments, 0, {});
             v74 = Object(...arguments);
        }
        v62(1);
    }
    console.log(v74);
}

When commenting v71, Object(...arguments) evaluates to Object(1) which yield a Number object. However, line 71, is supposed to request the creation of the ArgumentObject, which set the argument 1 to the property 0, which is then supposed to be overwritten by Object.defineProperty call.

The problem might be that we create a new ArgumentObject for v74 computation, instead of re-using the one which is in the BaselineFrame, or that we do not guard against overwritten elements in CacheIR dealing with the argument of the JSOp::SpreadCall IC, such as:

    uint8_t flags = ArgumentsObject::ELEMENT_OVERRIDDEN_BIT;
    writer.guardArgumentsObjectFlags(argObjId, flags);

Matthew, would the be the right person to look in Argument Objects in Cache IR?

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

I am definitely the wrong person currently. Iain: is this perhaps a regression from Bug 1688033?

Comment 3

[Iain Ireland [:iain]]

Oh, this is a good catch. It's a regression from bug 1740737, although reading through the pre-existing arguments object code while figuring this out also makes me want to blame the general design of arguments object property lookup.

function foo() {
  "use strict"; // Ensure that the arguments object is unmapped.
  Object.defineProperty(arguments, 0, {value: 2});
  arguments[0] = 3;
  return Object(...arguments);
}

print(foo(1));

The problem is that OptimizeArgumentsSpreadCall checks whether the length or iterator has been overridden for an arguments object, or whether any element has been deleted, and assumes that this is sufficient to justify calling ArrayFromArgumentsObject and copying values from the ArgumentsData array.

This is true if all updates to the elements of the arguments object use SetProperty (eg arguments[0] = 3): the resolve hook for UnmappedArgumentsObject and the custom data property code in NativeSetExistingDataProperty conspire to update the ArgumentsData array directly.

However, if we use defineProperty to redefine an element of an unmapped arguments object, the hasOverriddenElement flag is set, but we don't have any equivalent to the special casing in NativeSetExistingDataProperty. Instead, the updated value is stored in a slot of the arguments object (as if it were a sparse element). If we don't check the overridden element flag in OptimizeArgumentsSpreadCall, then we will read the stale value out of the ArgumentsData, instead of the correct value in the slot. (After the slot has been created, mutation using SetProperty will also update the existing slot instead of using the resolve hook.)

The immediate fix is to change OptimizeArgumentsSpreadCall to check args->hasOverriddenElement() instead of args->isAnyElementDeleted(). In the long run we might also want to clean up some of the arguments object code. For example, it's not clear to me that there's any value in storing the arguments for an unmapped arguments object in a separate array, instead of storing them as regular elements. I also have to believe that there's a cleaner way to make this all work than our current mess of hooks, flags, and special cases.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1749460: Check for any overridden element in OptimizeArgumentsSpreadCall r=jandem

DefineProperty on an arguments object doesn't have the same magic as SetProperty for updating the ArgumentsData array, so we have to make sure that no elements have been overridden in OptimizeArgumentsSpreadCall before calling ArrayFromArgumentsObject.

This is a regression from anba's patches in bug 1740737 to optimize ...arguments. It's the rare bug that only fails in the interpreter. (In higher tiers, we already always guarded on the HasOverriddenElement flag.)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1740737

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2c4a16885ce
Check for any overridden element in OptimizeArgumentsSpreadCall r=jandem

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/f2c4a16885ce

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Iain Ireland [:iain]]

This isn't security sensitive, and is unlikely to trigger in any real-world code. (I can't think of a good reason to assign to elements of arguments, much less to do so using Object.defineProperty.) Unless we start getting bug reports, this should be fine to ride the trains.