1825569 - [tsan] data races in Android font backend (FT2FontEntry)

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

+++ This bug was initially created as a clone of Bug #1824200 +++

Although we don't have tsan reports from Android, or known crashes occurring, examination of the Android font backend shows that it has similar data-race issues to those fixed for Linux in bug 1824200.

Therefore, I propose to apply equivalent fixes to the gfxFT2FontList backend, and I think we should uplift them so that the Android fix ships at the same time as the Linux one.

(I guess it's possible that currently-unexplained crashes such as bug 1825417 could be related to the potential data races here, though we don't have direct evidence of that for now.)

Comment 1

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high bug that is a variation of something we're fixing in 112.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1825569 - Atomics/locking in FT2FontEntry. r=lsalzman

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 9326035 [details]
Bug 1825569 - Atomics/locking in FT2FontEntry. r=lsalzman

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily, I guess - the fact that we're adding locking and atomics suggests a thread-related issue, but it's unclear how to reliably trigger a data race, or to exploit it if it happens.
• 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?: 105+
• 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 be trivial
• How likely is this patch to cause regressions; how much testing does it need?: Low risk - Android version of fixes applied to the Linux backend in bug 1824200 (and verified by tsan there)
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: Potential data races in font code, could result in crashes, wild-ptr access, or similar
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: No known STR
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Similar fixes successfully landed (and checked with tsan) in Linux backend
• String changes made/needed: none
• Is Android affected?: Yes

Comment 4

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jfkthame, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 5

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

The bug is marked as tracked for firefox112 (beta) and tracked for firefox113 (nightly). However, the bug still has low severity.

:bhood, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 6

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9326035 [details]
Bug 1825569 - Atomics/locking in FT2FontEntry. r=lsalzman

sec approved to land today; Relman said they would uplift it next week after it bakes over the weekend

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I was looking at the logcat for a debug test run and saw a lot of logspew like this:

03-31 15:12:40.440  3624  3639 I Gecko   : [Child 3624, Main Thread] WARNING: Potential deadlock detected:
03-31 15:12:40.440  3624  3639 I Gecko   : Cyclical dependency starts at
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : gfxFontEntry lock
03-31 15:12:40.440  3624  3639 I Gecko   : Next dependency:
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : SharedFTFace::mLock (currently acquired)
03-31 15:12:40.440  3624  3639 I Gecko   : Cycle completed at
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : gfxFontEntry lock
03-31 15:12:40.440  3624  3639 I Gecko   : Deadlock may happen for some other execution

Is that cause for concern?

Comment 8

[Jonathan Kew [:jfkthame]]

Hmm, will take a look - thanks.

Comment 9

[Andrew McCreight [:mccr8]]

FWIW, I've locally seen something similar to that for awhile. (on OSX)

Comment 10

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

Atomics/locking in FT2FontEntry. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/2a1fc04f9a8cee1868ccd5096ea89317578053eb
https://hg.mozilla.org/mozilla-central/rev/2a1fc04f9a8c

Comment 11

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9326035 [details]
Bug 1825569 - Atomics/locking in FT2FontEntry. r=lsalzman

Approved for 112.0rc1

Comment 12

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9326035 [details]
Bug 1825569 - Atomics/locking in FT2FontEntry. r=lsalzman

switching flag since the merge to release already happened

Comment 13

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

https://hg.mozilla.org/releases/mozilla-release/rev/3a27c8622dba

Comment 14

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: advisory.txt

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to [PTO until 26-June] Ryan VanderMeulen [:RyanVM] from comment #7)

I was looking at the logcat for a debug test run and saw a lot of logspew like this:

03-31 15:12:40.440  3624  3639 I Gecko   : [Child 3624, Main Thread] WARNING: Potential deadlock detected:
03-31 15:12:40.440  3624  3639 I Gecko   : Cyclical dependency starts at
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : gfxFontEntry lock
03-31 15:12:40.440  3624  3639 I Gecko   : Next dependency:
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : SharedFTFace::mLock (currently acquired)
03-31 15:12:40.440  3624  3639 I Gecko   : Cycle completed at
03-31 15:12:40.440  3624  3639 I Gecko   : Mutex : gfxFontEntry lock
03-31 15:12:40.440  3624  3639 I Gecko   : Deadlock may happen for some other execution

Is that cause for concern?

That looks like bug 1835110, which was fixed a little while ago.