Closed Bug 1384121 Opened 2 years ago Closed 2 years ago
C++ helper functions can trigger Object
Group sweeping and walk the stack on OOM
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.
Summary: C++ helper functions can trigger ObjectGroup sweeping → C++ helper functions can trigger ObjectGroup sweeping and walk the stack on OOM
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.
Sounds kind of moderate due to how hard it is to trigger, but adjust as desired.
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.
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.
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.
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.
Attachment #8908150 - Flags: review?(nicolas.b.pierron)
Attachment #8908145 - Flags: review?(bhackett1024) → review+
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.
Oh this was marked sec-moderate. Well let's request sec-approval just in case :)
Attachment #8908150 - Flags: review?(nicolas.b.pierron) → review+
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.
Whiteboard: [adv-main57+] → [adv-main57+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.