1263525 - Differential Testing: Different output message involving std_Array and Object.prototype.prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = [];
''.split(/x/);
''.split(/x/);
__proto__["prototype"] = new ArrayBuffer();
x.filter(function() {});
print(uneval(x.concat([])));


$ ./js-dbg-64-dm-clang-darwin-29d5a4175c8b --fuzzing-safe --no-threads --baseline-eager testcase.js
({})

$ ./js-dbg-64-dm-clang-darwin-29d5a4175c8b --fuzzing-safe --no-threads --ion-eager testcase.js
[]

Tested this on m-c rev 29d5a4175c8b.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 29d5a4175c8b

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1a3a6133271c
user:        Tooru Fujisawa
date:        Sat Sep 05 22:01:43 2015 +0900
summary:     Bug 887016 - Part 13: Implement RegExp.prototype[@@split] and call it from String.prototype.split. r=h4writer,till

arai-san, is bug 887016 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting s-s for now. The other regressing bugs off bug 887016 were both s-s so let's keep this locked pending further analysis.

Comment 2

[Tooru Fujisawa [:arai]]

By replacing RegExpSplit implementation to following:

> function RegExpSplit(string, limit) {
>     GetBuiltinConstructor("RegExp");
> }

Same issue happens with following code:

  var x = [];
  /x/[Symbol.split]();
  /x/[Symbol.split]();
  __proto__["prototype"] = new ArrayBuffer();
  x.filter(function() {});
  var y = x.concat();
  print(Object.getPrototypeOf(y).constructor);

So this is not actually related to @@split.
maybe @@species handling in filter or concat.

Comment 3

[Tooru Fujisawa [:arai]]

it seems to be the difference in std_Array.

in both case, std_Array is called, but it returns different thing for each case.

Comment 4

[Tooru Fujisawa [:arai]]

btw, can anyone tell me what the |__proto__["prototype"] = new ArrayBuffer();| actually means?
what's the expected behavior with the code?

Comment 5

[Tooru Fujisawa [:arai]]

somewhat reduced testcase, that doesn't use split:

var x = [];
__proto__["prototype"] = new ArrayBuffer();
x.filter(function() {});
x.filter(function() {});
var y = x.concat();
print(y.constructor.name);

Comment 6

[Tooru Fujisawa [:arai]]

In inlined path for NewArrayDynamicLength, it doesn't get prototype property, and returns an Array with |Object.getPrototypeOf(y) == Array.prototype,|
In js::ArrayConstructor, it gets prototype property, and returns an Array with |Object.getPrototypeOf(y) == new ArrayBuffer()|.


Then, actually, I'm not sure if/how following output is correct

  > Object.prototype.prototype = new ArrayBuffer(0); print([].concat().constructor.name);
  ArrayBuffer

similar output is shown on Firefox 45 with following:

  > Object.prototype.prototype = new ArrayBuffer(0); print([].map(x => x).constructor.name);
  ArrayBuffer

map was already self-hosted and was using std_Array to create new Array instance.  I think concat falls into similar situation by bug 1233642.


The question is that how the value of Object.prototype.prototype gets accessed from Array constructor?

Comment 7

[Tooru Fujisawa [:arai]]

I think, the issue is that js::ArrayConstructor tries to get "prototype" property of "std_Array" function, not Array constructor, when std_Array is called from Self-hosted JS.

> bool
> js::ArrayConstructor(JSContext* cx, unsigned argc, Value* vp)
> {
>     CallArgs args = CallArgsFromVp(argc, vp);
> 
>     RootedObject proto(cx);
>     if (!GetPrototypeFromCallableConstructor(cx, args, &proto))
>         return false;

> bool
> js::GetPrototypeFromCallableConstructor(JSContext* cx, const CallArgs& args, MutableHandleObject proto)
> {
>     RootedObject newTarget(cx);
>     if (args.isConstructing())
>         newTarget = &args.newTarget().toObject();
>     else
>         newTarget = &args.callee();  // <-- this code is used
>     return GetPrototypeFromConstructor(cx, newTarget, proto);
> }

maybe, we have to create another function for std_Array, that doesn't call GetPrototypeFromCallableConstructor.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Add dedicated function for std_Array self-hosted intrinsic.

Added array_construct that does almost same thing as ArrayConstructor, without following:
  * GetPrototypeFromCallableConstructor
    (we should always use ArrayPrototype)
  * fallback to ArrayFromCallArgs
    (std_Array should be called with length)
  * throw an error when the argument is negative
    (the argument should already be valid length)

With this change, the testcase in comment #0 shows "[]" in both case.

Comment 9

[Tooru Fujisawa [:arai]]

bug 1267041 hits the issue on actual website, we should fix this shortly.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8739958 [details] [diff] [review]
Add dedicated function for std_Array self-hosted intrinsic.

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

r=me with refactor suggested.

::: js/src/jsarray.cpp
@@ +3205,5 @@
>      return true;
>  }
>  
> +bool
> +js::array_construct(JSContext* cx, unsigned argc, Value* vp)

Hmm, I think I would rather do it this way.

