1736009 - The `AutoForbidPoolsAndNops` mechanism fails (asserts) on legitimate use cases on arm64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Julian Seward [:jseward]]

Attachment: test case patch

AutoForbidPoolsAndNops is an RAII class available in at least the arm and
arm64 macro assemblers. It facilitates emitting small blocks of instructions
with the guarantee that the block won't be broken up for any reason.

Legitimate uses, as exemplified by the attached test patch, have been observed
to lead to the assertion failure:

Assertion failure: hasSpaceForInsts(maxInst, 0),
  at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1086

#0 AssemblerBufferWithConstantPools<..>::enterNoPool (this=0xffffffff67f0,
                                                       maxInst=5)
   at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1086
    
#1 MozBaseAssembler::enterNoPool (this=0xffffffff64c0, maxInst=5)
   at js/src/jit/arm64/vixl/MozBaseAssembler-vixl.h:304
 
#2 AutoForbidPoolsAndNops::AutoForbidPoolsAndNops(this=0xffffffff5420,
                                                  asm_=0xffffffff64c0, maxInst=5)
   at js/src/jit/arm64/Assembler-arm64.h:775

#3 0x0000aaaaabcb9c9c in js::jit::PreCallAssertions (masm=0xffffffff64c0)
   at js/src/jit/arm64/MacroAssembler-arm64.cpp:

STR

• pull m-c and apply the test patch
• configure the shell thusly: --enable-debug --enable-optimize="-g -Og"
• run jit_tests: at least 3 tests should fail:
  ../src/jit-test/jit_test.py --no-xdr --args="" -f ./dist/bin/js

one of which is tests/bug1681258.js

Marking sec-low for now, because:

• the assertion has to do with the relative placement of code and in-line data

• so it's plausible to argue that it might be exploitable

• the assertion that fails is not a release assert, hence the failure could be
  silently missed in release builds

Why haven't we seen this before? AutoForbidPoolsAndNops is used only
relatively infrequently. The test patch causes it to be used before many but
not all call instructions that SM generates. Because those are common, this
would represent a large increase in how often it is used, compared to
unpatched SM. And even then we get only 3 failures in circa 8700 tests done
by jit_tests.

So I am inclined to think: it's really a bug, but sufficiently obscure that
jit_tests doesn't uncover it. And release builds won't trigger the assertion
but presumably could fail some other, obscure way, as a result.

Comment 1

[Lars T Hansen [:lth]]

This looks like a missed OOM condition, code does not check OOM before checking that invariants hold. The OOM will for sure be detected later so there shouldn't be any risk of bad code being executed. And by assumption all our other emitters check for buffer-full, so there should be no chance of a buffer overrun. So I don't think this is s-s.

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1736009 - Check OOM after finishPool. r?nbp

Since we depend on the state of the buffer after finishPool returns,
make sure finishPool did not OOM. Returning early seemed the cleanest
to me, as it leaves the pool state intact. In contrast, a
MOZ_ASSERT_IF(!this->oom(), ...) would allow the pool state to change.

Comment 3

[Lars T Hansen [:lth]]

Not s-s, see comment 1. Can we reopen? Thanks.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/2ffa9258ed04
Check OOM after finishPool. r=nbp

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2ffa9258ed04