1633880 - Intermittent PID 4148 | SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPtr.h:171:13 in AssignAssumingAddRef | After xpcshell return code: -6

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: nbeleuzu [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=299839709&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/NXuwz04MTzGpuWJMXmLtOg/runs/0/artifacts/public/logs/live_backing.log

---

[task 2020-04-28T19:31:18.611Z] 19:31:18 INFO - PID 4148 | Write of size 8 at 0x7f2c1893cd18 by main thread:
[task 2020-04-28T19:31:18.611Z] 19:31:18 INFO - PID 4148 | #0 AssignAssumingAddRef /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPtr.h:171:13 (libxul.so+0x3912542)
[task 2020-04-28T19:31:18.611Z] 19:31:18 INFO - PID 4148 | #1 AssignWithAddref /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPtr.h:166:5 (libxul.so+0x3912542)
[task 2020-04-28T19:31:18.611Z] 19:31:18 INFO - PID 4148 | #2 operator= /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPtr.h:120:5 (libxul.so+0x3912542)
[task 2020-04-28T19:31:18.612Z] 19:31:18 INFO - PID 4148 | #3 mozilla::dom::IPCBlobInputStreamStorage::Observe(nsISupports*, char const*, char16_t const*) /builds/worker/checkouts/gecko/dom/file/ipc/IPCBlobInputStreamStorage.cpp:64:14 (libxul.so+0x3912542)
[task 2020-04-28T19:31:18.612Z] 19:31:18 INFO - PID 4148 | #4 nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /builds/worker/checkouts/gecko/xpcom/ds/nsObserverList.cpp:65:19 (libxul.so+0xa4dbd3)
[task 2020-04-28T19:31:18.612Z] 19:31:18 INFO - PID 4148 | #5 nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /builds/worker/checkouts/gecko/xpcom/ds/nsObserverService.cpp:288:19 (libxul.so+0xa50a0c)
[task 2020-04-28T19:31:18.613Z] 19:31:18 INFO - PID 4148 | #6 mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:618:26 (libxul.so+0xb13c22)
[task 2020-04-28T19:31:18.613Z] 19:31:18 INFO - PID 4148 | #7 NS_ShutdownXPCOM /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:565:10 (libxul.so+0xb13ae5)
[task 2020-04-28T19:31:18.614Z] 19:31:18 INFO - PID 4148 | #8 XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/js/xpconnect/src/XPCShellImpl.cpp:1383:8 (libxul.so+0x1923f85)
[task 2020-04-28T19:31:18.614Z] 19:31:18 INFO - PID 4148 | #9 mozilla::BootstrapImpl::XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/toolkit/xre/Bootstrap.cpp:54:12 (libxul.so+0x63a26ab)
[task 2020-04-28T19:31:18.614Z] 19:31:18 INFO - PID 4148 | #10 main /builds/worker/checkouts/gecko/js/xpconnect/shell/xpcshell.cpp:66:27 (xpcshell+0xc8b86)

[task 2020-04-28T19:31:18.648Z] 19:31:18 INFO - PID 4148 | #65 XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/js/xpconnect/src/XPCShellImpl.cpp:1353:14 (libxul.so+0x1924634)
[task 2020-04-28T19:31:18.649Z] 19:31:18 INFO - PID 4148 | #66 mozilla::BootstrapImpl::XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/toolkit/xre/Bootstrap.cpp:54:12 (libxul.so+0x63a26ab)
[task 2020-04-28T19:31:18.649Z] 19:31:18 INFO - PID 4148 | #67 main /builds/worker/checkouts/gecko/js/xpconnect/shell/xpcshell.cpp:66:27 (xpcshell+0xc8b86)
[task 2020-04-28T19:31:18.649Z] 19:31:18 INFO - PID 4148 | SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/mozilla/StaticPtr.h:171:13 in AssignAssumingAddRef

Comment 1

[Andrew McCreight [:mccr8]]

IPCBlobInputStreamStorage is at the top of the stack.

Comment 2

[Andrew McCreight [:mccr8]]

Looks like the reported write is gStorage getting set to null in IPCBlobInputStreamStorage::Observe() and that's racing with IPCBlobInputStreamParent::HasValidStream().

Comment 3

[Intermittent Failures Robot]

8 failures in 5423 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 8

Platform breakdown:

• linux1804-64-tsan: 7
• windows7-32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-04-27&endday=2020-05-03&tree=all

Comment 4

[Intermittent Failures Robot]

9 failures in 5276 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 6
• mozilla-central: 2
• try: 1

Platform breakdown:

• windows10-64-qr: 1
• linux1804-64-tsan: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-05-04&endday=2020-05-10&tree=all

Comment 5

[Intermittent Failures Robot]

6 failures in 5488 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 6

Platform breakdown:

• linux1804-64-tsan: 6

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-05-11&endday=2020-05-17&tree=all

Comment 6

[Intermittent Failures Robot]

22 failures in 4727 pushes (0.005 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 3
• mozilla-beta: 4
• autoland: 14
• mozilla-release: 1

Platform breakdown:

• linux1804-64-tsan: 22

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-05-18&endday=2020-05-24&tree=all

Comment 7

[Intermittent Failures Robot]

8 failures in 5458 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 7
• mozilla-central: 1

Platform breakdown:

• linux1804-64-tsan: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-05-25&endday=2020-05-31&tree=all

Comment 8

[Intermittent Failures Robot]

10 failures in 4949 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 8
• try: 1
• mozilla-beta: 1

Platform breakdown:

• linux1804-64-tsan: 9
• windows7-32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-06-01&endday=2020-06-07&tree=all

Comment 9

[Intermittent Failures Robot]

21 failures in 5211 pushes (0.004 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 15
• mozilla-beta: 3
• mozilla-central: 1
• try: 2

Platform breakdown:

• linux1804-64-tsan: 21

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1633880&startday=2020-06-08&endday=2020-06-14&tree=all

Comment 10

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

So IPCBlobInputStreamStorage cleans up at "xpcom-shutdown" but PBackground where the storage can be used is alive until "xpcom-shutdown-threads", and StaticRefPtr doesn't do anything to be threadsafe on its own and IPCBlobInputStreamStorage doesn't do any kind of synchronization to protect its gStorage StaticRefPtr.

The initial reporting test is an xpcshell test, which we expect to have less shutdown fidelity, but https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306094025&repo=try&lineNumber=4671 is an example from comment 9 of a mochitest experiencing the failure. In that case, it's happening in PBackgroundParent::OnChannelError which then tears down the actors. That seems like a very likely thing to happen in real world situations because the content processes should be gone by xpcom-shutdown, but there's no guarantee the IPC fallout of that has been handled.

Comment 11

[Jens Stutte [:jstutte]]

And the race between IPCBlobInputStreamStorage::Get() and gStorage = nullptr; indicates potential for UAF, even if this is probably/hopefully only possible during shutdown.

So I see two things to do here:

1. protect the gStorage pointer itself against concurrent access from multiple threads (it is the memory of the pointer on which we race, not the memory of the object)
2. let IPCBlobInputStreamStorage::Get() return a StaticRefPtr, not a raw* in order to keep the object alife during usage even if gStorage is set to null

Subhamoy, you might want to take a look?

Comment 12

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

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

2. let IPCBlobInputStreamStorage::Get() return a StaticRefPtr, not a raw* in order to keep the object alife during usage even if gStorage is set to null

Not a StaticRefPtr, but rather a regular RefPtr (or SafeRefPtr), this one isn't static.

Comment 13

[Andrea Marchesini [:baku]]

Not a StaticRefPtr, but rather a regular RefPtr (or SafeRefPtr), this one isn't static.

The most important thing is to update the callers because, if ::Get() returns a nullptr (or a smart pointer with a nullptr value), the existing code paths must be updated. Almost all the uses of IPCBlobInputStreamStorage::Get() do not handle nullptr or failures.

Comment 15

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

Is there a reason to not keep IPCBlobInputStreamStorage alive until PBackground is fully shutdown or all of the PIPCBlobInputStream actors are shutdown and no new actors are allowed to be created? Which could also mean explicitly terminating the actors as part of an "xpcom-shutdown" shutdown blocker (or earlier)?

Adding the null-handling seems like adding new edge-cases that don't really need to exist given that there's no reason why any streams should be active post (or during) "xpcom-shutdown".

Comment 16

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

(see comment 15)

Comment 17

[Andrea Marchesini [:baku]]

My first approach was to move the shutdown of IPCBlobInputStream a bit forward down the shutdown phases. But this still means that ::Get() could return nullptr and we do not support it. As a temporary solution, I'm OK with what you propose, but in general, I would like to have some better checks of the return value of ::Get().

Comment 18

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

Then it probably makes sense to do the get/null handling first as it's more uplift-able, etc.

Comment 19

[Subhamoy [:ssengupta][he/him]]

Attachment: Bug 1633880 - P1 - gStorage in RemoteLazyInputStreamStorage is protected by gMutex r=baku

Comment 20

[Subhamoy [:ssengupta][he/him]]

Attachment: Bug 1633880 - P2 - RemoteLazyInputStreamStorage::Get() now returns mozilla::Result<RefPtr<RemoteLazyInputStreamStorage>, nsresult> r=baku

Depends on D80540

Comment 21

[Subhamoy [:ssengupta][he/him]]

Comment on attachment 9158336 [details]
Bug 1633880 - P1 - gStorage in RemoteLazyInputStreamStorage is protected by gMutex r=baku

Security Approval Request

• How easily could an exploit be constructed based on the patch?: An exploit is -- in my opinion -- not easy to construct.
• 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?: Added checks never existed before, so all of them
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: N/A
• How likely is this patch to cause regressions; how much testing does it need?: This patch is unlikely to cause regression.

Comment 22

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9158336 [details]
Bug 1633880 - P1 - gStorage in RemoteLazyInputStreamStorage is protected by gMutex r=baku

Approved to land and uplift.

Comment 23

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 24

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

Sorry for the delay, haven't checked the security checkin-needed requests over the weekend:
https://hg.mozilla.org/integration/autoland/rev/ea3941b867cace40f8ac2576512dc17f1507d3e0
https://hg.mozilla.org/integration/autoland/rev/d0b1dc88e635ee849df2fef405d5dafb68ccba8c

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Looks like these patches will need rebasing for uplift to all of the affected branches. Subhamoy, can you please attach those patches when you get a chance? Thanks!

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/ea3941b867ca
https://hg.mozilla.org/mozilla-central/rev/d0b1dc88e635

Comment 27

[Jens Stutte [:jstutte]]

Hi Ryan, Subhamoy is on holiday this week. If it is more urgent, I can ask someone else to take it.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

It can wait until next week, thanks.

Comment 29

[Jens Stutte [:jstutte]]

Subhamoy, this waits for you then ;-)

Comment 30

[Subhamoy [:ssengupta][he/him]]

Thank you! Working on it right now :)

Comment 31

[Subhamoy [:ssengupta][he/him]]

Attachment: Bug 1633880 - P1 - gStorage in IPCBlobInputStreamStorage is protected by gMutex [beta] r=baku

Comment 32

[Subhamoy [:ssengupta][he/him]]

Attachment: Bug 1633880 - P2 - IPCBlobInputStreamStorage::Get() now returns mozilla::Result<RefPtr<IPCBlobInputStreamStorage>, nsresult> [beta] r=baku

Depends on D83617

Comment 33

[Subhamoy [:ssengupta][he/him]]

Comment on attachment 9163722 [details]
Bug 1633880 - P1 - gStorage in IPCBlobInputStreamStorage is protected by gMutex [beta] r=baku

Beta/Release Uplift Approval Request

• User impact if declined: Possible crashes due to data race.
• 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): It mutex-locks certain variable accesses and introduces more robust error checking for certain subroutines. I cannot determine any downside to introducing these changes.
• String changes made/needed:

Comment 34

[Subhamoy [:ssengupta][he/him]]

Attachment: ESR68.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible crashes due to data race and/or null pointer return
Fix Landed on Version: 80
Risk to taking this patch (and alternatives if risky): None I can determine

Comment 35

[Subhamoy [:ssengupta][he/him]]

Attachment: ESR78.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible crashes due to data race and/or null pointer return
Fix Landed on Version: 80
Risk to taking this patch (and alternatives if risky): None I can determine

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163722 [details]
Bug 1633880 - P1 - gStorage in IPCBlobInputStreamStorage is protected by gMutex [beta] r=baku

Approved for 79.0b9, 78.1esr, and 68.11esr.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8605536b000
https://hg.mozilla.org/releases/mozilla-beta/rev/44b029997449

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/efe82a2aacfb
https://hg.mozilla.org/releases/mozilla-esr68/rev/bf6f03da7880

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/3111d972f397
https://hg.mozilla.org/releases/mozilla-esr78/rev/f64d74a9b713