Closed Bug 1281965 Opened 3 years ago Closed 2 years ago

Hash table re-entrancy during shutdown with non-ASCII font names

Categories

(Core :: Graphics: Text, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jruderman, Assigned: milan)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

1. Put computer under heavy load
2. Load the testcase
3. Quit

Result:

Assertion failure: IsIdle(oldState), at xpcom/glue/PLDHashTable.h:132

...sometimes.
Attached file stack+
Partial stack:

  * frame #0: XUL`Checker::StartWriteOp
    frame #1: XUL`AutoWriteOp::AutoWriteOp
    frame #2: XUL`AutoWriteOp::AutoWriteOp
    frame #3: XUL`PLDHashTable::Add
    frame #4: XUL`PLDHashTable::Add
    frame #5: XUL`nsTHashtable<nsStringHashKey>::PutEntry
    frame #6: XUL`gfxPlatformFontList::FindAndAddFamilies
    frame #7: XUL`gfxMacPlatformFontList::FindAndAddFamilies
    frame #8: XUL`gfxPlatformFontList::FindFamily
    frame #9: XUL`gfxPlatformFontList::CleanupLoader
    frame #10: XUL`gfxFontInfoLoader::CancelLoader
    frame #11: XUL`gfxFontInfoLoader::ShutdownObserver::Observe
    frame #12: XUL`nsObserverList::NotifyObservers
    frame #13: XUL`nsObserverService::NotifyObservers
    frame #14: XUL`nsAppStartup::Quit

frame 9, gfxPlatformFontList::CleanupLoader, is iterating over mOtherNamesMissed 

frame 6, gfxPlatformFontList::FindAndAddFamilies, adds an entry to mOtherNamesMissed. 

Modifying a PLDHashTable while iterating over it is unsafe. In debug builds, it aborts. In release builds, IIRC, it can lead to the use of dangling pointers, especially if the hash table is resized.
I'm guessing the time-based heuristics in gfxPlatformFontList.cpp are responsible for the intermittency :/
Attachment #8764785 - Attachment mime type: text/html → text/html;charset=UTF-8
(In reply to Jesse Ruderman from comment #1)
>...
> 
> frame 9, gfxPlatformFontList::CleanupLoader, is iterating over
> mOtherNamesMissed 
> 
> frame 6, gfxPlatformFontList::FindAndAddFamilies, adds an entry to
> mOtherNamesMissed. 
> 
> Modifying a PLDHashTable while iterating over it is unsafe. In debug builds,
> it aborts. In release builds, IIRC, it can lead to the use of dangling
> pointers, especially if the hash table is resized.

Didn't think it'd be a problem as we break out of the iteration loop if FindAndAddFamilies adds an entry, but I guess it is.
Flags: needinfo?(jfkthame)
::CleanupLoader does appear to be the only place that needs this different behaviour (don't create if you don't find it), calling through FindFamily() into FindAndAddFamilies().
Comment on attachment 8923591 [details] [diff] [review]
When iterating for deletion, do not add more elements. r=jfkthame

Review of attachment 8923591 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable, except for one small detail... r=me with that fixed.

::: gfx/thebes/gfxPlatformFontList.h
@@ +152,5 @@
>          // family name for a styled variant.
> +        eNoSearchForLegacyFamilyNames = 1 << 1,
> +
> +        // If set, FindAndAddFamilies will not add a missing entry to mOtherNamesMissed
> +        eNoAddToNamesMissedWhenSearching = 1 << 1,

I think you meant "1 << 2" here?
Attachment #8923591 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> ...
> 
> Looks reasonable, except for one small detail

Kind of you calling it a small detail that makes the code not work properly :)
Flags: needinfo?(jfkthame)
I can't recall if sec-moderate needs a security review or not, so here's a request anyway.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really.

Which older supported branches are affected by this flaw? All of them, but this is sec-moderate so I'm not sure how far back we need to go.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backports should be easy.

How likely is this patch to cause regressions; how much testing does it need? Not likely, will receive testing through regular Firefox usage.
Attachment #8923591 - Attachment is obsolete: true
Attachment #8924174 - Flags: sec-approval?
Attachment #8924174 - Flags: review+
https://wiki.mozilla.org/Security/Bug_Approval_Process says you can land a sec-moderate directly; only high or critical are supposed to wait for approval.
Comment on attachment 8924174 [details] [diff] [review]
When iterating for deletion, do not add more elements. Carry r=jfkthame

Yes, as a sec-moderate, this doesn't need sec-approval to land on trunk.
Attachment #8924174 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/a9930291f639

I'm going to assume this is something we don't need to worry about backporting to ESR52, but feel free to set the status to affected and nominate it for approval if you feel strongly otherwise. Same for 57.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: gfx-core-security → core-security-release
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.