Closed Bug 1423173 Opened 3 years ago Closed 3 years ago

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


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




Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed


(Reporter: gkw, Assigned: djvj)



(Keywords: regression, sec-moderate, testcase, Whiteboard: [post-critsmash-triage][adv-main59+])


(1 file, 1 obsolete file)

var x = [];
for (var j = 0; j < 3; ++j) {
    try {
        x.push(function() {});
    } catch (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:
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.
Flags: needinfo?(kvijayan)
Keywords: regression
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.
Assignee: nobody → kvijayan
Flags: needinfo?(kvijayan)
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.
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?
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.
Priority: -- → P1
Attached patch fix-bug-1423173.patch (obsolete) — Splinter Review
Turns out the shape change was a red herring.  The real issue is the one identified by :jorendorff in comment 4.  Small fix.
Attachment #8937099 - Flags: review?(jorendorff)
Can we add the testcase?
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.
(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

var x = [];
var c = 0;
for (var j = 0; j < 20; ++j) {
    try {
        x.push(function() {});
    } catch (e) {
assertEq(c, j);
Thanks Jan.  I was using basically the same approach.  Turns out I wasn't hooking into the test harness correctly.
Attachment #8937099 - Attachment is obsolete: true
Attachment #8937099 - Flags: review?(jorendorff)
Attachment #8938144 - Flags: review?(jdemooij)
Attachment #8938144 - Flags: review?(jdemooij) → review+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?
Flags: needinfo?(kvijayan)
Flags: in-testsuite+
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.
Flags: needinfo?(kvijayan)
Based on comment 13 I'll mark this wontfix for 58, though it should still get a sec rating.  Dan, who can do that?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Keywords: sec-moderate
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.