1488385 - Assertion failure: !callerFun->isBuiltin() (non-builtin iterator returned a builtin?), at js/src/vm/JSFunction.cpp:319

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2a4cf603095a (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe):

function f() {
    return f.caller.p;
}
a = new class extends f {}


Backtrace:

received signal SIGSEGV, Segmentation fault
#0  0x088340e9 in CallerGetterImpl (cx=<optimized out>, args=...) at js/src/vm/JSFunction.cpp:319
#1  0x088341e5 in JS::CallNonGenericMethod<&(IsFunction(JS::Handle<JS::Value>)), &(CallerGetterImpl(JSContext*, JS::CallArgs const&))> (args=..., cx=0xf6e1b800) at dist/include/js/CallNonGenericMethod.h:100
#2  CallerGetter (cx=0xf6e1b800, argc=0, vp=0xffffc5c0) at js/src/vm/JSFunction.cpp:335
#3  0x08226caa in CallJSNative (cx=0xf6e1b800, native=0x8834150 <CallerGetter(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:449
#4  0x0821aa0d in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:537
#5  0x0821afb0 in InternalCall (cx=cx@entry=0xf6e1b800, args=...) at js/src/vm/Interpreter.cpp:588
#6  0x0821b16a in js::Call (cx=0xf6e1b800, fval=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:607
#7  0x0821b2c3 in js::CallGetter (cx=0xf6e1b800, thisv=..., getter=..., rval=...) at js/src/vm/Interpreter.cpp:727
#8  0x08868a54 in CallGetter (vp=..., shape=..., receiver=..., obj=..., cx=<optimized out>) at js/src/vm/NativeObject.cpp:2122
#9  GetExistingProperty<(js::AllowGC)1> (cx=<optimized out>, receiver=..., obj=..., shape=..., vp=...) at js/src/vm/NativeObject.cpp:2179
#10 0x0886ece7 in NativeGetPropertyInline<(js::AllowGC)1> (cx=<optimized out>, obj=..., receiver=..., id=..., nameLookup=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:2388
#11 0x0886f127 in js::NativeGetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:2424
#12 0x0821e08d in js::GetProperty (cx=0xf6e1b800, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.h:1705
#13 0x08209852 in js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=<optimized out>) at js/src/vm/JSObject.h:784
#14 js::GetProperty (cx=<optimized out>, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4644
#15 0x0820db8d in GetPropertyOperation (vp=..., lval=..., pc=<optimized out>, script=..., fp=<optimized out>, cx=0xf6e1b800) at js/src/vm/Interpreter.cpp:220
#16 Interpret (cx=0xf6e1b800, state=...) at js/src/vm/Interpreter.cpp:2983
#17 0x0821a3b1 in js::RunScript (cx=0xf6e1b800, state=...) at js/src/vm/Interpreter.cpp:429
#18 0x0821ac3b in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::CONSTRUCT) at js/src/vm/Interpreter.cpp:561
#19 0x0821bdbe in InternalConstruct (cx=<optimized out>, cx@entry=0xf6e1b800, args=...) at js/src/vm/Interpreter.cpp:636
#20 0x0821bfd5 in js::Construct (cx=0xf6e1b800, fval=..., args=..., newTarget=..., objp=...) at js/src/vm/Interpreter.cpp:690
#21 0x0821c895 in js::SpreadCallOperation (cx=0xf6e1b800, script=..., pc=0xf59d6634 "\246V", thisv=..., callee=..., arr=..., newTarget=..., res=...) at js/src/vm/Interpreter.cpp:5130
#22 0x0820d4c2 in Interpret (cx=0xf6e1b800, state=...) at js/src/vm/Interpreter.cpp:3216
[...]
#33 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9973
eax	0x0	0
ebx	0xf5c71ca0	-171500384
ecx	0xf7d92864	-136763292
edx	0x0	0
esi	0xffffc1a8	-15960
edi	0x8eb5ff4	149643252
ebp	0xffffc3a8	4294951848
esp	0xffffc160	4294951264
eip	0x88340e9 <CallerGetterImpl(JSContext*, JS::CallArgs const&)+1177>
=> 0x88340e9 <CallerGetterImpl(JSContext*, JS::CallArgs const&)+1177>:	movl   $0x0,0x0
   0x88340f3 <CallerGetterImpl(JSContext*, JS::CallArgs const&)+1187>:	ud2


