1815904 - Crash in [@ nsStringBuffer::AddRef] on poison value from QuotaManager::QuotaManager during shutdown

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P2)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/d24f9444-fc23-4dde-a636-90edc0230202

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  std::_Fetch_add_relaxed_4  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xatomic.h:1550
0  xul.dll  std::_Atomic_fetch_add_4  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xatomic.h:1574
0  xul.dll  std::atomic_fetch_add_explicit  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xxatomic:910
0  xul.dll  std::_Atomic_uint::fetch_add  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xxatomic:1105
0  xul.dll  nsStringBuffer::AddRef  xpcom/string/nsStringBuffer.cpp:31
0  xul.dll  nsTSubstring<char16_t>::Assign  xpcom/string/nsTSubstring.cpp:516
0  xul.dll  nsTSubstring<char16_t>::Assign  xpcom/string/nsTSubstring.cpp:482
1  xul.dll  nsTString<char16_t>::nsTString  xpcom/string/nsTString.h:105
1  xul.dll  mozilla::dom::quota::QuotaManager::QuotaManager  dom/quota/ActorsParent.cpp:2781
2  xul.dll  mozilla::MakeRefPtr  mfbt/RefPtr.h:603

It looks like QuotaManager::QuotaManager accesses gStorageName and gBasePath, and one or both of them have been freed already. In all of the crashes I looked at, the main thread was in the middle of XPCOM shutdown, so this doesn't seem easy to exploit, but maybe we could clean it up somehow.

Comment 1

[Jens Stutte [:jstutte]]

It is surprising that we do not bail out on IsShuttingDown if we are already inside xpcom-shutdown-threads, as we should have done QuotaManager::Shutdown way earlier.

We might see a situation where we actually never initialized the QuotaManager ? At least on two of the crashes I looked at I see the BackgroundTaskName: backgroundupdate annotation, which would speak for a situation of immediate shutdown without time or reason to initialize anything as normal.

So it seems we do initialize both gBasePath and gStorageName at profile-do-change and free them on profile-before-change-qm. But gShutdown will be set only if we ever had a QM instance. However, it remains unclear to me, why we would not see gBasePath to be nullptr, though.

Comment 2

[Jan Varga [:janv]]

In theory, it can happen that gBasePath is not null and gStorageName is null if we fail here https://hg.mozilla.org/releases/mozilla-beta/file/9a24b0436a32f390ee59fc318a23cd8d4ecb5571/dom/quota/ActorsParent.cpp#l2675

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1815904 - Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r?#dom-storage-reviewers

Comment 4

[Jens Stutte [:jstutte]]

Please note that I see other instances with the same signature that seem unrelated to quota manager. This patch will help only with the QM ones, if ever.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Jens Stutte [:jstutte] from comment #4)

Please note that I see other instances with the same signature that seem unrelated to quota manager. This patch will help only with the QM ones, if ever.

Yeah, it seemed like it was the majority of them, even if it isn't all of them.

Comment 6

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

Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d86e714651a2dc474ebc04278b694a627af02f05
https://hg.mozilla.org/mozilla-central/rev/d86e714651a2

Comment 7

[Jens Stutte [:jstutte]]

Something weird happened here as the phabricator push message is missing from the bug?

Should we better close this bug and in case file a new one for remaining cases? That might help relman to understand if we should uplift something here, too.

Comment 8

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

Did you mean pulsebot? It does not have access to security bugs and cannot post the comment about the push to autoland. The autoland revision has the phabricator url.

Comment 9

[Jens Stutte [:jstutte]]

Ah thanks, never noticed that.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jstutte, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Jens Stutte [:jstutte]]

Comment on attachment 9318213 [details]
Bug 1815904 - Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r?#dom-storage-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Unclear, as most crashes seem to happen inside a backgroundtask process. Regular shutdown is tested very frequently, but we lack an explicit test for immediate startup/shutdown.
  Note also that this signature might still appear after landing for other, unrelated cases.
• Is this code covered by automated tests?: Unknown
• 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): This really just closes a possible hole in our QM shutdown state machinery, setting a flag, so this seems easy and safe enough to be uplifted.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9318213 [details]
Bug 1815904 - Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r?#dom-storage-reviewers

Approved for 111.0b5

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c78f541674a

Comment 14

[Jens Stutte [:jstutte]]

The so far remaining crashes after patch uplift refer to different causes as comment 4 already said. I'd propose to file new bugs for those.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Do you think we should land this on ESR still? Please nominate if so.

Comment 16

[Jens Stutte [:jstutte]]

Comment on attachment 9318213 [details]
Bug 1815904 - Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r?#dom-storage-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-moderate, but only because this happens during shutdown, otherwise it would be sec-high.
• User impact if declined: Unclear, as most crashes seem to happen inside a backgroundtask process. Regular shutdown is tested very frequently, but we lack an explicit test for immediate startup/shutdown.
  Note also that this signature might still appear after landing for other, unrelated cases.
• Fix Landed on Version: 111
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): If it grafts cleanly (I assume it will), this really just closes a possible hole in our QM shutdown state machinery, setting a flag, so this seems easy and safe enough to be uplifted.

Edit: Updated to sec-moderate.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Jens Stutte [:jstutte] from comment #16)

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is a sec-high.

This is a sec-moderate not a sec-high. We rated it like that because it is happening during shutdown and thus is likely to be hard to exploit. I think it is still a good idea to backport because it seems low risk and might be worse than we thought.

Comment 18

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #17)

This is a sec-moderate not a sec-high. We rated it like that because it is happening during shutdown and thus is likely to be hard to exploit. I think it is still a good idea to backport because it seems low risk and might be worse than we thought.

Sure, sorry for the confusion and thanks for the correction.

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9318213 [details]
Bug 1815904 - Set the gShutdown flag on shutdown even if we never instantiated a QM instance. r?#dom-storage-reviewers

Approved for 102.9esr.

Comment 20

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/f96b38e12f04