Assertion failure: scriptData->nfixed() <= frontend::ParseContext::Scope::FixedSlotLimit, at js/src/vm/JSScript.cpp:2472
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
People
(Reporter: decoder, Assigned: arai)
References
(Regression)
Details
(Keywords: assertion, regression, testcase, Whiteboard: [bugmon:update,bisect])
Attachments
(6 files)
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.
Reporter | ||
Comment 1•1 year ago
|
||
Reporter | ||
Comment 2•1 year ago
|
||
Comment 3•1 year ago
|
||
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.
Assignee | ||
Comment 4•1 year ago
|
||
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
.
// 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.
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•1 year ago
|
||
:arai could you set the correct regressor on this? If you found it during your investigation
Assignee | ||
Comment 6•1 year ago
|
||
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.
Assignee | ||
Comment 7•1 year ago
|
||
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.
// 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.
js::UniquePtr<ImmutableScriptData>
BytecodeEmitter::createImmutableScriptData() {
uint32_t nslots;
if (!getNslots(&nslots)) {
return nullptr;
}
...
return ImmutableScriptData::new_(
fc, mainOffset(), maxFixedSlots, nslots, bodyScopeIndex,
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).
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:
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:
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);
inline void InterpreterFrame::restoreGeneratorSlots(ArrayObject* src) {
...
const Value* srcElements = src->getDenseElements();
mozilla::PodCopy(slots(), srcElements, src->length());
same for Baseline JIT case:
inline bool BaselineFrame::saveGeneratorSlots(JSContext* cx, unsigned nslots,
ArrayObject* dest) const {
...
return dest->initDenseElementsFromRange(cx, span.rbegin(), span.rend());
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.
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
andawait
, 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
Updated•1 year ago
|
Comment 8•1 year ago
|
||
(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?
Assignee | ||
Comment 9•1 year ago
|
||
This isn't security issue and can be opened up.
Updated•1 year ago
|
Comment 10•1 year ago
|
||
: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.
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 11•1 year ago
|
||
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.
Assignee | ||
Comment 12•1 year ago
|
||
Updated•1 year ago
|
Assignee | ||
Comment 13•1 year ago
|
||
Depends on D198297
Assignee | ||
Comment 14•1 year ago
|
||
Depends on D198298
Assignee | ||
Comment 15•1 year ago
|
||
Depends on D198299
Updated•1 year ago
|
Comment 16•1 year ago
|
||
Comment 17•1 year ago
|
||
bugherder |
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
Updated•1 year ago
|
Updated•9 months ago
|
Description
•