1914982 - (CVE-2024-10468) Assertion failure: IsIdle(oldState) || IsRead(oldState), at /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:130

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Found with m-c 20240814-ee094976d8e3 (--enable-address-sanitizer --enable-fuzzing)

This was found by visiting a live website with a debug build.

STR:

• Launch browser and visit site

This issue was triggered by visiting http://www.fortisbc.com/.

This has been triggered by visiting a few different sites so far:

• http://hp.com/
• http://cdn.branch.io/
• http://www.fortisbc.com/
• http://eurogamer.net/
• http://webfeathosting.com/

I cannot reproduce the issue but it has been reported multiple times since m-c 20240814-ee094976d8e3.

Assertion failure: IsIdle(oldState) || IsRead(oldState), at /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:130
Assertion failure: IsWrite(oldState), at /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:151

#0 0x77bdf3dddb04 in EndWriteOp /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:151:5
#1 0x77bdf3dddb04 in PLDHashTable::EntryHandle::~EntryHandle() /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.cpp:707:20
#2 0x77bdf88277b9 in ~EntryHandle /builds/worker/workspace/obj-build/dist/include/nsTHashtable.h:384:28
#3 0x77bdf88277b9 in operator()<nsTHashtable<nsBaseHashtableET<nsIntegralHashKey<unsigned long, 0>, mozilla::NotNull<mozilla::dom::indexedDB::FileInfo<mozilla::dom::indexedDB::DatabaseFileManager> *> > >::EntryHandle> /builds/worker/workspace/obj-build/dist/include/nsBaseHashtable.h:836:11
#4 0x77bdf88277b9 in operator()<PLDHashTable::EntryHandle> /builds/worker/workspace/obj-build/dist/include/nsTHashtable.h:437:18
#5 0x77bdf88277b9 in WithEntryHandle<(lambda at /builds/worker/workspace/obj-build/dist/include/nsTHashtable.h:436:9)> /builds/worker/workspace/obj-build/dist/include/PLDHashTable.h:605:12
#6 0x77bdf88277b9 in WithEntryHandle<(lambda at /builds/worker/workspace/obj-build/dist/include/nsBaseHashtable.h:835:15)> /builds/worker/workspace/obj-build/dist/include/nsTHashtable.h:434:25
#7 0x77bdf88277b9 in WithEntryHandle<(lambda at /builds/worker/workspace/obj-build/dist/include/nsBaseHashtable.h:464:34)> /builds/worker/workspace/obj-build/dist/include/nsBaseHashtable.h:834:18
#8 0x77bdf88277b9 in InsertOrUpdate<mozilla::NotNull<mozilla::dom::indexedDB::FileInfo<mozilla::dom::indexedDB::DatabaseFileManager> *> &> /builds/worker/workspace/obj-build/dist/include/nsBaseHashtable.h:464:12
#9 0x77bdf88277b9 in mozilla::dom::indexedDB::FileInfoManager<mozilla::dom::indexedDB::DatabaseFileManager>::CreateFileInfo()::'lambda'()::operator()() const /builds/worker/checkouts/gecko/dom/indexedDB/FileInfoManager.h:62:18
#10 0x77bdf88275e9 in operator() /builds/worker/checkouts/gecko/dom/indexedDB/FileInfoManager.h:112:34
#11 0x77bdf88275e9 in mozilla::SafeRefPtr<mozilla::dom::indexedDB::FileInfo<mozilla::dom::indexedDB::DatabaseFileManager>> mozilla::dom::indexedDB::FileInfoManager<mozilla::dom::indexedDB::DatabaseFileManager>::AcquireFileInfo<mozilla::dom::indexedDB::FileInfoManager<mozilla::dom::indexedDB::DatabaseFileManager>::CreateFileInfo()::'lambda'()>(mozilla::dom::indexedDB::FileInfoManager<mozilla::dom::indexedDB::DatabaseFileManager>::CreateFileInfo()::'lambda'() const&) const /builds/worker/checkouts/gecko/dom/indexedDB/FileInfoManager.h:109:21
#12 0x77bdf880b65d in CreateFileInfo /builds/worker/checkouts/gecko/dom/indexedDB/FileInfoManager.h:53:12
#13 0x77bdf880b65d in mozilla::dom::indexedDB::(anonymous namespace)::Database::AllocPBackgroundIDBDatabaseFileParent(mozilla::dom::IPCBlob const&) /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:9559:30
#14 0x77bdf8916cf1 in mozilla::dom::indexedDB::PBackgroundIDBDatabaseParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundIDBDatabaseParent.cpp:558:63
#15 0x77bdf4a53c86 in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundParent.cpp:2144:32
#16 0x77bdf49f2bef in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1785:25
#17 0x77bdf49efb72 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1712:9
#18 0x77bdf49f07f2 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1503:3
#19 0x77bdf49f193f in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1603:14
#20 0x77bdf3e92f56 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1149:16
#21 0x77bdf3e99a3f in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#22 0x77bdf49f9965 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:329:5
#23 0x77bdf4950771 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#24 0x77bdf4950771 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#25 0x77bdf3e8e483 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:366:10
#26 0x77be085876ef in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
#27 0x77be08641ac2 in start_thread nptl/pthread_create.c:442:8
#28 0x77be086d384f  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Tyson Smith [:tsmith]]

