Closed Bug 1263525 Opened 4 years ago Closed 3 years ago

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

Categories

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

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- affected
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed
firefox-esr38 --- unaffected
firefox-esr45 --- affected

People

(Reporter: gkw, Assigned: arai)

References

(Blocks 2 open bugs)

Details

(Keywords: sec-audit, testcase, Whiteboard: [adv-main47-])

Attachments

(3 files)

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?
Flags: needinfo?(arai.unmht)
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.
Group: javascript-core-security
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.
it seems to be the difference in std_Array.

in both case, std_Array is called, but it returns different thing for each case.
btw, can anyone tell me what the |__proto__["prototype"] = new ArrayBuffer();| actually means?
what's the expected behavior with the code?
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);
Blocks: 1165052
No longer blocks: 887016
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?
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.
Flags: needinfo?(arai.unmht)
Summary: Differential Testing: Different output message involving RegExp and __proto__ → Differential Testing: Different output message involving std_Array and Object.prototype.prototype
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.
Assignee: nobody → arai.unmht
Attachment #8739958 - Flags: review?(efaustbmo)
bug 1267041 hits the issue on actual website, we should fix this shortly.
Flags: needinfo?(efaustbmo)
See Also: → 1267041
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.
Attachment #8739958 - Flags: review?(efaustbmo) → review+
Reviewed; clearing ni?
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b16661bbae5e6c37018bce5122b00d437f703cc
Bug 1263525 - Add dedicated function for std_Array self-hosted intrinsic. r=efaust
https://hg.mozilla.org/mozilla-central/rev/4b16661bbae5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Arai: can you look to backport this and bug 1267565 to aurora?
Flags: needinfo?(arai.unmht)
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.
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
Flags: needinfo?(arai.unmht)
Attachment #8745928 - Flags: review+
Attachment #8745928 - Flags: approval-mozilla-aurora?
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
Flags: needinfo?(efaustbmo)
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?
Flags: needinfo?(efaustbmo) → needinfo?(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.
Flags: needinfo?(till)
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
Attachment #8746609 - Flags: review+
Attachment #8746609 - Flags: approval-mozilla-beta?
Group: javascript-core-security
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 #8746609 - Flags: approval-mozilla-aurora?
(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 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+
Attachment #8746609 - Flags: approval-mozilla-beta?
Attachment #8746609 - Flags: approval-mozilla-beta+
Attachment #8746609 - Flags: approval-mozilla-aurora?
Attachment #8746609 - Flags: approval-mozilla-aurora+
Attachment #8745928 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1267041
Whiteboard: [adv-main47-]
You need to log in before you can comment on or make changes to this bug.