1868992 - Assertion failure: scriptData->nfixed() <= frontend::ParseContext::Scope::FixedSlotLimit, at js/src/vm/JSScript.cpp:2472

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231208-3152110c63b5 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

parseModule(`
  var x = 449627;
  var summary = 'foo';
  if (typeof unescape === 'function') {
    let x00, x01, x02, x03, x04, x05, x06, x07, x08, x09, x0a, x0b, x0c, x0d, x0e, x0f, 
        x10, x11, x12, x13, x14, x15, x16, x17, x18, x19, x1a, x1b, x1c, x1d, x1e, x1f, 
        x20, x21, x22, x23, x24, x25, x26, x27, { get } = ()=>{x41++;y63++;z84++},
        x29, x2a, x2b, x2c, x2d, x2e, x2f, 
        x30, x31, x32, x33, x34, x35, x36, x37, x38, x39, x3a, x3b, x3c, x3d, x3e, x3f, 
        x40, x41, x42, x43, x44, x45, x46, x47, x48, x49, x4a, x4b, x4c, x4d, x4e, x4f, 
        x50, x51, x52, x53, x54, x55, x56, x57, x58, x59, x5a, x5b, x5c, x5d, x5e, x5f, 
        x60, x61, x62, x63, x64, x65, x66, x67, x68, x69, x6a, x6b, x6c, x6d, x6e, x6f, 
        x70, x71, x72, x73, x74, x75, x76, x77, x78, x79, x7a, x7b, x7c, x7d, x7e, x7f, 
        x80, x81, x82, x83, x84, x85, x86, x87, x88, x89, x8a, x8b, x8c, x8d, x8e, x8f, 
        x90, x91, x92, x93, x94, x95, x96, x97, x98, x99, x9a, x9b, x9c, x9d, x9e, x9f, 
        xa0, of , xa2, xa3, xa4, xa5, xa6, xa7, xa8, xa9, xaa, xab, xac, xad, xae, xaf, 
        xb0, xb1, xb2, xb3, xb4, xb5, xb6, xb7, xb8, xb9, xba, xbb, xbc, xbd, xbe, xbf, 
        xc0, xc1, xc2, xc3, xc4, xc5, xc6, xc7, xc8, xc9, xca, xcb, xcc, xcd, xce, xcf, 
        xd0, xd1, xd2, xd3, xd4, xd5, xd6, xd7, xd8, xd9, xda, xdb, xdc, xdd, xde, xdf, 
        xe0, xe1, xe2, xe3, xe4, xe5, xe6, xe7, xe8, xe9, xea, xeb, xec, xed, xee, xef, 
        xf0, xf1, xf2, xf3, xf4, xf5, xf6, xf7, xf8, xf9, xfa, xfb, xfc, xfd, xfe, xff;
  }
  await isOdd();
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555557303e92 in JSScript::fullyInitFromStencil(JSContext*, js::frontend::CompilationAtomCache const&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&, JS::Handle<JSScript*>, js::frontend::TypedIndex<js::frontend::ScriptStencil>) ()
#1  0x0000555557305810 in JSScript::fromStencil(JSContext*, js::frontend::CompilationAtomCache&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&, js::frontend::TypedIndex<js::frontend::ScriptStencil>) ()
#2  0x000055555794b392 in InstantiateTopLevel(JSContext*, js::frontend::CompilationInput&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&) ()
#3  0x0000555557949a4d in js::frontend::CompilationStencil::instantiateStencilAfterPreparation(JSContext*, js::frontend::CompilationInput&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&) ()
#4  0x00005555579488b6 in js::frontend::CompilationStencil::instantiateStencils(JSContext*, js::frontend::CompilationInput&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&) ()
#5  0x000055555789c589 in js::frontend::InstantiateStencils(JSContext*, js::frontend::CompilationInput&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&) ()
#6  0x00005555578def0f in bool ParseModuleToStencilAndMaybeInstantiate<char16_t>(JSContext*, js::FrontendContext*, js::LifoAlloc&, js::frontend::CompilationInput&, js::frontend::ScopeBindingCache*, JS::SourceText<char16_t>&, mozilla::Variant<mozilla::UniquePtr<js::frontend::ExtensibleCompilationStencil, JS::DeletePolicy<js::frontend::ExtensibleCompilationStencil> >, RefPtr<js::frontend::CompilationStencil>, js::frontend::CompilationGCOutput*>&) ()
#7  0x000055555789fc9d in js::frontend::CompileModule(JSContext*, js::FrontendContext*, JS::ReadOnlyCompileOptions const&, JS::SourceText<char16_t>&) ()
#8  0x0000555556ecc892 in ParseModule(JSContext*, unsigned int, JS::Value*) ()
#9  0x000055555706f955 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#20 0x0000555556e966d9 in main ()
rax	0x55555581fc49	93824995163209
rbx	0x7fffffffcb70	140737488341872
rcx	0x5555589a7b28	93825047100200
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb570	140737488336240
rsp	0x7fffffffb4e0	140737488336096
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f92840	140737353689152
r10	0x2	2
r11	0x0	0
r12	0x7fffffffb5a0	140737488336288
r13	0x7fffffffbc40	140737488337984
r14	0x7ffff3d2e100	140737284071680
r15	0x0	0
rip	0x555557303e92 <JSScript::fullyInitFromStencil(JSContext*, js::frontend::CompilationAtomCache const&, js::frontend::CompilationStencil const&, js::frontend::CompilationGCOutput&, JS::Handle<JSScript*>, js::frontend::TypedIndex<js::frontend::ScriptStencil>)+2210>
=> 0x555557303e92 <_ZN8JSScript20fullyInitFromStencilEP9JSContextRKN2js8frontend20CompilationAtomCacheERKNS3_18CompilationStencilERNS3_19CompilationGCOutputEN2JS6HandleIPS_EENS3_10TypedIndexINS3_13ScriptStencilEEE+2210>:	movl   $0x9a8,0x0
   0x555557303e9d <_ZN8JSScript20fullyInitFromStencilEP9JSContextRKN2js8frontend20CompilationAtomCacheERKNS3_18CompilationStencilERNS3_19CompilationGCOutputEN2JS6HandleIPS_EENS3_10TypedIndexINS3_13ScriptStencilEEE+2221>:	callq  0x555556f37c90 <abort>

Marking s-s because the assert looks like a potential oob issue.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1868992 using build mozilla-central 20231208094241-3152110c63b5. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 4

[Tooru Fujisawa [:arai]]

This is a bad interaction between the module's behavior around top-level await and the slot optimization.

At the beginning, the module's context is marked as "sync", and it's flipped to "async" on demand for await or for-await-of.

https://searchfox.org/mozilla-central/rev/23e7e940337d0e0b29aabe0080e4992d3860c940/js/src/frontend/Parser.cpp#9013-9016,9021

// If we encounter an await in a module, and the module is not marked
// as async, mark the module as async.
if (tt == TokenKind::Await && !pc_->isAsync()) {
  if (pc_->atModuleTopLevel()) {
...
    pc_->sc()->asModuleContext()->setIsAsync();

Then, the slot optimization uses the "async" flag for whether to optimize the slot, in propagateFreeNamesAndMarkClosedOverBindings.
if "async", the slots needs to be copied on suspend, and we have a limit on the number of slots.

If there are too many bindings, all of them are moved to the environment object, so that they don't have to be copied on suspend.

https://searchfox.org/mozilla-central/rev/23e7e940337d0e0b29aabe0080e4992d3860c940/js/src/frontend/Parser.cpp#976-977

if (pc_->isGeneratorOrAsync()) {
  scope.setOwnStackSlotCount(slotCount);

Then, propagateFreeNamesAndMarkClosedOverBindings is operated for each scope, and if the scope ends before the first await,
the calculation is done with "sync", which doesn't have a limit on the number of slots.

then, to my understanding, the limit is just a threshold comes from the balance between "the cost of environment object allocation" vs "the cost of copy on suspend/resume",
and as long as the other place doesn't expect the limit for some other optimization, it's safe.
The actual threshold value 256 is used only in the frontend (other than the assertion), and increasing the value doesn't seem to break anything.
I'll check the details next week.

Also, given the information calculated with "sync" is for nested blocks before await, and the effect is to make the variable not-closed-over-by-default.
If the variable can be accessed after await, it already means the variable is closed over with other reason (such as, actually closed over by inner function),
and the effect isn't observed for other not-actually-closed-over bindings.

anyway, let's keep this hidden until I check the remaining edge cases.

Comment 5

[Donal Meehan [:dmeehan]]

:arai could you set the correct regressor on this? If you found it during your investigation

Comment 6

[Tooru Fujisawa [:arai]]

the underlying code change is from bug 1519100 (firefox 85) and enabled in bug 1681046 (firefox 89),
so all supported versions are affected, but this likely doesn't hit anything on non-debug builds.
I will continue investigating the edge cases.

Comment 7

[Tooru Fujisawa [:arai]]

Confirmed that this just results in an unnecessary allocation for the stack storage array of the generator object.

The details below:

the motivation for the limitation

The limitation comes from bug 1412202.

Before that, all local variables are stored in an environment object, but it results in inefficient execution.
And the patch there makes it possible to store locals in frame slots in addition with the regular stack values.
It has the limitation because of the cost of copy.
If there are many locals, the cost of copy becomes problematic because it happens for each suspend/resume,
and the "optimization" becomes less effective.
So that the optimization is applied only to the case where there's not so much locals to copy.

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/frontend/ParseContext.h#194-203

// Maximum number of fixed stack slots in a generator or async function
// script. If a script would have more, we instead store some variables in
// heap EnvironmentObjects.
//
// This limit is a performance heuristic. Stack slots reduce allocations,
// and `Local` opcodes are a bit faster than `AliasedVar` ones; but at each
// `yield` or `await` the stack slots must be memcpy'd into a
// GeneratorObject. At some point the memcpy is too much. The limit is
// plenty for typical human-authored code.
static constexpr uint32_t FixedSlotLimit = 256;

the nfixed value and the stack storage array

The asserted nfixed value is BytecodeEmitter::maxFixedSlots in the following.
The value of BytecodeEmitter::maxFixedSlots is used also for the nslots below.
If the nfixed value exceeds the frontend::ParseContext::Scope::FixedSlotLimit value, it means the nslots also exceeds the value, with the max stack depth.

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/frontend/BytecodeEmitter.cpp#2472-2477,2490-2491

js::UniquePtr<ImmutableScriptData>
BytecodeEmitter::createImmutableScriptData() {
  uint32_t nslots;
  if (!getNslots(&nslots)) {
    return nullptr;
  }
...
  return ImmutableScriptData::new_(
      fc, mainOffset(), maxFixedSlots, nslots, bodyScopeIndex,

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/frontend/BytecodeEmitter.cpp#2499-2501,2506

bool BytecodeEmitter::getNslots(uint32_t* nslots) {
  uint64_t nslots64 =
      maxFixedSlots + static_cast<uint64_t>(bytecodeSection().maxStackDepth());
...
  *nslots = nslots64;

In term of generator/async context, the nslots value is used for allocating the "stack storage" array as a space to save the stack (frame slot and local stack values).

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/vm/GeneratorObject.cpp#26-28,47,52

AbstractGeneratorObject* AbstractGeneratorObject::create(
    JSContext* cx, HandleFunction callee, HandleScript script,
    HandleObject environmentChain, Handle<ArgumentsObject*> argsObject) {
...
  ArrayObject* stack = NewDenseFullyAllocatedArray(cx, script->nslots());
...
  genObj->setStackStorage(*stack);

the array's content is initialized by the stack values when suspending the generator frame:

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/vm/Stack-inl.h#188-190

inline bool InterpreterFrame::saveGeneratorSlots(JSContext* cx, unsigned nslots,
                                                 ArrayObject* dest) const {
  return dest->initDenseElementsFromRange(cx, slots(), slots() + nslots);

and the copied back to the stack when resuming the generator frame:

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/vm/GeneratorObject.cpp#267-270,284-290

bool AbstractGeneratorObject::resume(JSContext* cx,
                                     InterpreterActivation& activation,
                                     Handle<AbstractGeneratorObject*> genObj,
                                     HandleValue arg, HandleValue resumeKind) {
...
  if (genObj->hasStackStorage() && !genObj->isStackStorageEmpty()) {
    JSScript* script = activation.regs().fp()->script();
    ArrayObject* storage = &genObj->stackStorage();
    uint32_t len = storage->getDenseInitializedLength();
    activation.regs().fp()->restoreGeneratorSlots(storage);
    activation.regs().sp += len - script->nfixed();
    storage->setDenseInitializedLength(0);

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/vm/Stack-inl.h#193,197-198

inline void InterpreterFrame::restoreGeneratorSlots(ArrayObject* src) {
...
  const Value* srcElements = src->getDenseElements();
  mozilla::PodCopy(slots(), srcElements, src->length());

same for Baseline JIT case:

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/jit/BaselineFrame-inl.h#44-45,52

inline bool BaselineFrame::saveGeneratorSlots(JSContext* cx, unsigned nslots,
                                              ArrayObject* dest) const {
...
  return dest->initDenseElementsFromRange(cx, span.rbegin(), span.rend());

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/jit/BaselineCodeGen.cpp#5809-5810,5971-5995

template <typename Handler>
bool BaselineCodeGen<Handler>::emit_Resume() {
...
  Address stackStorageSlot(genObj,
                           AbstractGeneratorObject::offsetOfStackStorageSlot());
  masm.fallibleUnboxObject(stackStorageSlot, scratch2, &noStackStorage);
  {
    Register initLength = regs.takeAny();
    masm.loadPtr(Address(scratch2, NativeObject::offsetOfElements()), scratch2);
    masm.load32(Address(scratch2, ObjectElements::offsetOfInitializedLength()),
                initLength);
    masm.store32(
        Imm32(0),
        Address(scratch2, ObjectElements::offsetOfInitializedLength()));

    Label loop, loopDone;
    masm.branchTest32(Assembler::Zero, initLength, initLength, &loopDone);
    masm.bind(&loop);
    {
      masm.pushValue(Address(scratch2, 0));
      masm.guardedCallPreBarrierAnyZone(Address(scratch2, 0), MIRType::Value,
                                        scratch1);
      masm.addPtr(Imm32(sizeof(Value)), scratch2);
      masm.branchSub32(Assembler::NonZero, Imm32(1), initLength, &loop);
    }
    masm.bind(&loopDone);
    regs.add(initLength);
  }

Then, the actual number of copied elements are the stach depth at the point of await operation.
So, the local slots in inner lexical scope before await doesn't affect the number, and all local slots outside of those lexical scope gets smaller index for the local slot.
thus, the larger nfixed there just affects the size of the stack storage array,
and just consumes the memory unnecessarily.

For other consumers of nfixed and nslots, the value is correct,

For the stack storage size, the value is used as "the worst case"'s size, and the array won't be fully used unless yield/await happens in the deepest stack,
and it applies also to regular generator and async functions.

frame slots vs environment slots

For local variables befor the first await, using the frame slots is totally fine, as long as the variable isn't closed over.
If the variable is closed over, the flag is correctly set, and an environment object is created for the scope, and the variable is stored in it.

This is true also for locals with slot which exceeds frontend::ParseContext::Scope::FixedSlotLimit.

For local variables after the first await, the frame slot is used only when there are less than frontend::ParseContext::Scope::FixedSlotLimit number.
and once it exceeds, all variables are marked closed over and moved to environment objects.
This is the expected behavior and also the same behavior as generator and async functions.

the problem

As noted above, the scriptData->nfixed() value below represents the maximum number of fixed slots throughout the entire execution of the function (module top-level script here),
and that's the "worst case"'s value of the number of values needs to be copied to the stack storage.

So, the assertion and the restriction is already too strict.

For generator and async functions, the assertion still passes, because the limitation is applied regardless of whether there's yield or await with the maximum number of slots used.

https://searchfox.org/mozilla-central/rev/da48f565f70a57ac28862090828fbaa7fd8556f6/js/src/vm/JSScript.cpp#2470-2472

MOZ_ASSERT_IF(
    script->isGenerator() || script->isAsync(),
    scriptData->nfixed() <= frontend::ParseContext::Scope::FixedSlotLimit);

For module's case, it becomes problematic, because the limitation is applied only after the first await appears.
(otherwise the module script is not marked "async")

So, this is only about the assertion, which is too strict and can hit false positive on the module's case.

possible solution

Possible solution here would be either:

• (a) track the actual number of stack slots of each yield and await, instead of the maximum number of stack slots, and use the value for the limitation and the assertion
• (b) for module, separately handle the maximum stack slots after the first await, and use the value for the assertion

(a) is more complete solution, which can also improve the generator/async function execution with machine-generated code (which can contain many local variables), but it's more difficult task.

(b) is simpler option to workaround the assertion itself, while still asserting that the limitation is properly applied for code after the first await

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Tooru Fujisawa [:arai] from comment #7)

Confirmed that this just results in an unnecessary allocation for the stack storage array

Thanks for looking into this. Does this mean that it isn't a security issue and I can unhide the bug? Or do you want to investigate further?

Comment 9

[Tooru Fujisawa [:arai]]

This isn't security issue and can be opened up.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

:yulia, since you are the author of the regressor, bug 1519100, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 11

[Tooru Fujisawa [:arai]]

On second thought, apparently the assertion doesn't much verify something useful for runtime.
The size of the stack storage array can be long if there are many stack values, and the length isn't actually limited with the FixedSlotLimit.
So the assertion should be moved to frontend, with better context, or maybe just assert that the limitation is applied at necessary point, instead of asserting the number of slots.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1868992 - Part 1: Remove duplicate and unused constant. r=yulia

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1868992 - Part 2: Use union with better types to track the inner scope slots in generator and async. r=yulia!

Depends on D198297

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1868992 - Part 3: Move the assertion about fixed slot to BytecodeEmitter to verify the condition only when emitting yield/await. r=yulia

Depends on D198298

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1868992 - Part 4: Add test. r=yulia

Depends on D198299

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a4874fca22b
Part 1: Remove duplicate and unused constant. r=yulia
https://hg.mozilla.org/integration/autoland/rev/7f45ff51f90d
Part 2: Use union with better types to track the inner scope slots in generator and async. r=yulia
https://hg.mozilla.org/integration/autoland/rev/156e6a5b8caa
Part 3: Move the assertion about fixed slot to BytecodeEmitter to verify the condition only when emitting yield/await. r=yulia
https://hg.mozilla.org/integration/autoland/rev/8d1104da2824
Part 4: Add test. r=yulia

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8a4874fca22b
https://hg.mozilla.org/mozilla-central/rev/7f45ff51f90d
https://hg.mozilla.org/mozilla-central/rev/156e6a5b8caa
https://hg.mozilla.org/mozilla-central/rev/8d1104da2824