Closed
Bug 1423173
Opened 3 years ago
Closed 3 years ago
Differential Testing: Different output message involving Object.freeze and __proto__
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | fixed |
People
(Reporter: gkw, Assigned: djvj)
References
Details
(Keywords: regression, sec-moderate, testcase, Whiteboard: [post-critsmash-triage][adv-main59+])
Attachments
(1 file, 1 obsolete file)
1.02 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Flags: needinfo?(kvijayan)
Updated•3 years ago
|
Keywords: regression
Assignee | ||
Comment 1•3 years ago
|
||
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)
Assignee | ||
Comment 2•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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.
Updated•3 years ago
|
Priority: -- → P1
Assignee | ||
Comment 5•3 years ago
|
||
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)
Comment 6•3 years ago
|
||
Can we add the testcase?
Assignee | ||
Comment 7•3 years ago
|
||
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•3 years ago
|
||
(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);
Assignee | ||
Comment 9•3 years ago
|
||
Thanks Jan. I was using basically the same approach. Turns out I wasn't hooking into the test harness correctly.
Assignee | ||
Comment 10•3 years ago
|
||
Attachment #8937099 -
Attachment is obsolete: true
Attachment #8937099 -
Flags: review?(jorendorff)
Attachment #8938144 -
Flags: review?(jdemooij)
Updated•3 years ago
|
Attachment #8938144 -
Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/6a92a108abeb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 12•3 years ago
|
||
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?
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(kvijayan)
Flags: in-testsuite+
Assignee | ||
Comment 13•3 years ago
|
||
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)
Comment 14•3 years ago
|
||
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)
Updated•3 years ago
|
Flags: needinfo?(dveditz)
Keywords: sec-moderate
Updated•3 years ago
|
Group: javascript-core-security → core-security-release
Updated•3 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•3 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Updated•3 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•