1423173 - Differential Testing: Different output message involving Object.freeze and __proto__

Status: RESOLVED FIXED

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

Description

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

Array.prototype.push(1);
Object.freeze([].__proto__);
var x = [];
for (var j = 0; j < 3; ++j) {
    try {
        x.push(function() {});
    } catch (e) {
        print(e);
    }
}


$ ./js-64-dm-linux-e19b017880c8 --fuzzing-safe --no-threads --ion-eager testcase.js
TypeError: 0 is read-only
TypeError: 0 is read-only
TypeError: 0 is read-only

$ ./js-64-dm-linux-e19b017880c8 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
TypeError: 0 is read-only
TypeError: 0 is read-only

Tested this on m-c rev e19b017880c8.

My configure flags are:

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

python -u -m funfuzz.js.compile_shell -b "--enable-more-deterministic" -r e19b017880c8

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5bb170d70875
user:        Kannan Vijayan
date:        Tue Jul 25 11:28:38 2017 -0400
summary:     Bug 1366375 - Add CacheIR stub for optimizing calls to array_push.  r=jandem

Kannan, is bug 1366375 a likely regressor?

Setting s-s as a start since this seems to involve Arrays (array_push) and JIT optimization, to be safe.

Comment 1

[Kannan Vijayan [:djvj]]

Looking into this, it's definitely that patch that's causing the issues.  There's some subtle shape-checking behaviour that is going wrong.  Still figuring out EXACTLY why it's happening, but getting closer.

Comment 2

[Kannan Vijayan [:djvj]]

There's something weird going on that I don't understand about the shape behaviour.

The first time the 'push()' is called on the array object (not the first push() in the code on the Array prototype), it changes the shape of the prototype even though the push throws.  The shape change happens within the "SetProperty()" call that fails.

I need to understand both the exact semantics for behaviour in the case of indexed properties on protos, and why this causes a shape change in our implementation even if it ostensibly throws without any user-visible mutation of the array.

Comment 3

[Jason Orendorff [:jorendorff]]

Weird, I don't see the shape of Array.prototype changing after it's frozen. Instead, it looks like js::array_push isn't being called at all the third time through the loop. js::jit::DoCallFallback isn't being called there either.

Anyway an extra shape change shouldn't cause a correctness bug, just unnecessary deoptimization, right?

Comment 4

[Jason Orendorff [:jorendorff]]

Oh, we end up attaching a stub here, but we shouldn't. CanAttachAddElement should return false in this case, because Array.prototype has a non-writable element, and that causes .push() to throw.

It checks proto->isIndexed(), but that isn't good enough, because the indexed properties are stored in elements. It needs to check for OBJECT_FLAG_FROZEN_ELEMENTS.

Now what I don't understand is why it works in Ion.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-1423173.patch

Turns out the shape change was a red herring.  The real issue is the one identified by :jorendorff in comment 4.  Small fix.

Comment 6

[Jan de Mooij [:jandem]]

Can we add the testcase?

Comment 7

[Kannan Vijayan [:djvj]]

I actually tried to get a working test-case out of this, but it's not happening on a regular build (and I can't seem to trigger it).  Seems to only show up when specifically built according to the test conditions.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #7)
> I actually tried to get a working test-case out of this, but it's not
> happening on a regular build (and I can't seem to trigger it).  Seems to
> only show up when specifically built according to the test conditions.

The test below fails with no flags, opt and debug:

  test.js:12:1 Error: Assertion failed: got 10, expected 20

Or with --baseline-eager:

  test.js:12:1 Error: Assertion failed: got 2, expected 20

Array.prototype.push(1);
Object.freeze([].__proto__);
var x = [];
var c = 0;
for (var j = 0; j < 20; ++j) {
    try {
        x.push(function() {});
    } catch (e) {
        c++;
    }
}
assertEq(c, j);

Comment 9

[Kannan Vijayan [:djvj]]

Thanks Jan.  I was using basically the same approach.  Turns out I wasn't hooking into the test harness correctly.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-1423173.patch

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6a92a108abeb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Given that this affects more than trunk, it shouldn't have landed without a sec rating and, if necessary, sec-approval. Can you please comment on the severity here, Kannan?

Comment 13

[Kannan Vijayan [:djvj]]

Ah, sorry about that.  This is not _that_ serious of a security issue.  There aren't any fundamental invariants being broken here, and the post-error state of the system is still consistent.  The only situation where this would be exploitable is if somebody was particularly relying on a very specific usage of corner language features (indexed properties, shadowed properties, frozen objects, etc.) as a security gating mechanism.  This is very unlikely, and the bug is more of a minor correctness issue.

Comment 14

[Julien Cristau [:jcristau]]

Based on comment 13 I'll mark this wontfix for 58, though it should still get a sec rating.  Dan, who can do that?