1384121 - C++ helper functions can trigger ObjectGroup sweeping and walk the stack on OOM

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

decoder found some hard to repro crashes that seem to be caused by C++ functions called directly from IC code triggering ObjectGroup sweeping.

OOM there will walk the stack which is not okay when we make a callWithABI call.

Comment 1

[Jan de Mooij [:jandem]]

Tomorrow I'll look into adding a debug-only mechanism to make this stuff safer by complaining more loudly in debug builds. The goal is to get assertion failures on jit-tests with that in place.

Comment 2

[Andrew McCreight [:mccr8]]

Sounds kind of moderate due to how hard it is to trigger, but adjust as desired.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP asserts

This adds some DEBUG-only code so we can later add the following assert to ObjectGroup::maybeSweep:

  MOZ_ASSERT(!TlsContext.get()->inUnsafeCallWithABI);

With the patch + that assert we catch the bug reliably on jit-tests.

The patch requires annotating all callWithABI callees but that seems worth it.

Comment 4

[Jan de Mooij [:jandem]]

Filed bug 1399471 with the patch to add the instrumentation to check for these issues.

The patch there won't catch *this* bug yet, but after that lands it's a matter of adding a few asserts. I'll do that as part of this bug.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

This uses the DontCheckGeneration variants of various TI functions. It fixes all jit-test failures I get with the next patch that adds debug checks.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add debug asserts

This uses the machinery added in bug 1399471 to assert we don't walk the stack etc under callWithABI calls.

This also fixes some false positives in jit-tests: under Bailout and InvalidationBailout we can walk the stack (due to the bailoutData on JitActivation) so I whitelisted them.

InitBaselineFrameForOsr can walk the stack when the debugger is enabled and that's okay because we push an exit frame.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8908145 [details] [diff] [review]
Part 1 - Fix

Requesting sec-approval for both patches. I think we should land both on 57 but we only have to backport the first one.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very difficult.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
55+.

> If not all supported branches, which bug introduced the flaw?
Bug 1341067 and more recent ones.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial.

> How likely is this patch to cause regressions; how much testing does it need?
Pretty unlikely.

Comment 8

[Jan de Mooij [:jandem]]

Oh this was marked sec-moderate. Well let's request sec-approval just in case :)

Comment 10

[Al Billings [:abillings - ex-MoCo]]

This is too late for Firefox 56. We're shipping in a week and a half. As a sec-moderate, you can just check this into trunk though.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05f6da3339b33ba5bc4d369de5a706cd9523395d

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/05f6da3339b3

Comment 14

[Jan de Mooij [:jandem]]

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab55565955de81c880c31c1e1c37506b5b042e0

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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