1764737 - Crash [@ js::ModuleBuilder::processExportFrom(js::frontend::BinaryNode*)] with OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20220414-6c55ba30c858 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

function r(src) {
  oomTest(function() {
      parseModule(src);
  });
}
r("export * from 'y';");
r("export * from 'y';");

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555556d86182 in js::ModuleBuilder::processExportFrom(js::frontend::BinaryNode*) ()
#0  0x0000555556d86182 in js::ModuleBuilder::processExportFrom(js::frontend::BinaryNode*) ()
#1  0x00005555573f92d3 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::exportFrom(unsigned int, js::frontend::ParseNode*) ()
#2  0x00005555573f96b7 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::exportBatch(unsigned int) ()
#3  0x00005555573f1590 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::exportDeclaration() ()
#4  0x00005555573ebbef in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem(js::frontend::YieldHandling, bool) ()
#5  0x00005555573e9885 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList(js::frontend::YieldHandling) ()
#6  0x00005555574212de in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::moduleBody(js::frontend::ModuleSharedContext*) ()
#7  0x000055555746e2c6 in ModuleCompiler<char16_t>::compile(JSContext*) ()
#8  0x000055555746dd93 in bool ParseModuleToStencilAndMaybeInstantiate<char16_t>(JSContext*, js::frontend::CompilationInput&, JS::SourceText<char16_t>&, mozilla::Variant<mozilla::UniquePtr<js::frontend::ExtensibleCompilationStencil, JS::DeletePolicy<js::frontend::ExtensibleCompilationStencil> >, RefPtr<js::frontend::CompilationStencil>, js::frontend::CompilationGCOutput*>&) ()
#9  0x0000555557437aa9 in js::frontend::CompileModule(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceText<char16_t>&) ()
#10 0x0000555556b690fe in ParseModule(JSContext*, unsigned int, JS::Value*) ()
#11 0x00003743e66c80df in ?? ()
#12 0x0000000000000008 in ?? ()
#13 0x00007fffffffb630 in ?? ()
#14 0x0000000000000000 in ?? ()
rax	0x7ffff4f03128	140737302769960
rbx	0x7fffffffa138	140737488331064
rcx	0x0	0
rdx	0x7ffff4f030f8	140737302769912
rsi	0x7ffff4f03160	140737302770016
rdi	0x5555558e8830	93824995985456
rbp	0x7fffffff9930	140737488329008
rsp	0x7fffffff9860	140737488328800
r8	0x11	17
r9	0x7fffffffae28	140737488334376
r10	0x2	2
r11	0x7ffff6ed3f90	140737336131472
r12	0x7ffff4f03128	140737302769960
r13	0x7ffff4f03160	140737302770016
r14	0x7ffff4f03090	140737302769808
r15	0x7ffff4f03090	140737302769808
rip	0x555556d86182 <js::ModuleBuilder::processExportFrom(js::frontend::BinaryNode*)+258>
=> 0x555556d86182 <_ZN2js13ModuleBuilder17processExportFromEPNS_8frontend10BinaryNodeE+258>:	movzwl (%rcx),%eax
   0x555556d86185 <_ZN2js13ModuleBuilder17processExportFromEPNS_8frontend10BinaryNodeE+261>:	cmp    $0x3e8,%rax

This is a fairly frequent OOM that we have not been able to isolate as a standalone testcase until now.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

I'd be curious to know why you need two calls to oomTest here. I've seen this pattern quite a few times and wonder if we should change our testing approach.

Comment 4

[André Bargull [:anba]]

There are missing OOM checks in GeneralParser<ParseHandler, Unit>::exportFrom: https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/js/src/frontend/Parser.cpp#5704-5705 and https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/js/src/frontend/Parser.cpp#5719-5720.

Comment 5

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

Set release status flags based on info from the regressing bug 1736060

Comment 6

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

:jon4t4n, since you are the author of the regressor, bug 1736060, could you take a look?
For more information, please visit auto_nag documentation.

Comment 7

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220414092955-6c55ba30c858.
The bug appears to have been introduced in the following build range:

Start: e565a940acd104cc68f0695a00ed6d6e476bf6fb (20211201183242)
End: 3bd18dfc114ff7dd5cdd288b345331aa3568a2e3 (20211201183342)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e565a940acd104cc68f0695a00ed6d6e476bf6fb&tochange=3bd18dfc114ff7dd5cdd288b345331aa3568a2e3

Comment 8

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1764737 - Add missing OOM checks in GeneralParser<ParseHandler, Unit>::exportFrom. r=mgaudet

Comment 9

[Jonatan Klemets [:jon4t4n]]

The above patch should fix the crash.

Regarding the question of why we need to call oomTest twice here, I am not able to add any insight.

Comment 10

[Iain Ireland [:iain]]

I can't speak to this particular case, but oomTest is inevitably going to be somewhat flaky on parts of the engine, because its implementation trades off coverage for speed.

oomTest works by repeatedly calling a function and failing the Nth allocation, for increasing N, until N gets large enough that we don't fail an allocation. However, we don't reset the state of the world between each call. For example, the warmup count will increase on each execution. When we hit a warmup threshold and compile, it's likely that we'll OOM somewhere in the compiler, but there are no guarantees where. When the compilation fails, we will usually reset the warmup count and start again. On the next iteration, it's likely that allocation N+1 is not in the compiler, because we need to warm up more before we enter it again. This means that code that depends on previous state (compilers, ICs, etc...) gets patchy OOM coverage.

The alternative would be to reset the engine to its starting point on each iteration. The downside of that is that it would be even slower than the current version of oomTest, which is already unwieldy. Given that we're talking about removing OOM handling entirely, I'm not sure this is the most valuable place to invest our time (although we've been talking of removing OOM handling for years without any movement, so there are no guarantees.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4f304b80390
Add missing OOM checks in GeneralParser<ParseHandler, Unit>::exportFrom. r=mgaudet

Comment 12

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/b4f304b80390

Comment 13

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220414214512-d8f67ac43580.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 14

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

The patch landed in nightly and beta is affected.
:jon4t4n, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Andrew McCreight [:mccr8]]

This is a fairly old regression, and we're late in beta, so I think we can let this ride the trains.