Closed Bug 1417962 Opened 2 years ago Closed 2 years ago

Crash [@ js::ScriptedProxyHandler::boxedValue_unbox] or Hit MOZ_CRASH(Should not end up in ScriptedProxyHandler::boxedValue_unbox) at js/src/proxy/ScriptedProxyHandler.cpp:1284

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 - fixed
firefox59 - fixed

People

(Reporter: decoder, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision fc194660762d+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

let hook = {}
let Base = function() {}
Base.prototype = new Proxy(Base.prototype, hook);
class Derived extends Base {
    testPrimitiveReceiver() {
        super.foo = "Derived";
    }
}
Derived.prototype.testPrimitiveReceiver.call(null);


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::ScriptedProxyHandler::boxedValue_unbox (this=0x1b43ff0 <js::ScriptedProxyHandler::singleton>, cx=0x7ffff5f15000, proxy=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1284
#0  js::ScriptedProxyHandler::boxedValue_unbox (this=0x1b43ff0 <js::ScriptedProxyHandler::singleton>, cx=0x7ffff5f15000, proxy=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1284
#1  0x000000000081205b in JS::ObjectOpResult::reportStrictErrorOrWarning (this=this@entry=0x7fffffffc6a0, cx=cx@entry=0x7ffff5f15000, obj=..., id=..., strict=strict@entry=true) at js/src/jsapi.cpp:183
#2  0x000000000050dfa2 in JS::ObjectOpResult::checkStrictErrorOrWarning (strict=true, id=..., obj=..., cx=0x7ffff5f15000, this=0x7fffffffc6a0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/opt/dist/include/js/Class.h:204
#3  js::SetPropertySuper (cx=0x7ffff5f15000, obj=..., receiver=..., name=..., rval=..., strict=<optimized out>) at js/src/vm/Interpreter.cpp:5276
#4  0x00000000005141c0 in Interpret (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:2916
#5  0x000000000051f90a in js::RunScript (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:423
#6  0x000000000051fe5b in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f15000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:495
#7  0x00000000005200b5 in InternalCall (cx=cx@entry=0x7ffff5f15000, args=...) at js/src/vm/Interpreter.cpp:522
#8  0x0000000000520118 in js::Call (cx=cx@entry=0x7ffff5f15000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:541
#9  0x000000000085aeb2 in js::fun_call (cx=0x7ffff5f15000, argc=1, vp=0x7ffff41b3098) at js/src/jsfun.cpp:1237
#10 0x000000000051fc94 in js::CallJSNative (args=..., native=0x85ad60 <js::fun_call(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f15000) at js/src/jscntxtinlines.h:291
[...]
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8987
rax	0x1b6b880	28752000
rbx	0x7ffff5f15000	140737319620608
rcx	0x7fffffffc650	140737488340560
rdx	0xe344d0	14894288
rsi	0x7ffff5f15000	140737319620608
rdi	0x1b43ff0	28590064
rbp	0x0	0
rsp	0x7fffffffc5d8	140737488340440
r8	0x2b	43
r9	0x20	32
r10	0x201	513
r11	0x7ffff6b5edc0	140737332506048
r12	0x7fffffffc6a0	140737488340640
r13	0x7fffffffc650	140737488340560
r14	0x7ffff5f15060	140737319620704
r15	0x7ffff5f49080	140737319833728
rip	0x8c82a1 <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+17>
=> 0x8c82a1 <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+17>:	movl   $0x0,0x0
   0x8c82ac <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+28>:	ud2
Blocks: 1241966
Assignee: nobody → evilpies
Priority: -- → P1
I didn't think proxies could actually appear in this case somehow. I think we can just not unbox proxies at all.
Attachment #8929852 - Flags: review?(jdemooij)
Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

Review of attachment 8929852 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, yeah.
Attachment #8929852 - Flags: review?(jdemooij) → review+
Is this really a proxy issue or actually an issue that the receiver can be different from |obj| in super-property accesses? For example the following test case is probably still wrong, i.e. it shouldn't unbox the Date object.

---
let Base = function() {}
Base.prototype = new Date("2017-11-16T00:00:00.000Z");

class Derived extends Base {
    testPrimitiveReceiver() {
        super.foo = "Derived";
    }
}

// TypeError: can't assign to property "foo" on 1510790400000: not an object
Derived.prototype.testPrimitiveReceiver.call(null); 
---
I guess you are right, but I am not sure how to fix this. We would probably have to carry around the value instead of the object.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ace8b5531c
Don't unbox proxies when reporting strict assignment error. r=jandem
I landed this for now, but we need to figure out a proper fix. I don't think we know that we are handling a super property lookup when throwing this exception.
https://hg.mozilla.org/mozilla-central/rev/e6ace8b5531c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi Tom,
Is this worth uplifting to 58?
Flags: needinfo?(evilpies)
Yes, let's do that.
Flags: needinfo?(evilpies)
Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

Approval Request Comment
[Feature/Bug causing the regression]: bug 1241966
[User impact if declined]: Potential crash
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just disables code for proxies, which gives us the previous, non crashing behavior
[String changes made/needed]: None
Attachment #8929852 - Flags: approval-mozilla-beta?
Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

Fix a potential crash. Beta58+.
Attachment #8929852 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Tom Schuster [:evilpie] from comment #10)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]:  No

Has automated coverage, does not need manual testing, per Tom.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.