Marking this s-s because I don't know if the confusion between builtin vs. non-builtin iterator is in any way security relevant (e.g. if the builtin operator exposes something it shouldn't expose).

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/714f1a873154
user:        Jim Blandy
date:        Sat Jul 28 20:26:46 2018 -0700
summary:     Bug 1473272 - Don't treat classes with default constructors as self-hosted code. r=tcampbell

This iteration took 293.141 seconds to run.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jim, is bug 1473272 a likely regressor?

Comment 3

[Ted Campbell [:tcampbell]]

I'll take a look. This is definitely related to the patch in Comment 1. My instinct is that this is a false positive, but since we were supposed to have looked over all these cases in the original work I should give it another look.

Comment 4

[Andrew McCreight [:mccr8]]

The assertion sounds scary, so I'm going to mark this sec-high. If it turns out to be a false positive, feel free to unhide the bug and remove the sec rating.

Comment 5

[Jim Blandy :jimb]

I don't think this is security-sensitive.

We create default constructor functions for declarations like `class f {}` by cloning self-hosted functions, and then fixing up their source positions and clearing the self-hosted bit on the JSScript[1]. But I didn't realize there was a self-hosted flag on the JSFunction as well, so my patch in bug 1473272 didn't clear it.

The assertion in question is simply checking that we don't reveal self-hosted functions via the function `caller` accessor. But the function that would be revealed, if the assertion didn't trigger, is just a default constructor, not really a self-hosted function.

The bug is that NonBuiltinScriptFrameIter::settle is checking the bit on the script[2] whereas the assertion is checking the bit on the JSFunction[3].

[1]: https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/vm/JSScript.cpp#1095
[2]: https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/vm/Stack.cpp#1589
[3]: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/js/src/vm/JSFunction.cpp#332

Comment 6

[Jim Blandy :jimb]

Needinfo'ing continuation, to re-evaluate security impact

Comment 7

[Jim Blandy :jimb]

Stealing from Ted.

Comment 8

[Andrew McCreight [:mccr8]]

unhiding per comment 5.

Comment 9

[Jim Blandy :jimb]

Attachment: Bug 1488385: Remove JSFunction::infallibleIsDefaultClassConstructor and its uses. r?tcampbell

This function is used only by js::MakeDefaultConstructor, which uses it to
assert that the self-hosted function clone it just created is a default class
constructor. This assertion is not very valuable:

- The function is always a self-hosted builtin, since we created it by calling
  createLazySelfHostedFunctionClone a few lines earlier.

- In the self-hosted builtin case, infallibleIsDefaultClassConstructor can't
  consult the script's isDefaultClassConstructor predicate, so it just checks
  the function's LAZY_FUNCTION_NAME_SLOT, verifying that it is one of the two
  names supplied in MakeDefaultConstructor, also a few lines above the call.

- It then asserts that, if the function is a default class constructor, its
  `isConstructor` and `isClassConstructor` predicates agree. This is also
  checked in MakeDefaultConstructor.

So the checks are superfluous.

In the next patch, we want to clear the SELF_HOSTED flag on default
constructors, since they should appear in all respects as if they originated
with the class declaration, in user code. Having a predicate like
infallibleIsDefaultClassConstructor around insisting that they must be
self-hosted builtins would be confusing.

Comment 10

[Jim Blandy :jimb]

Attachment: Bug 1488385: Default class constructors are no longer self-hosted once they're cloned. r?tcampbell

When we clone the script of a default class constructor (derived or base), we
need to clear its SELF_HOSTED flag, since such functions are supposed to behave
just like ordinary functions from user code: their frames should appear on the
stack, and so on. However, the SELF_HOSTED flag must remain set until the script
is actually cloned from the original, to satisfy the (quite reasonable)
assertion that only SELF_HOSTED functions get their scripts cloned from the
self-hosted compartment.

Depends on D8620

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2593fa8b1bd
Remove JSFunction::infallibleIsDefaultClassConstructor and its uses. r=tcampbell

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c317490b6e78
Default class constructors are no longer self-hosted once they're cloned. r=tcampbell

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b2593fa8b1bd
https://hg.mozilla.org/mozilla-central/rev/c317490b6e78