1429028 - Crash [@ ParseExprBody] with infinite recursion involving wasmTextToBinary

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ca379fcca95b (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

evaluate(`
  for (var i = 0; i < 10; ++i) {}
  var N = 1 << 16;
  var m = new Map;
  for (var get  = 0; i < N; i++)
    m.get(i)
  evaluate(\`
    var expr = '(get_local 0)';
    for (var a = 1000; i --> 0; )
      expr = \\\`(f32.neg \\\${expr})\\\`;
    var code = \\\`(module (func \\\${expr})\\\`;
  wasmTextToBinary(code);
  \`);
`);


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000e5cb59 in ParseExprBody (c=..., token=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:2932
#0  0x0000000000e5cb59 in ParseExprBody (c=..., token=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:2932
#1  0x0000000000e5e358 in ParseExprInsideParens (c=...) at js/src/wasm/WasmTextToBinary.cpp:2950
#2  0x0000000000e5e4a8 in ParseExpr (c=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:1704
#3  0x0000000000e5cb6d in ParseUnaryOperator (inParens=true, op=js::wasm::Op::F32Neg, c=...) at js/src/wasm/WasmTextToBinary.cpp:2307
#4  ParseExprBody (c=..., token=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:2932
#5  0x0000000000e5e358 in ParseExprInsideParens (c=...) at js/src/wasm/WasmTextToBinary.cpp:2950
#6  0x0000000000e5e4a8 in ParseExpr (c=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:1704
#7  0x0000000000e5cb6d in ParseUnaryOperator (inParens=true, op=js::wasm::Op::F32Neg, c=...) at js/src/wasm/WasmTextToBinary.cpp:2307
#8  ParseExprBody (c=..., token=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:2932
#9  0x0000000000e5e358 in ParseExprInsideParens (c=...) at js/src/wasm/WasmTextToBinary.cpp:2950
#10 0x0000000000e5e4a8 in ParseExpr (c=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:1704
#11 0x0000000000e5cb6d in ParseUnaryOperator (inParens=true, op=js::wasm::Op::F32Neg, c=...) at js/src/wasm/WasmTextToBinary.cpp:2307
#12 ParseExprBody (c=..., token=..., inParens=inParens@entry=true) at js/src/wasm/WasmTextToBinary.cpp:2932
[...]
#127 0x0000000000e5cb6d in ParseUnaryOperator (inParens=true, op=js::wasm::Op::F32Neg, c=...) at js/src/wasm/WasmTextToBinary.cpp:2307
rax	0xffffffffffbc9d54	-4416172
rbx	0x7fffffff8d00	140737488325888
rcx	0xe5cb50	15059792
rdx	0x1	1
rsi	0x7fffff7ff0a8	140737479962792
rdi	0x3e	62
rbp	0x7fffff7ff080	140737479962752
rsp	0x7fffff7fef80	140737479962496
r8	0x7ffff3a41a14	140737281006100
r9	0x3b	59
r10	0x100003e00	4294983168
r11	0x1	1
r12	0x7fffffff8d00	140737488325888
r13	0x7ffff414a020	140737288380448
r14	0x1	1
r15	0x7fffffff8d00	140737488325888
rip	0xe5cb59 <ParseExprBody((anonymous namespace)::WasmParseContext&, (anonymous namespace)::WasmToken, bool)+537>
=> 0xe5cb59 <ParseExprBody((anonymous namespace)::WasmParseContext&, (anonymous namespace)::WasmToken, bool)+537>:	callq  0xe40e30 <(anonymous namespace)::WasmToken::op() const>
   0xe5cb5e <ParseExprBody((anonymous namespace)::WasmParseContext&, (anonymous namespace)::WasmToken, bool)+542>:	movzbl %r14b,%esi

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1429028: Remove unused JSContext arg to CheckRecursionLimitDontReport;

Review commit: https://reviewboard.mozilla.org/r/211338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211338/

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1429028: Add a stack recursion check in the wasm::TextToBinary function;

Review commit: https://reviewboard.mozilla.org/r/211340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211340/

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(unless it helps fuzzers, there's no need to uplift this; this isn't exposed to web content)

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8941039 [details]
Bug 1429028: Add a stack recursion check in the wasm::TextToBinary function;

https://reviewboard.mozilla.org/r/211340/#review217128

Thanks!

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8941038 [details]
Bug 1429028: Remove unused JSContext arg to CheckRecursionLimitDontReport;

https://reviewboard.mozilla.org/r/211338/#review217482

Ah, nice find.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47706f72f1d3
Remove unused JSContext arg to CheckRecursionLimitDontReport; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/316e5fab18f1
Add a stack recursion check in the wasm::TextToBinary function; r=luke

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a7ff5e790d
Backed out 2 changesets for spidermonkey bustages on js/src/jit-test/tests/wasm/compiler-frame-depth.js r=backout a=backout on a CLOSED TREE

Comment 8

[Andreea Pavel [:apavel]]

Backed out for spidermonkey bustages on js/src/jit-test/tests/wasm/compiler-frame-depth.js r=backout a=backout on a CLOSED TREE

push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=316e5fab18f1bd02f1159e64ed137aab927f73cd

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=155583927&repo=mozilla-inbound&lineNumber=83384

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/59a7ff5e790dac72e89aef826db19640a69ac5b3

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: updated.patch

One test (compiler-frame-depth) was triggering the stack limit on ASAN optimized builds; just ignore the error in this very specific case (ASAN is known to create bigger stack frames, making it prone to hit the stack limits more easily).

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8941038 [details]
Bug 1429028: Remove unused JSContext arg to CheckRecursionLimitDontReport;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211338/diff/1-2/

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8941039 [details]
Bug 1429028: Add a stack recursion check in the wasm::TextToBinary function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211340/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b178ce9de1db
Remove unused JSContext arg to CheckRecursionLimitDontReport; r=jandem
https://hg.mozilla.org/integration/autoland/rev/5c7f59616c3b
Add a stack recursion check in the wasm::TextToBinary function; r=luke

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b178ce9de1db
https://hg.mozilla.org/mozilla-central/rev/5c7f59616c3b