1862777 - Poison crash in [@ mozilla::fontlist::FontList::ShmBlock::Memory] on Android

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/4b03eeef-a0ba-4542-ad8c-250160231028

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  mozilla::UniquePtr<base::SharedMemory, mozilla::DefaultDelete<base::SharedMemory> >::get const  mfbt/UniquePtr.h:286
0  libxul.so  mozilla::UniquePtr<base::SharedMemory, mozilla::DefaultDelete<base::SharedMemory> >::operator-> const  mfbt/UniquePtr.h:281
0  libxul.so  mozilla::fontlist::FontList::ShmBlock::Memory const  gfx/thebes/SharedFontList-impl.h:312
0  libxul.so  mozilla::fontlist::Pointer::ToPtr const  gfx/thebes/SharedFontList.cpp:97
0  libxul.so  mozilla::fontlist::Pointer::ToArray<char const> const  gfx/thebes/SharedFontList.h:88
0  libxul.so  mozilla::fontlist::String::BeginReading const  gfx/thebes/SharedFontList.h:133
0  libxul.so  mozilla::fontlist::FontList::FindFamily const  gfx/thebes/SharedFontList.cpp:1144
0  libxul.so  mozilla::BinarySearchIf<mozilla::fontlist::Family*, mozilla::fontlist::FontList::FindFamily  mfbt/BinarySearch.h:80
1  libxul.so  mozilla::fontlist::FontList::FindFamily  gfx/thebes/SharedFontList.cpp:1160
2  libxul.so  gfxPlatformFontList::FindAndAddFamiliesLocked  gfx/thebes/gfxPlatformFontList.cpp:1536

These crashes are mostly on Android, though I did see a couple of on MacOS (bp-c5ac2da1-b0ee-4f31-baff-3b57d0231019, bp-12aa1791-8630-4620-956c-9f0670231018), but they were both on extremely old versions of MacOS so maybe they can be disregarded. I don't know if that's a sign that the Android crashes are also from very old versions of Android...

Comment 1

[Jim Mathies [:jimm]]

Maybe jfkflame has some ideas, font list related.

Comment 2

[Jamie Nicol [:jnicol]]

Jonathan, this looks similar to bug 1848890. mBlocks could be reallocated when appended to by UpdateShmBlocks(). This is called by the constructor (ie not relevant), and ToPtr(). ToPtr() takes the lock when called off main thread - but is there anything stopping it being called concurrently by the main thread and a dom worker?

Comment 3

[Jonathan Kew [:jfkthame]]

Yeah, that looks like a potential problem. (I guess when this was originally written, we didn't have font usage happening from DOM workers; the only non-main-thread caller would've been stylo, and the main thread is paused during stylo traversal.)

If I'm thinking straight here, we can address this by taking the lock within UpdateShmBlocks() (which is only called in the main-thread case), and that way we can still avoid the cost on the main-thread "happy path".

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 9364950 [details]
Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not readily. The patch adds locking, which implies a possible race issue, but it's unclear how to reliably reproduce it, and the most likely result is a safe crash (though that probably can't be guaranteed)
• 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?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: should backport trivially
• How likely is this patch to cause regressions; how much testing does it need?: minimal risk, just adds a missing lock
• Is Android affected?: Yes

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 9364950 [details]
Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

Approved to land and uplift

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

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

Comment 8

[Phabricator Automation]

Uplift Approval Request

• Risk associated with taking this patch: minimal
• String changes made/needed: none
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• Explanation of risk level: just adds a lock to existing code
• Needs manual QE test: no
• User impact if declined: possible UAF/poisoned-value crashes
• Fix verified in Nightly: no
• Code covered by automated testing: yes

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

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

Comment 10

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: minimal
• String changes made/needed: none
• Fix verified in Nightly: no
• User impact if declined: possible UAF/poisoned-value crash
• Code covered by automated testing: yes
• Needs manual QE test: no
• Explanation of risk level: just adds a lock in existing code

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3dfc3bdc689
Lock the list in UpdateShmBlocks. r=gfx-reviewers,lsalzman

Comment 12

[Jonathan Kew [:jfkthame]]

Backed out in https://hg.mozilla.org/integration/autoland/rev/f7d3d45e1f4616c30fecfdf172da1341435cbb8b for thread-safety-analysis warnings:

/builds/worker/checkouts/gecko/gfx/thebes/SharedFontList.cpp:877:17: error: recursive mutex 'PlatformFontList(#undefined).mLock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
/builds/worker/checkouts/gecko/gfx/thebes/SharedFontList.cpp:887:46: error: releasing recursive mutex 'PlatformFontList(#undefined).mLock' that was not held [-Werror,-Wthread-safety-analysis] 

I'll add the necessary annotations, as we're deliberately not taking the lock on all codepaths here.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcdbc1e5d9a7
Lock the list in UpdateShmBlocks. r=gfx-reviewers,lsalzman

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

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

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

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

Comment 16

[Donal Meehan [:dmeehan]]

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

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9365079 [details]
Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

Approved for 120.0b3

Comment 18

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: no
• User impact if declined: possible UAF/poisoned-value crashes
• Code covered by automated testing: yes
• Needs manual QE test: no
• Explanation of risk level: just adds a lock to existing code
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• String changes made/needed: none
• Risk associated with taking this patch: minimal

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2d0a1875930

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9365080 [details]
Bug 1862777 - Lock the list in UpdateShmBlocks. r=#gfx-reviewers

Approved for 115.6esr.

Comment 21

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: minimal
• String changes made/needed: none
• User impact if declined: possible UAF/poisoned-value crashes
• Fix verified in Nightly: no
• Code covered by automated testing: yes
• Needs manual QE test: no
• Explanation of risk level: just adds a lock to existing code

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/94a397b3882d

Comment 23

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter