Closed Bug 1577224 Opened 5 years ago Closed 5 years ago

Assertion failure: inhibitNops_ && (isPoolEmptyFor(InstSize) || canNotPlacePool_), at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1121 with Baseline Interpreter on the stack

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Regression)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Attachments

(2 files)

I don't have a good testcase for this but it reproduced in m-c rev f6a1009171e5 - all testcases so far are non-reproducible.

Backtrace:

#0  js::jit::AssemblerBufferWithConstantPools<1024u, 4u, js::jit::Instruction, js::jit::Assembler, 0u>::assertNoPoolAndNoNops (this=0x1affd9a8) at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1121
#1  js::jit::Assembler::writeCodePointer (this=0x1affe554, label=0x1affdfe8) at js/src/jit/arm/Assembler-arm.cpp:781
#2  0x5815dfd9 in js::jit::BaselineInterpreterGenerator::emitInterpreterLoop (this=0x1affe4e8) at js/src/jit/BaselineCodeGen.cpp:7019
#3  0x5815e9cc in js::jit::BaselineInterpreterGenerator::generate (this=0x1affe4e8, interpreter=...) at js/src/jit/BaselineCodeGen.cpp:7071
#4  0x582e2153 in js::jit::GenerateBaselineInterpreter (cx=0xd70aa800, interpreter=...) at js/src/jit/BaselineJIT.cpp:1049
#5  0x583c236e in js::jit::JitRuntime::initialize (this=0x17c10000, cx=0xd70aa800) at js/src/jit/Ion.cpp:218
#6  0x57aa36af in JSRuntime::createJitRuntime (this=0x2d111000, cx=0xd70aa800) at js/src/vm/Realm.cpp:138
#7  0x57d301e6 in JS::InitSelfHostedCode (cx=0xd70aa800) at js/src/jsapi.cpp:428
/snip

For detailed crash information, see attachment.

This has appeared since m-c rev 872c3c79f906 in July 2019 and happens on 32-bit ARM simulator, 64-bit ARM64 simulator and aarch64 Linux shells, so I'm guessing this is related to ARM and baseline interpreter code interactions.

Jan, is this likely related to bug 1567327 based on the stack?

Flags: needinfo?(jdemooij)

I was the one to add this assertion. What it means is that we are attempting to write a jump table, but we did no protected the jump table region with AutoForbidPoolsAndNops, or we called it with not enough space which could cause a pool entry to be inserted.

This assertion highlight issues which could lead to random memory execution, by inserting random bytes in the expected sequence of a jump table. However, we might downgrade the sec-critical status if there is no user-controlled bits executed before the GenerateBaselineInterpreter function. I will let Jan judge that.

Group: javascript-core-security
Component: JavaScript Engine → JavaScript Engine: JIT
Keywords: sec-critical
Priority: -- → P1

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

I was the one to add this assertion. What it means is that we are attempting to write a jump table, but we did no protected the jump table region with AutoForbidPoolsAndNops, or we called it with not enough space which could cause a pool entry to be inserted.

It should be called with enough space here: https://searchfox.org/mozilla-central/rev/8ea946dcf51f0d6400362cc1d49c8d4808eeacf1/js/src/jit/BaselineCodeGen.cpp#7007-7008 Does anything there look wrong to you?

I wonder if it could be a weird OOM issue; the interpreter code is like a trampoline and should basically be identical each time it's generated.

Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)

I looked at the code which is being called. I do not see what might be going wrong. The reserved amount of instructions sounds correct.
I will just note that the AssemblerBufferWithConstantPools is way to complex for what it is supposed to be doing.

Flags: needinfo?(nicolas.b.pierron)

I think it's an OOM issue and we just need to fix the assertion to take that into account.

Gary, if I give you a patch can you confirm that, or is it too unreliable for you to tell?

Flags: needinfo?(nth10sd)

Too unreliable I think, it happens once every few days or week? I'd say land the patch and we can follow up as necessary.

Flags: needinfo?(nth10sd)
Flags: needinfo?(jdemooij)

Most of the pool-related methods return early on OOM.

Flags: needinfo?(jdemooij)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Group: javascript-core-security
Keywords: sec-critical
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c352d78e1c5
Fix assertion in assertNoPoolAndNoNops to account for OOM. r=nbp
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Do you want to request uplift to beta? If not let's mark it wontfix but I'm happy to take a patch now in early beta.

Flags: needinfo?(nicolas.b.pierron)
Keywords: regression

(In reply to Liz Henry (:lizzard) from comment #9)

Do you want to request uplift to beta? If not let's mark it wontfix but I'm happy to take a patch now in early beta.

This is a debug only assertion, so unless this is required by fuzzers, I suggest to only follow the train.

Flags: needinfo?(nicolas.b.pierron) → needinfo?(nth10sd)

We don't perform beta uplifts for fuzzing-only anymore, so this can ride the trains.

Flags: needinfo?(nth10sd)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: