1942169 - Hit MOZ_CRASH(ReceivedMessage not thread-safe) at /xpcom/base/nsISupportsImpl.cpp:43

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev f7524feb52aa (built with: --enable-debug --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch --build f7524feb52aa --debug --fuzzing  -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Hit MOZ_CRASH(ReceivedMessage not thread-safe) at /xpcom/base/nsISupportsImpl.cpp:43

    ==1836280==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7dc34df88a2a bp 0x7ffd3e31f8d0 sp 0x7ffd3e31f8c0 T1836280)
    ==1836280==The signal is caused by a WRITE memory access.
    ==1836280==Hint: address points to the zero page.
        #0 0x7dc34df88a2a in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:337:3
        #1 0x7dc34df88a2a in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const /xpcom/base/nsISupportsImpl.cpp:43:5
        #2 0x7dc35387273e in AssertOwnership<32> /builds/worker/workspace/obj-build/dist/include/nsISupportsImpl.h:59:5
        #3 0x7dc35387273e in AddRef /dom/serviceworkers/ServiceWorkerContainer.cpp:138:3
        #4 0x7dc35387273e in AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:48:39
        #5 0x7dc35387273e in AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:408:35
        #6 0x7dc35387273e in RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:108:7
        #7 0x7dc35387273e in operator()<StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage> &> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1085:28
        #8 0x7dc35387273e in mozilla::dom::ServiceWorkerContainer std::__invoke_impl<void, decltype(auto) mozilla::detail::RunnableMethodArguments<RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>>::apply<mozilla::dom::ServiceWorkerContainer, void (mozilla::dom::ServiceWorkerContainer::*)(RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>)>(mozilla::dom::ServiceWorkerContainer*, void (mozilla::dom::ServiceWorkerContainer::*)(RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>))::'lambda'(auto&&...), StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>&>(std::__invoke_other, void (mozilla::dom::ServiceWorkerContainer::*&&)(RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>), StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>&) /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/invoke.h:60:14
        #9 0x7dc3538725c1 in __invoke<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1084:9), StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage> &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/invoke.h:95:14
        #10 0x7dc3538725c1 in __apply_impl<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1084:9), std::tuple<StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage> > &, 0UL> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/tuple:1678:14
        #11 0x7dc3538725c1 in apply<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1084:9), std::tuple<StoreRefPtrPassByPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage> > &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/tuple:1687:14
        #12 0x7dc3538725c1 in apply<mozilla::dom::ServiceWorkerContainer, void (mozilla::dom::ServiceWorkerContainer::*)(RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>)> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1083:12
        #13 0x7dc3538725c1 in mozilla::detail::RunnableMethodImpl<mozilla::dom::ServiceWorkerContainer*, void (mozilla::dom::ServiceWorkerContainer::*)(RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>), true, (mozilla::RunnableKind)0, RefPtr<mozilla::dom::ServiceWorkerContainer::ReceivedMessage>>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1134:13
        #14 0x7dc34e04dd57 in mozilla::RunnableTask::Run() /xpcom/threads/TaskController.cpp:688:16
        #15 0x7dc34e04423d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:1015:20
        #16 0x7dc34e042eb7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:838:15
        #17 0x7dc34e043335 in mozilla::TaskController::ProcessPendingMTTask(bool) /xpcom/threads/TaskController.cpp:624:36
        #18 0x7dc34e0565a9 in operator() /xpcom/threads/TaskController.cpp:339:37
        #19 0x7dc34e0565a9 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /xpcom/threads/nsThreadUtils.h:548:5
        #20 0x7dc34e069c64 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1159:16
        #21 0x7dc34e07091f in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:480:10
        #22 0x7dc34ec22973 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:107:5
        #23 0x7dc34eb733b1 in RunHandler /ipc/chromium/src/base/message_loop.cc:362:3
        #24 0x7dc34eb733b1 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:344:3
        #25 0x7dc353a0dfc8 in nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:148:27
        #26 0x7dc353acfb64 in nsAppShell::Run() /widget/gtk/nsAppShell.cpp:470:33
        #27 0x7dc3549f7ddb in XRE_RunAppShell() /toolkit/xre/nsEmbedFunctions.cpp:646:20
        #28 0x7dc34ec23864 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:235:9
        #29 0x7dc34eb733b1 in RunHandler /ipc/chromium/src/base/message_loop.cc:362:3
        #30 0x7dc34eb733b1 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:344:3
        #31 0x7dc3549f720a in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:584:34
        #32 0x64b6cc51190e in main /browser/app/nsBrowserApp.cpp:397:22
        #33 0x7dc35e2491c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #34 0x7dc35e24928a in __libc_start_main csu/../csu/libc-start.c:360:3
        #35 0x64b6cc4e5178 in _start (/home/jkratzer/builds/m-c-20250115215720-fuzzing-debug/firefox-bin+0x5b178) (BuildId: b0dbc4c59886c768ae58cbf2da1c780f2faba1ae)
    
    ==1836280==Register values:
    rax = 0x000064b6ccf94e20  rbx = 0x00007dc34a375fc0  rcx = 0x0000000000000000  rdx = 0x00007dc35e423563  
    rdi = 0x00007dc35e424700  rsi = 0x0000000000000000  rbp = 0x00007ffd3e31f8d0  rsp = 0x00007ffd3e31f8c0  
     r8 = 0x0000000000000000   r9 = 0x0000000000000003  r10 = 0x0000000000000000  r11 = 0x0000000000000293  
    r12 = 0x00007dc288084d80  r13 = 0x00007dc35b7bdd59  r14 = 0x000064b6fe172840  r15 = 0x00007dc3538561a0  
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:337:3 in MOZ_Crash
    ==1836280==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 2

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1942169 using build mozilla-central 20250115215720-f7524feb52aa. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: testcase.zip

