Crash [@ js::GetOwnPropertyDescriptor] with private field and Proxy
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox81 | --- | affected |
People
(Reporter: decoder, Unassigned)
Details
(Keywords: crash, regression, testcase, Whiteboard: [bugmon:update,bisected,confirmed])
Crash Data
Attachments
(2 files)
The following testcase crashes on mozilla-central revision 20200727-932240e49142 (opt build, run with --fuzzing-safe --no-threads --enable-private-fields):
class A83 {
#z = () => {}
static chainTest(o25) {
o25?.#z().#z()?.#z();
}
};
b39 = new Proxy({}, {});
A83.chainTest(b39);
Backtrace:
received signal SIGSEGV, Segmentation fault.
0x5689b339 in js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>) ()
#0 0x5689b339 in js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>) ()
#1 0x5677f81a in js::BaseProxyHandler::getPrivate(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const ()
#2 0x567a1777 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) ()
#3 0x566db427 in Interpret(JSContext*, js::RunState&) ()
[...]
#12 0x565cd3f4 in main ()
eax 0x0 0
ebx 0x585a1000 1482297344
ecx 0xffffab8c -21620
edx 0xffffab88 -21624
esi 0xffffabb8 -21576
edi 0xffffabb8 -21576
ebp 0xffffab78 4294945656
esp 0xffffab30 4294945584
eip 0x5689b339 <js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>)+41>
=> 0x5689b339 <_ZN2js24GetOwnPropertyDescriptorEP9JSContextN2JS6HandleIP8JSObjectEENS3_INS2_11PropertyKeyEEENS2_13MutableHandleINS2_18PropertyDescriptorEEE+41>: mov (%eax),%eax
0x5689b33b <_ZN2js24GetOwnPropertyDescriptorEP9JSContextN2JS6HandleIP8JSObjectEENS3_INS2_11PropertyKeyEEENS2_13MutableHandleINS2_18PropertyDescriptorEEE+43>: mov (%eax),%eax
This crash is really weird as it only reproduces on a 32-bit non-debug optimized build. If I try anything else (all 64-bit builds as well as the 32-bit debug build), I get TypeError: Trying to read undeclared field
.
Reporter | ||
Comment 1•4 years ago
|
||
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200727203201-932240e49142.
The bug appears to have been introduced in the following build range:
> Start: 2f2cb7c9bcced2ec48d8fba6a369af03d1b46f64 (20200720214226)
> End: f0ed5585b6adac269e39dc3241fba9c4f52aa4c4 (20200721032334)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f2cb7c9bcced2ec48d8fba6a369af03d1b46f64&tochange=f0ed5585b6adac269e39dc3241fba9c4f52aa4c4
Comment 3•4 years ago
•
|
||
Great test case. I'm deeply confused by the platform requirement. Here's my initial diagnosis though (I'm still going to try to figure out the platform dependence issue). I believe this is fixed by Bug 1653567, which changed the bytecode emission strategy for private accesses, and blocks this particular issue. Looking at the Bytecode, dumped directly from a crash: ``` $1 = {<js::RootedBase<JSScript*, JS::Rooted<JSScript*> >> = {<js::MutableWrappedPtrOperations<JSScript*, JS::Rooted<JSScript*> >> = {<js::WrappedPtrOperations<JSScript*, JS::Rooted<JSScript*> >> = {<No data fields>}, <No data fields>}, <No data fields>}, stack = 0xf6b0a014, prev = 0xff891acc, ptr = 0xf69740b0} (rr) call js::DumpScript(cx, script.ptr) loc line op ----- ---- -- main: 00000: 4 GetArg 0 # 00003: 4 Dup # 00004: 4 Undefined # 00005: 4 StrictNe # 00006: 4 And 20 (+14) # 00011: 4 JumpTarget (ic: 4) # 00016: 4 Pop # 00017: 4 Dup # 00018: 4 Null # 00019: 4 StrictNe # 00020: 4 JumpTarget (ic: 5) # 00025: 4 Not # 00026: 4 IfEq 41 (+15) # 00031: 4 JumpTarget (ic: 7) # 00036: 4 Goto 132 (+96) # 00041: 4 JumpTarget (ic: 7) # 00046: 4 Dup # 00047: 4 GetAliasedVar "#z" (hops = 0, slot = 2) # 00052: 4 CheckAliasedLexical "#z" (hops = 0, slot = 2) # 00057: 4 CallElem # <snip> ``` You can see we're using CallElem; it's worth noting that I had only ever implemented special case code for private fields for Init/Get and Set. I'd *not* implemented special support for `CallElem`. This means that the first `CallElem` may freely try to read `#z` off the proxy, without there having been any check for `#z` existing: that checking would have happened as part of `{Init,Get,Set}PrivateElem`. Obviously this is insufficient. We ended up aborting inside of `getPrivate` where we asserted `expando` already existed (as would have been invariant should we only ever taken `Init,Get,Set` paths. Now. I still don't have a good answer for why this failure is dependent on architecture. This seems like a clear case that was missed and should crash everywhere. I need to dig deeper still to see if I can't answer that question. This is fixed by Bug 1653567, which changed the bytecode emission strategy to instead separate the check into a separate operation (`CheckPrivateField`). The relevant bytecode for chainTest becomes: ``` 00046: Dup # o25 o25 00047: GetAliasedVar "#z" (hops = 0, slot = 2) # o25 o25 #z 00052: CheckAliasedLexical "#z" (hops = 0, slot = 2) # o25 o25 #z 00057: CheckPrivateField 1 5 # o25 <unknown> <unknown> <unknown> 00060: Pop # o25 <unknown> <unknown> 00061: CallElem # o25 <unknown>[<unknown>] ``` So we first check and throw, before doing the callelem. Which semantically is how things ought to work.
Comment 4•4 years ago
|
||
(The above comment got splinched as the result of a bugzilla bug methinks)
Comment 5•4 years ago
|
||
If you gently modify the test case, you can see other peculiarities of the old implementation:
class A {
#z = () => {};
static chainTest(o) {
o?.#z().#z()?.#z();
}
};
var obj = {};
A.chainTest(obj);
you get an error messages that say o[#z]
is not a function.
Still. This is fixed by the new implementation.
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Bugmon Analysis: Bug marked as FIXED but still reproduces on mozilla-central 20200727203201-932240e49142.
Comment 7•4 years ago
|
||
For some reason bugmon is trying an old version of central?
Reporter | ||
Comment 8•4 years ago
|
||
Yes, it's because linux opt builds were removed and we are ony left with shippable builds on mozilla-central. We are working on it.
Comment 9•4 years ago
|
||
We should have a fix in place later today. If you want to close this sooner, you can go ahead and remove the bugmon
keyword to prevent it from being scanned again.
Comment 10•4 years ago
|
||
We should mark this bug as a duplicate of Bug 1653567 based on comment 3.
Description
•