1620203 - Assertion failure: whyMagic() == why, at js/Value.h:651 with Reflect and Proxy

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200305-2e1a978b09d7 (build with (buildFlags not available), run with --fuzzing-safe --no-threads --ion-warmup-threshold=1 --baseline-warmup-threshold=0):

"use strict";
var target = {};
var proxy = new Proxy(target, {
    defineProperty(set, [...x27], ... [{p10: q34}]) {}
});
Reflect.defineProperty(proxy, "test", {writable: false})

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555564c0881 in js::jit::GetPropIRGenerator::tryAttachDenseElement(JS::Handle<JSObject*>, js::jit::ObjOperandId, unsigned int, js::jit::Int32OperandId) ()
#0  0x00005555564c0881 in js::jit::GetPropIRGenerator::tryAttachDenseElement(JS::Handle<JSObject*>, js::jit::ObjOperandId, unsigned int, js::jit::Int32OperandId) ()
#1  0x00005555564bbfc0 in js::jit::GetPropIRGenerator::tryAttachStub() ()
#2  0x000055555639e8e6 in js::jit::TryAttachGetPropStub(char const*, JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, js::jit::CacheKind, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) ()
#3  0x000055555639d86f in js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICGetElem_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#4  0x0000054a53184863 in ?? ()
[...]
#28 0x0000000000000000 in ?? ()
rax	0x555556e205ff	93825018234367
rbx	0x7fffffff8430	140737488323632
rcx	0x555557ef4850	93825035880528
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffff8340	140737488323392
rsp	0x7fffffff8300	140737488323328
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9cd00	140737353731328
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x0	0
r13	0x0	0
r14	0x7ffff5e27000	140737318645760
r15	0x0	0
rip	0x5555564c0881 <js::jit::GetPropIRGenerator::tryAttachDenseElement(JS::Handle<JSObject*>, js::jit::ObjOperandId, unsigned int, js::jit::Int32OperandId)+145>
=> 0x5555564c0881 <_ZN2js3jit18GetPropIRGenerator21tryAttachDenseElementEN2JS6HandleIP8JSObjectEENS0_12ObjOperandIdEjNS0_14Int32OperandIdE+145>:	movl   $0x28b,0x0
   0x5555564c088c <_ZN2js3jit18GetPropIRGenerator21tryAttachDenseElementEN2JS6HandleIP8JSObjectEENS0_12ObjOperandIdEjNS0_14Int32OperandIdE+156>:	callq  0x5555557ef05e <abort>

Marking s-s because this assertion can indicate a critical JIT bug.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Nicolas B. Pierron [:nbp]]

Jan, can you look at this issue or forward it to Iain or Matthew?

Comment 3

[Iain Ireland [:iain]]

I took a quick look at this this morning. We are bailing out of the defineProperty handler and leaving a JS_OPTIMIZED_OUT in what I think is the rest array. Then it looks like we try iterating over it (self-hosted ArrayIteratorNext) and fail while attaching a GetElem IC. Not clear to me what the fix is, though.

Comment 4

[Daniel Veditz [:dveditz]]

Dangerous assertion in a sensitive bit of code. It's a release assert so maybe that saves us (in which case we can revise the rating), or maybe there's a way around it?

Comment 5

[Jan de Mooij [:jandem]]

This is a bit similar to bug 1620189. In this case isObservableArgumentSlot also has to test script()->hasRest().

I really don't like the duplication between isRecoverableOperand and isObservableFrameSlot. I'll see if I can untangle this code a bit today.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #5)

I really don't like the duplication between isRecoverableOperand and isObservableFrameSlot. I'll see if I can untangle this code a bit today.

IIRC, Some slots might be observed but cannot be recovered.
Maybe we could have a single function which return an enum-type with the observability/recoverability status.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)

Maybe we could have a single function which return an enum-type with the observability/recoverability status.

Yeah I had the same idea and started working on it this morning, it's much nicer so far.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1620203 part 1 - Clean up isRecoverableOperand and isObservableSlot. r?nbp!

De-duplicate by merging a number of functions into a single method that returns
an enum. Branch on the slot kind first to make the code easier to reason about.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1620203 part 2 - Fix some issues with formal arguments. r?nbp!

Depends on D65707

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1620203 part 3 - Add tests. r?nbp!

Depends on D65708

Comment 11

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200313163649-87ab9a88abce
The bug appears to have been introduced in the following build range:
> Start: 9b7cd94eaf0a2385f53791c982e2210bd6b96818 (20191215094838)
> End: 7e6a4e2214958f95a105a53ee575fde499a0cee2 (20191215214948)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9b7cd94eaf0a2385f53791c982e2210bd6b96818&tochange=7e6a4e2214958f95a105a53ee575fde499a0cee2

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jason Kratzer [:jkratzer] from comment #11)

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200313163649-87ab9a88abce
The bug appears to have been introduced in the following build range:

Those changes probably exposed this by Ion-compiling more scripts, but the underlying bug is older I think.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 9131433 [details]
Bug 1620203 part 1 - Clean up isRecoverableOperand and isObservableSlot. r?nbp!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard. It's not clear this bug is exploitable to begin with.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should be easy. For ESR we can probably land a simpler patch.
• How likely is this patch to cause regressions; how much testing does it need?:

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 9131433 [details]
Bug 1620203 part 1 - Clean up isRecoverableOperand and isObservableSlot. r?nbp!

sec-approval+, dveditz

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9131434 [details]
Bug 1620203 part 2 - Fix some issues with formal arguments. r?nbp!

sec-approval+, dveditz

Comment 17

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/13ae2caaec54f735393d306b35afae3896e5cdb8
https://hg.mozilla.org/integration/autoland/rev/1ad72a9f0c4fa696de9043dfd146f9e021be5fd5

I think this should be on Nightly for a few days before beta uplift. NI myself for that.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/13ae2caaec54
https://hg.mozilla.org/mozilla-central/rev/1ad72a9f0c4f

Comment 19

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200318160552-5bfecf5aff6d.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 9131433 [details]
Bug 1620203 part 1 - Clean up isRecoverableOperand and isObservableSlot. r?nbp!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, maybe security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This has been on Nightly for a few days but patches are not entirely trivial because they reorganize the code a bit to make this easier to fix.
• String changes made/needed:

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Bug 1620203 - Patch for ESR68. r?nbp!

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 9131433 [details]
Bug 1620203 part 1 - Clean up isRecoverableOperand and isObservableSlot. r?nbp!

approved for 75.0b7

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 9134658 [details]
Bug 1620203 - Patch for ESR68. r?nbp!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Maybe crashes or security issues.
• User impact if declined:
• Fix Landed on Version: backporting to 75
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): One-line change to deoptimize functions with a rest-parameter similar to functions that use 'arguments'.
• String or UUID changes made by this patch:

Comment 24

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e94e0ea3893e
https://hg.mozilla.org/releases/mozilla-beta/rev/e830ecd86f3a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9134658 [details]
Bug 1620203 - Patch for ESR68. r?nbp!

Simpler patch for ESR68 to disable the optimization. Approved for 68.7esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/bfb521abfdcd

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35093e2b0047
part 3 - Add tests. r=nbp

Comment 29

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/35093e2b0047