In addition to the original assertion I also see Assertion failure: IsWrite(oldState), at /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:151 reported.

Comment 2

[Jan Varga [:janv]]

I quickly checked related code, there's nothing obvious. Maybe the file manager is initializing mFileInfos (which is not protected by the mutex) or the manager is being destroyed.
I think we should start with either trying to reproduce the problem and/or adding a check for mClosed in Database::AllocPBackgroundIDBDatabaseFileParent like we have in Database::AllocPBackgroundIDBTransactionParent.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

It looks like the mLastFileId and mFileInfos members of FileInfoManager don't have thread safety annotations using the thread safety analysis (https://searchfox.org/mozilla-central/rev/aee7c3a0dbf33af0c4f6648f391db62b35895e50/dom/indexedDB/FileInfoManager.h#132-135).

They should probably be marked as MOZ_GUARDED_BY(FileManager::Mutex()). The actual implementations of that should probably then be annotated like MOZ_RETURN_CAPABILITY(sMutex).

I don't know for sure how well this stuff works with templates, but it's probably worth a shot and might be helpful when finding the failure here.

Comment 4

[Andrew McCreight [:mccr8]]

Tyson, do you have stacks from any other threads in these reports? It might show if another thread is modifying the same hash table. The other possibility is reentrance, but it doesn't look obviously like that to me.

Comment 5

[Jan Varga [:janv]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #3)

It looks like the mLastFileId and mFileInfos members of FileInfoManager don't have thread safety annotations using the thread safety analysis (https://searchfox.org/mozilla-central/rev/aee7c3a0dbf33af0c4f6648f391db62b35895e50/dom/indexedDB/FileInfoManager.h#132-135).

Yeah, now that we have this option, this bug is a good opportunity to start using it.

I suspect that the hash table is accessed by
https://searchfox.org/mozilla-central/rev/8fffdc727aa507ee4955042ec2d6f71d23c9c2de/dom/indexedDB/ActorsParent.cpp#11974
https://searchfox.org/mozilla-central/rev/8fffdc727aa507ee4955042ec2d6f71d23c9c2de/dom/indexedDB/FileInfoManager.h#62
at the same time.

Comment 6

[Tyson Smith [:tsmith]]

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

Tyson, do you have stacks from any other threads in these reports?

No sorry, all reports have the same stack.

Comment 7

[Andrew McCreight [:mccr8]]

Hmm, right, I guess the assertion crashes are one kind of stack, and it is the minidump stacks that have all of the threads. I'm not sure how those get generated exactly.

Comment 8

[Harveer Singh]

Attachment: Bug 1914982: Fixing the race condition in FileInfoManager.h.r=janv

Comment 9

[Harveer Singh]

Attachment: Bug 1914982: Annotated FileInfoManager template with mutex attributes.r=#dom-storage-reviewers

Comment 10

[Harveer Singh]

Attachment: Bug 1914982: Fixed potential race conditions in using FileInfoManager.r=#dom-storage-reviewers

Comment 11

[Harveer Singh]

Attachment: Bug 1914982: Adjusted gtest.r=#dom-storage-reviewers

Comment 12

[Harveer Singh]

Attachment: WIP: Bug 1914982: Fixing the race condition in FileInfoManager.h.r=janv

https://phabricator.services.mozilla.com/D222469

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0038235e26e1
Fixing the race condition in FileInfoManager.h.r=dom-storage-reviewers,janv

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/0038235e26e1

Comment 15

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

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

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

For more information, please visit BugBot documentation.

Comment 16

[Daniel Veditz [:dveditz]]

How far back does this go? Is ESR-128 affected? Is ESR-115 affected? Something seemed to change in our detection of this with m-c 20240814-ee094976d8e3, but the bug itself might go back further.

Comment 17

[Jan Varga [:janv]]

(In reply to Daniel Veditz [:dveditz] from comment #16)

How far back does this go? Is ESR-128 affected? Is ESR-115 affected? Something seemed to change in our detection of this with m-c 20240814-ee094976d8e3, but the bug itself might go back further.

As we say here, it's old like Prague :)
However, the issue can only manifest in debug builds and AFAIK, there are not many users running beta or ESR debug builds.

Comment 18

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt