Open Bug 1538386 Opened 5 years ago Updated 5 months ago

Global Object property resolution is observable with for-in

Categories

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

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

Tracking

()

Tracking Flags:
Tracking Status
firefox68 --- fix-optional

People

(Reporter: gkw, Unassigned)

References

Details

(Keywords: testcase)

Edit: See updated testcase in comment 7.

undefined;
__proto__[2
    .v] = 2;
count = 0;
for (let x in this) {
    if (count ===246) {
        print("count is: " + count);
    }
    count++;
}

$ ./js-dbg-64-dm-linux-x86_64-fd1adb9941e9 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
count is: 246
$

$ ./js-dbg-64-dm-linux-x86_64-fd1adb9941e9 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
$

Tested this on m-c rev fd1adb9941e9.

My configure flags are:

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift

python3 -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic" -r fd1adb9941e9

I'm setting this as s-s because this came up not long after I made some tweaks (with :decoder's assistance) in lieu of the pwn2own bugs this year, just to be safe.

Running bisection soon...

It is unlikely the regressor. I rather think that this patch changed the number of global objects.

:gkw, what happens if you let it print the count after the for loop instead?

Actually the count printing isn't needed. It's just that it dumps too much stdout, hence why I put it there. I can start another bisection for a testcase without the count stuff:

undefined;
__proto__[2
    .v] = 2;
for (let x in this) {
    print("FOO");
}

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/51f0acbee7c9
user: Jan de Mooij
date: Wed Jun 14 10:39:07 2017 +0200
summary: Bug 1370608 part 2 - Add JS_NewEnumerateStandardClasses and use it in js/src. r=evilpie

Jan, is bug 1370608 a likely regressor?

Blocks: 1370608
No longer blocks: 1403679
Flags: needinfo?(andrebargull) → needinfo?(jdemooij)

Could this be a conceptual problem in JS_NewEnumerateStandardClasses, because it doesn't add non-enumerable properties to the visited set, which means prototype properties with the same name are iterated over in for-in?

It looks like that is indeed what is going on.

Here's a cleaned-up testcase:

this.__proto__[undefined] = 3;
for (let x in this) {
    if (x === 'undefined') {
	print("WRONG");
    }
}

|undefined| is defined on the global object as an unenumerable property: https://tc39.github.io/ecma262/#sec-undefined. If we define "undefined" as an enumerable property on the prototype of the global object, we see it in a for-in. This is incorrect according to the spec (https://tc39.github.io/ecma262/#sec-enumerate-object-properties):

Enumerating the properties of the target object includes enumerating properties of its prototype, and the prototype of the prototype, and so on, recursively; but a property of a prototype is not processed if it has the same name as a property that has already been processed by the iterator's next method. The values of [[Enumerable]] attributes are not considered when determining if a property of a prototype object has already been processed.

It looks like we have the same bug with this.proto["Infinity"] and this.proto["NaN"].

One thing to note in the testcase of Comment 7 is that 'undefined' will trigger the property resolution and skew behaviour. If you use 'void 0' instead, the test even fails on the interpreter (--no-baseline too). Baseline uses short-circuiting in [1] which is why the behaviour appears to be JIT differential.

[1] https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/src/jit/BaselineCompiler.cpp#2905

Yeah, the newEnumerate Class hook needs a way to return non-enumerable properties. Probably a struct {jsid id, bool enumerable} or two Vectors.

Group: javascript-core-security
Severity: major → normal
Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)

We already have the enumerableOnly flag. I wonder if the simplest fix is to just call newEnumerate twice for this for-in case.

(In reply to Ted Campbell [:tcampbell] from comment #10)

We already have the enumerableOnly flag. I wonder if the simplest fix is to just call newEnumerate twice for this for-in case.

Problem is that the global has a ton of lazy properties (in the browser) and this path is somewhat hot, so calling it twice with enumerableOnly true and false just to fill the |visited| set would be unfortunate...

Let's wait for bug 1534967 to land first to avoid conflicts.

Depends on: 1534967

Test in comment 7 still fails with --baseline-eager.

Oops, you're right.

Clearing the NI. It's a bug we should fix, but it's also pretty obscure and non-trivial to fix so it's not exactly high priority.

Flags: needinfo?(jdemooij)

(In reply to Jan de Mooij [:jandem] from comment #16)

Clearing the NI. It's a bug we should fix, but it's also pretty obscure and non-trivial to fix so it's not exactly high priority.

May I suggest to bump up this priority? It's not easy to ignore this particular issue involving __proto__, I found bug 1548063 separately and then had to sieve out a bunch of this issue's dupes to find these other testcases as they all have __proto__.

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

Raising priority to P1, based on Gary's comment.
Jan, can you investigate since you seems to have fixed all other bugs of this category lately.

Flags: needinfo?(nicolas.b.pierron)
Priority: P2 → P1
Priority: P1 → P2

I spent some time looking at bug 1764563 before noticing that anba had duped it to this bug.

One thing I noticed was that the bug didn't happen in that case if we had ever resolved the undefined property on the global (using JS_ResolveStandardClass. In that case, the normal enumeration code sees the unenumerable undefined property on the global, and adds it to visited but not props. When we enumerate Object.prototype, undefined is already in visited, so we filter it out and get the right answer.

The problem happens when, instead of calling GetNameOperation, we use the optimization in baseline or WarpBuilder to optimize the lookup of undefined/NaN/infinity. (Ted pointed this out in comment 8.)

Is it the case that this only affects undefined / NaN / Infinity? How bad would it be to make them normal properties on the global instead of resolving them lazily? Would that fix the problem?

Severity: normal → S3

I took a look at this because of my work in Bug 1833484 which blessed the optimization we had in Baseline right into the parser; so now this isn't a differential fuzzing issue, this is just a straight up resolve hook issue.

While fixing this for NaN, undefined and Infinity is straightforward enough (define these values on the global early as all globals should have them), fixing this for globalThis is harder. See the comment on GlobalObject::maybeResolveGlobalThis for some details.

See Also: → 1237935
Priority: P2 → P3
Summary: Differential Testing: Different output message involving __proto__ → Global Object property resolution is observable with for-in
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.