Closed
Bug 1405766
Opened 7 years ago
Closed 7 years ago
Assertion failure: isInt32(), at dist/include/js/Value.h:608 with clone
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: decoder, Assigned: jandem)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update,bisect])
Attachments
(2 files)
1.19 KB,
patch
|
arai
:
review+
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 97efdde466f1 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --disable-oom-functions min.js): var g = newGlobal(); var h = clone(g.Error); assertEq(h()()(), 42); Backtrace: received signal SIGSEGV, Segmentation fault. 0x080f768e in JS::Value::toInt32 (this=0xf5576c00) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/instrumentation/none/type/debug/dist/include/js/Value.h:608 #0 0x080f768e in JS::Value::toInt32 (this=0xf5576c00) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/instrumentation/none/type/debug/dist/include/js/Value.h:608 #1 0x085aa54e in Error (cx=0xf7940800, argc=0, vp=0xf528f068) at js/src/jsexn.cpp:494 #2 0x0818e3b9 in js::CallJSNative (cx=0xf7940800, native=0x85aa200 <Error(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293 [...] #16 main (argc=4, argv=0xffffd8c4, envp=0xffffd8d8) at js/src/shell/js.cpp:8668 eax 0x0 0 ebx 0x8db1ff4 148578292 ecx 0xf7da4864 -136689564 edx 0x0 0 esi 0xffffca2c -13780 edi 0xffffca68 -13720 ebp 0xffffc9c8 4294953416 esp 0xffffc9c0 4294953408 eip 0x80f768e <JS::Value::toInt32() const+62> => 0x80f768e <JS::Value::toInt32() const+62>: movl $0x0,0x0 0x80f7698 <JS::Value::toInt32() const+72>: ud2
Comment 1•7 years ago
|
||
This looks like a general issue when cloning extended native functions: js> new Promise((r) => { clone(r)() }) Assertion failure: fun->isInterpreted(), at /home/andre/git/mozilla-central/js/src/jsfun.h:448 js> (class extends Promise { constructor(e) { clone(e)() } }).all([]) Assertion failure: fun->isInterpreted(), at /home/andre/git/mozilla-central/js/src/jsfun.h:448 js> clone(newGlobal().Proxy.revocable({},{}).revoke)() Assertion failure: isObjectOrNull(), at /home/andre/git/mozilla-central/js/src/build-debug-master/dist/include/js/Value.h:654
Comment 2•7 years ago
|
||
js> clone(newGlobal().eval(`(class {})`))() Assertion failure: toStringEnd >= bufEnd, at /home/andre/git/mozilla-central/js/src/jsscript.cpp:2707
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
There are a number of issues related to cloning native functions. It seems JS::CloneFunctionObject is only called by XBL to clone interpreted functions. If that's true, we could just make it throw for native functions. I added a MOZ_RELEASE_ASSERT(!fun->isNative()) to CloneFunctionObject and it didn't hit on Try (so far). f? bz to confirm the XBL callers of CloneFunctionObject don't pass native functions.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8921474 -
Flags: review?(arai.unmht)
Attachment #8921474 -
Flags: feedback?(bzbarsky)
Comment 5•7 years ago
|
||
When I looked at this, it seemed to me like we should generally disallow cloning of functions with extended slots. This would get rid of most of the explicitly spelled out restrictions.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #5) > When I looked at this, it seemed to me like we should generally disallow > cloning of functions with extended slots. This would get rid of most of the > explicitly spelled out restrictions. We need to be careful with arrow functions, though. Btw there are also issues with native functions *without* extended slots, bug 1407282 for instance where we clone(Debugger) and crash because Debugger::construct does a .prototype lookup to get the Debugger proto... So I want to try disallowing all native functions first.
Comment 7•7 years ago
|
||
Comment on attachment 8921474 [details] [diff] [review] Patch Review of attachment 8921474 [details] [diff] [review]: ----------------------------------------------------------------- Rejecting all native function sounds good. then, if we do so, we should remove native-function specific code from remaining parts. r+ with those parts resolved, or addressed in the followup patch/bug. https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/js/src/jsapi.cpp#3686-3688 > if (!fun->isInterpreted()) > return true; https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/js/src/jsfun.cpp#2138 > return !fun->isInterpreted() || https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/js/src/jsfun.cpp#2206 > clone->initNative(fun->native(), fun->jitInfo()); (and maybe some other, not sure) then, I also agree with evilpie, that we should reject functions with extra slot by default. and if there's any that really needs to be allowed, explicitly allow them there as exceptions (like, fun->isArrow() if arrow function needs to be clone-able), in followup patch or another bug.
Attachment #8921474 -
Flags: review?(arai.unmht) → review+
Comment 8•7 years ago
|
||
In Gecko, JS::CloneFunctionObject is only used on functions that come from JS::CompileFunction or JS::DecodeInterpretedFunction.
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8) > In Gecko, JS::CloneFunctionObject is only used on functions that come from > JS::CompileFunction or JS::DecodeInterpretedFunction. Thanks. So just to be sure, that's feedback+ right?
Comment 11•7 years ago
|
||
Comment on attachment 8921474 [details] [diff] [review] Patch Er, I hadn't looked at the patch, sorry. Yes, feedback+.
Attachment #8921474 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #7) Unfortunately native functions can also be cloned by both the self-hosting code (intrinsics) and by JSOP_LAMBDA for asm.js modules... I can post a follow-up patch to clean up IsFunctionCloneable though.
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8921937 -
Flags: review?(arai.unmht)
Comment 14•7 years ago
|
||
Comment on attachment 8921937 [details] [diff] [review] Simplify IsFunctionCloneable Review of attachment 8921937 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8921937 -
Flags: review?(arai.unmht) → review+
Comment 15•7 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4beb0994d438 Reject all native functions in CloneFunctionObject. r=arai f=bz
Comment 16•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #12) > Unfortunately native functions can also be cloned by both the self-hosting > code (intrinsics) and by JSOP_LAMBDA for asm.js modules Can we add an entirely separate, not-exposed-to-the-world function to copy functions backed by natives, specifically named to reflect that very narrow purpose? Much better to have many different small, narrowly-scoped functions than fewer functions that each do more one-off things.
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4beb0994d438
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #16) > Can we add an entirely separate, not-exposed-to-the-world function to copy > functions backed by natives, specifically named to reflect that very narrow > purpose? Much better to have many different small, narrowly-scoped > functions than fewer functions that each do more one-off things. Yeah, I was considering that yesterday. I'll try something today.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #18) > Yeah, I was considering that yesterday. I'll try something today. Bug 1411954.
Flags: needinfo?(jdemooij)
You need to log in
before you can comment on or make changes to this bug.
Description
•