1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[Jan Varga [:janv]]

This is probably the root cause of sec bugs like bug 1499719 and bug 1499108.
It also contributes to quota manager shutdown hangs (there may still be other issues that cause quota manager shutdown hangs).

Not calling TransactionDatabaseOperationBase::SendFailureResult is normally ok, but in the case of OpenDatabaseOp::VersionChangeOp, it means that the parent operation (OpenDatabaseOp) won't have a chance to correctly cleanup.

I found this when I was working on bug 1533789 which involves new local storage implementation which uses a similar design like IDB.

Comment 1

[Jan Varga [:janv]]

Attachment: Patch that forces actor destruction

Comment 2

[Jan Varga [:janv]]

Just apply the patch and then run:
mach xpcshell-test dom/indexedDB/test/unit/test_put_get_values.js
and you should get a quota manager shutdown hang.

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Comment 4

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c5f4cf57b4302c64e8a4d4863671693f905560

Comment 5

[Jan Varga [:janv]]

Andrew, do you need anything else for this ?
It would be nice to see if this also helps with lowering down number of quota manager shutdown hangs.

Comment 6

[Jan Varga [:janv]]

Andrew, please see comment 5.

Comment 7

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78a50069de9a3644dd0972c21d40a8ea161055d6

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78a50069de9a3644dd0972c21d40a8ea161055d6
https://hg.mozilla.org/mozilla-central/rev/78a50069de9a

Comment 9

[Jan Varga [:janv]]

Ok, we will see now in bug 1541776 what effect this has on shutdown hangs.

Comment 10

[Jan Varga [:janv]]

Just FYI, this didn't have big impact on shutdown hangs, but it should fix some use after free sec issues.

Comment 11

[Jan Varga [:janv]]

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Probably last IDB rewrite
• User impact if declined: Potential use after free security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Verified on Nightly (landed 9 days ago), small changes and thorough review.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential use after free security issues.
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Verified on Nightly (landed 9 days ago), small changes and thorough review.
• String or UUID changes made by this patch: None

Comment 12

[Pascal Chevrel:pascalc]

I think this should have had sec-approval for landing on mozilla-central as this is rated as a sec-high and wasn't caused by a recent regression on mozilla-central (https://wiki.mozilla.org/Security/Bug_Approval_Process). Jan could you fill in the form a posteriori so as that it gets on our security team radar please? Thanks

Comment 13

[Jan Varga [:janv]]

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's a use after free issue, but the patch doesn't indicate that at all.
• 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?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be very easy to provide a patch for beta and ESR, the original patch may apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It's been tested on Nightly and there were no regressions.

Comment 14

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

Sec-approval+ for trunk. Yes, this should have had approval before going in.

I'll approve Beta as well.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a0993f0f4fb

Comment 16

[Liz Henry (:lizzard)]

Giving this another few days to see the results on beta before we do the ESR uplift.

Comment 17

[Liz Henry (:lizzard)]

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Fix for sec-high crash, OK for esr 60.7.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/fac75272fedd