1405766 - Assertion failure: isInt32(), at dist/include/js/Value.h:608 with clone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

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

[André Bargull [:anba]]

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

[André Bargull [:anba]]

js> clone(newGlobal().eval(`(class {})`))()                               
Assertion failure: toStringEnd >= bufEnd, at /home/andre/git/mozilla-central/js/src/jsscript.cpp:2707

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 5

[Tom S. (please needinfo tschuster)]

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.

Comment 6

[Jan de Mooij [:jandem]]

(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

[Tooru Fujisawa [:arai]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

In Gecko, JS::CloneFunctionObject is only used on functions that come from JS::CompileFunction or JS::DecodeInterpretedFunction.

Comment 9

[Jan de Mooij [:jandem]]

(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

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8921474 [details] [diff] [review]
Patch

Er, I hadn't looked at the patch, sorry.  Yes, feedback+.

Comment 12

[Jan de Mooij [:jandem]]

(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.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Simplify IsFunctionCloneable

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8921937 [details] [diff] [review]
Simplify IsFunctionCloneable

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

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4beb0994d438
Reject all native functions in CloneFunctionObject. r=arai f=bz

Comment 16

[Jeff Walden [:Waldo]]

(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

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

https://hg.mozilla.org/mozilla-central/rev/4beb0994d438

Comment 18

[Jan de Mooij [:jandem]]

(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.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> Yeah, I was considering that yesterday. I'll try something today.

Bug 1411954.