1619305 - Crash in [@ mozilla::dom::indexedDB::`anonymous namespace'::Database::Invalidate]

Status: RESOLVED FIXED

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

Description

[Tom Tung [:tt, :ttung]]

This bug is for crash report bp-ed1875d5-63e3-48b4-84a9-ea6cf0200301.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::Database::Invalidate dom/indexedDB/ActorsParent.cpp:13317
1 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::SendResults dom/indexedDB/ActorsParent.cpp:21733
2 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run dom/indexedDB/ActorsParent.cpp
3 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::NoteDatabaseClosed dom/indexedDB/ActorsParent.cpp:21570
4 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::Database::Invalidate dom/indexedDB/ActorsParent.cpp:13335
5 xul.dll mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::AbortOperationsForProcess dom/indexedDB/ActorsParent.cpp:17033
6 xul.dll mozilla::dom::quota::QuotaManager::AbortOperationsForProcess dom/quota/ActorsParent.cpp:4779
7 xul.dll mozilla::dom::quota::`anonymous namespace'::Quota::RecvAbortOperationsForProcess dom/quota/ActorsParent.cpp:8855
8 xul.dll mozilla::dom::quota::PQuotaParent::OnMessageReceived ipc/ipdl/PQuotaParent.cpp:423
9 xul.dll mozilla::ipc::PBackgroundParent::OnMessageReceived ipc/ipdl/PBackgroundParent.cpp:3522

Comment 1

[Simon Giesecke [:sg] [he/him]]

There's a re-entrant call into Database::Invalidate here, which looks suspicious.

There are two more inline frames here:

xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::Database::Invalidate() Line 13317
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(13317)
xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::SendResults() Line 21737
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(21737)
xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run() Line 0
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(0)
xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::NoteDatabaseClosed(mozilla::dom::indexedDB::`anonymous namespace'::Database * aDatabase) Line 21572
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(21572)
[Inline Frame] xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::Database::CloseInternal() Line 13569
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(13569)
xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::Database::Invalidate() Line 13335
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(13335)
[Inline Frame] xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::InvalidateLiveDatabasesMatching(const mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::AbortOperationsForProcess::<unnamed-tag> & aCondition) Line 17018
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(17018)
xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::AbortOperationsForProcess(mozilla::dom::IdType<mozilla::dom::ContentParent> aContentParentId) Line 17033
	at z:\task_1582840938\build\src\dom\indexedDB\ActorsParent.cpp(17033)
xul.dll!mozilla::dom::quota::QuotaManager::AbortOperationsForProcess(mozilla::dom::IdType<mozilla::dom::ContentParent> aContentParentId) Line 4778
	at z:\task_1582840938\build\src\dom\quota\ActorsParent.cpp(4778)
xul.dll!mozilla::dom::quota::`anonymous namespace'::Quota::RecvAbortOperationsForProcess(const mozilla::dom::IdType<mozilla::dom::ContentParent> & aContentParentId) Line 0
	at z:\task_1582840938\build\src\dom\quota\ActorsParent.cpp(0)

Comment 2

[Simon Giesecke [:sg] [he/him]]

This is a UAF to the OpenDatabaseOp object.

I think this can only happen when we ran through https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21609, because we don't have a kung-fu death grip on this before calling Run, but as https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21609 this is required.

Before https://bugzilla.mozilla.org/show_bug.cgi?id=1356824, we held a kung-fu death grip in OpenDatabaseOp::SendResults itself. I think this either needs to be put back in place (but there seems to have been a reason to remove it), or OpenDatabaseOp::NoteDatabaseClosed must be changed to always acquire a kung-fu death grip.

Jan, WDYT?

Comment 3

[Jan Varga [:janv]]

I suspect that there's a code path which leads to a dangling pointer in gLiveDatabaseHashtable->mLiveDatabases.
Fortunately we have CheckedUnsafePtr which can catch that when the diagnostic assertion is enabled. See this crash report:
https://crash-stats.mozilla.org/report/index/709d791b-1f4f-4b2e-8cff-235380200302

So I think the root cause is that we don't call Database::CleanupMetadata().
I don't know what would be ideal fix for this or where to put additional CleanupMetadata call. That requires more investigation.

Comment 4

[Simon Giesecke [:sg] [he/him]]

Thanks for the crash report with the CheckedUnsafePtr.

However, AFAIU this is not about the Database, but the OpenDatabaseOp. So you probably mean gFactoryOps and a missing call to FactoryOp::CleanupMetadata.

However, I don't think this is the cause of the issue. If the object is held alive as I suggested, CleanupMetadata will be called at https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21789 or https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21800.

Please reconsider my original comment https://bugzilla.mozilla.org/show_bug.cgi?id=1619305#c2.

Comment 5

[Jan Varga [:janv]]

Yeah, gFactoryOps, sorry for the confusion.

More details that I found:
WaitForTransactions is called here:
https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21609
That can synchronously call the callback, so FactoryOp::Run() is called and the state is WaitingForTransactionsToComplete
DispatchToWorkThread is called here:
https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#20720
But that can fail if we are shutting down or the actor is destroyed or database is invalidated.
So if that fails, SendResults is called synchronously and SendResults may release the last strong reference to FactoryOp/OpenDatabaseOp by calling PBackgroundIDBFactoryRequestParent::Send__delete__ here:
https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21775
So it seems that the object is released before SendResults is able to call CleanupMetadata.

Yeah it seems a strong self reference is needed, but that's not all I think.
Note that after calling WaitForTransactions which led to synchronous SendResults call, we are still here:
https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#22377

The code below calls SendResults() if rv is an error. That shouldn't be called if WaitForTransactions() was called above in this method because of the actorDestroyed check.

So yes, always acquire a strong self reference, but all this stuff looks pretty fragile to me. It deserves a big comment probably and maybe also add an assertion here:
https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#22385
that mState is not already State::Completed or something even better.

Comment 6

[Frederik Braun [:freddy]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

This is a UAF to the OpenDatabaseOp object.

Simon requested a rating.
Use-After free sounds like sec-high in accordance with https://wiki.mozilla.org/Security_Severity_Ratings/Client

Comment 7

[Simon Giesecke [:sg] [he/him]]

Unfortunately, the value of actorDestroyed is optimized out.

I think the stack trace indicates that Run was actually called from https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/indexedDB/ActorsParent.cpp#21620, but this might be misleading due to optimizations.

But you're right: If WaitForTransactions was called, we couldn't get into the branch containing that Run call.

And I see this code is duplicated for both OpenDatabaseOp and DeleteDatabaseOp, which should probably be resolved as well.

Unfortunately, this is probably almost impossible to reproduce, or is it?

Comment 8

[Jan Varga [:janv]]

Well, its seems it would be quite hard to write a test for this, but maybe not that hard if done as an xpcshell test.
And yes, DeleteDatabaseOp needs to be updates too, thanks for catching that.

Comment 9

[Simon Giesecke [:sg] [he/him]]

As indicated by https://bugzilla.mozilla.org/show_bug.cgi?id=1619305#c3, this shows no crashes for Nightly, as they crash with a different signature earlier due to the CheckedUnsafePtr diagnostic assertion with a generic signature. I think the CheckedUnsafePtr frames should be added to the prefix list to allow to add another specific signature here to better see the effects of any fix for this issue.

Comment 10

[Simon Giesecke [:sg] [he/him]]

Probably, esr68 is affected by the same root cause, but uplifting the patches from Bug 1619965 seems to be too risky given that the surrounding code changed significantly compared to esr68.

Comment 11

[Simon Giesecke [:sg] [he/him]]

Observing that there have been no new crash reports since Bug 1619965 was uplifted to 76.0b7 ca. 2 weeks ago.

Comment 12

[Simon Giesecke [:sg] [he/him]]

Fixed via Bug 1619965

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Bug 1619965 has been uplifted to ESR68 for the 68.9esr release.