Take the body of ArrayConstructor and make it static, and have it take a bool parameter for whether to call GetOrCreateArrayPrototype or GetPrototypeFromConstructor. Then have a little wrapper that calls that.

I don't think the other checks (the int length assertion, or the ArrayFromCallArgs length checks) are expensive enough to be worth adding extra codepaths to remove.

Comment 11

[Eric Faust [:efaust]]

Reviewed; clearing ni?

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b16661bbae5e6c37018bce5122b00d437f703cc
Bug 1263525 - Add dedicated function for std_Array self-hosted intrinsic. r=efaust

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b16661bbae5

Comment 14

[Hannes Verschore [:h4writer]]

Arai: can you look to backport this and bug 1267565 to aurora?

Comment 15

[Tooru Fujisawa [:arai]]

the underlying issue on std_Array was a regression from bug 1055472 (firefox45)

Array#map was using std_Array before bug 1165052.

  Function.prototype.prototype = {};
  console.log([1].map(function(x) {return x;}).shift);

it prints undefined on Firefox 45.0.2, but it should print a function.

anyway, I'll ask approval for aurora first.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Add dedicated function for std_Array self-hosted intrinsic and enable JIT for it. r=efaust

Folded bug 1267565 patch too.

Approval Request Comment
> [Feature/regressing bug #]
* underlying regression from bug 1055472
  https://hg.mozilla.org/mozilla-central/rev/d302571cd5e5
* exposed to more functions by bug 1165052

> [User impact if declined]
Wrong JavaScript behavior for very specific case (bug 1267041)

> [Describe test coverage new/current, TreeHerder]
Tested on mozilla-central automated test

> [Risks and why]
Low, it changes unnecessary code path to simpler one, that was added by bug 1055472.

> [String/UUID change made/needed]
None

Comment 17

[Tooru Fujisawa [:arai]]

So, all supported branches except firefox-esr38 have this issue.
efaust, which branch do you think we should uplift?

I think this is not actually any of security-sensitive, but the wrong behavior is already confirmed on the actual webpage, at least with bug 1165052's case:

  Function.prototype.prototype={};
  var _2=Array.prototype.slice;
  var a=_2.call(arguments);        // slice uses std_Array
  a.unshift;                       // undefined

then, before bug 1165052, following functions were using std_Array:

  * ArrayFrom (Array.from)
  * ArrayMap (Array#map)
  * bind_mapArguments (not exposed)
  * bind_invokeFunctionN (not exposed)

Array.from also hits this in very limited case:

  Function.prototype.prototype = {};
  var x = Array.from;
  function f() {
    return x(arguments).shift;  // ArrayFrom used std_Array because arguments
                                // is not iterable in esr45, and `this` is not
                                // Array
  }
  f(1, 2, 3);                   // undefined

Comment 18

[Eric Faust [:efaust]]

Well, this is pretty clearly idiocy. It should clearly go to 48, and I think even 47. I don't think it's worth going to release with this.

Till, you have experience with this kind of "self-hosted code can be made to be script dependent" thing. What do you think?

Comment 19

[Till Schneidereit [:till]]

I agree that it should be uplifted to beta. The compat issues are probably not substantial enough to warrant uplifting to release, but should we happen to bundle something up for release and adding this to that bundle would be cheap, we might do it, I guess.

I also agree that this isn't sec-sensitive: script that can cause this wrong behavior has substantially more straight-forward ways to do it's nefarious deeds.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Add dedicated function for std_Array self-hosted intrinsic and enable JIT for it. r=efaust

Almost same patch as the patch for mozilla-aurora, except surrounding unrelated code change.

Approval Request Comment
> [Feature/regressing bug #]
bug 1055472

> [User impact if declined]
Wrong JavaScript behavior for very specific case (comment #15 and comment #17)

> [Describe test coverage new/current, TreeHerder]
Tested on mozilla-central automated test

> [Risks and why]
Low, it changes unnecessary code path to simpler one, that was added by bug 1055472.

> [String/UUID change made/needed]
None

Comment 21

[Eric Faust [:efaust]]

Comment on attachment 8746609 [details] [diff] [review]
(mozilla-beta) Add dedicated function for std_Array self-hosted intrinsic and enable JIT for it. r=efaust

See above comment. This should go to 48 as well, since it landed just after the merge on 49.

Comment 22

[Tooru Fujisawa [:arai]]

(In reply to Eric Faust [:efaust] from comment #21)
> Comment on attachment 8746609 [details] [diff] [review]
> (mozilla-beta) Add dedicated function for std_Array self-hosted intrinsic
> and enable JIT for it. r=efaust
> 
> See above comment. This should go to 48 as well, since it landed just after
> the merge on 49.

attachment 8745928 [details] [diff] [review] is for mozilla-aurora.
they need different patch, for surrounding unrelated code change.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8746609 [details] [diff] [review]
(mozilla-beta) Add dedicated function for std_Array self-hosted intrinsic and enable JIT for it. r=efaust

Recent regression, noticeable on Kayak.com, baked in Nightly for a week, Beta47+, Aurora48+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b113090f2cc

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/89075ee71abb