Looks like I forgot to include the entire testcase. Re-enabling bugmon.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I think this is a regression from supporting SW in workers. About to try to reproduce locally, but the stack trace + test are quite clear.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1942169 - Dispatch messages to the current thread

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

asuth, see the patch and testcase.

Comment 7

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1942169 using build mozilla-central 20250115215720-f7524feb52aa. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 8

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1942169 using build mozilla-central 20250115215720-f7524feb52aa. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

After digging into it a bit, the reason we didn't have test coverage for this is:

• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/controlled-dedicatedworker-postMessage.https.html is supposed to provide test coverage, but...
• The test is broken. I commented here on the PR that landed it but in brief it seems very much that:
  • The test issues a call to Clients.get() which that is made inside a FetchEvent.respondWith handler, creating a deadlock because step 12.3 of run a worker is to perform the fetch, and setting the client execution ready only happens in step 9 of onComplete, which requires that the respondWith promise has resolved. And step 2.1.2 of Clients.get requires waiting for the client to be execution ready.
  • WebKit does not actually have a concept of the execution ready flag in its implementation, at least for Clients.get. So the test passes for WebKit.
• There are also some other problems with the test where it seems to want to test the opposite of the specific behavior for the client message queue.
• https://pernos.co/debug/sb469MMizI9l4h8UU-WJaA/index.html is the pernosco trace I created locally from running the test to dig into this.

Unfortunately, there's some additional corrections I need to make to the other sibling tests, so my patch for that isn't full coherent yet, but it does help validate that we lacked test coverage here, and I think we should land :smaug's fix, and we wouldn't want to land test changes with the fix.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Can you make your patch non-WIP?

Also, for security rating, I think this is probably sec-high since it very much looks like we probably are running the worker JS runtime on the main thread which seems like it would allow an attacker to induce a whole bunch of very ugly/dangerous races, but I haven't fully validated that with pernosco yet.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Attachment: WIP: Bug 1942169 - Correct logic errors related to Clients.get() r=#dom-workers!

This test had 2 notable bugs as I mention at
https://github.com/web-platform-tests/wpt/pull/40414

1. It created a deadlock between Clients.get() and FetchEvent.respondWith.
   Clients.get does not resolve until the worker is execution-ready and
   the worker cannot become execution-ready until the fetch has completed.
2. The test incorrectly thought that the client message queue should discard
   rather than buffer received messages until enabled. I've removed the
   logic related to this.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9460218 [details]
Bug 1942169 - Dispatch messages to the current thread

Security Approval Request

• How easily could an exploit be constructed based on the patch?: If someone has an attack already available that relies on being able to run JS on a worker and the main thread at the same time to cause bad things to happen, easily. That said, it seems very likely for that to be a hard thing to do because it would be likely crashes would happen.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: Firefox 133 onwards
• If not all supported branches, which bug introduced the flaw?: Bug 1131324
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This should graft cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: No chance of regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

I believe this is (at least) sec-high because I believe this results in letting content code run logic against a worker thread's JS runtime from the main thread, potentially racing use of the JS runtime on the worker thread.

Comment 14

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9460218 [details]
Bug 1942169 - Dispatch messages to the current thread

Approved to land and request uplift

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1942169 - Dispatch messages to the current thread

Original Revision: https://phabricator.services.mozilla.com/D234691

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c019b65d5698
Dispatch messages to the current thread r=asuth

Comment 17

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: sec-high security concerns
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: extremely low
• Explanation of risk level: The behavior without the patch is dangerously wrong for workers; the change in dispatch mechanism for the main-thread should not impact priority or ordering
• String changes made/needed: none
• Is Android affected?: yes

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/c019b65d5698

Comment 19

[Pulsebot]

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