Crash [@ nsUserFontSet::CheckFontLoad]

RESOLVED FIXED in Firefox 27, Firefox OS v1.1hd

Status

()

Core
Graphics: Text
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: jfkthame)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla29
x86_64
Mac OS X
crash, csectype-uaf, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 wontfix, firefox27 verified, firefox28 verified, firefox29 verified, firefox-esr2427+ verified, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [adv-main27+][adv-esr24.3+])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8347177 [details]
testcase

With:
  user_pref("security.fileuri.strict_origin_policy", false);

About half the time, the testcase crashes [@ nsUserFontSet::CheckFontLoad].
(Reporter)

Comment 1

4 years ago
Created attachment 8347178 [details]
stack
(Reporter)

Comment 2

4 years ago
I guess the testcase has to be local for the timing to be right to cause the crash.
(Assignee)

Comment 3

4 years ago
The script here does:

  function boom()
  {
    var canvas = document.createElement('canvas');
    var ctx = canvas.getContext('2d');
    ctx.font = 'normal 20px x'; // "x" is a user font family

    document.getElementById("s").remove(); // remove stylesheet that defined font family "x"

    setTimeout(function() {
       ctx.measureText("A");
    }, 50);
  }

At the point where ctx.font is set, there is a user font set in existence, and so the gfxFontGroup that's created and stored in the ContextState of the CanvasRenderingContext2D will have a reference to that font set. Then the stylesheet is removed, which causes the user font set to be deleted - but the font group held by the canvas context's state doesn't know this.

What is the expected behavior here, anyhow - given the script above, what font is the measureText call supposed to use? Should setting a user font as the current font on a canvas context cause it to stick around for subsequent use by that context, even if the rule that defined it is subsequently changed or removed? Or is ctx.font supposed to (in effect) be dynamically resolved each time it's used, so that changes to the user font set are reflected in canvas text behavior without needing to explicitly reset ctx.font?

cc'ing jdaggett and dbaron for any insights. Do you know whether the specs make it clear what's expected here, or is this an ill-defined area?
(Assignee)

Comment 4

4 years ago
Marking security-sensitive, as I suspect this could probably be manipulated from page script to least cause use-after-free errors; not sure what the consequences might be, but best to assume the worst.
Group: core-security
(Assignee)

Comment 5

4 years ago
Created attachment 8347323 [details] [diff] [review]
use nsRefPtr for user font set references in fontgroup and prescontext

Possible patch: the idea here is that gfxFontGroup as well as nsPresContext needs to hold a strong reference to the user font set, and when there's an update, the prescontext shouldn't directly call Destroy() on the old set but just release its reference, in case there are live font groups that also have references to it. AFAICT, this resolves the crashes with the testcase here. However, as far as I understand from http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#text-styles, I think our behavior is wrong per spec: if the user font is no longer defined in the style context, then the canvas shouldn't be using it ("if the font style source node does not have that font in scope at the time the font is to be used, then it must be treated as if it was an unknown font, falling back to another"). Fixing that will be a separate matter.
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
(Assignee)

Comment 6

4 years ago
While the patch above seems to prevent the crash here, it's leading to a large leak reported by Debug Crashtest runs. (Which doesn't entirely surprise me; we're probably ending up with a cycle somewhere that needs to be broken.)
Assignee: jfkthame → nobody
Component: Graphics: Text → Graphics: Layers
(Reporter)

Updated

4 years ago
Assignee: nobody → jfkthame
Component: Graphics: Layers → Graphics: Text

Comment 7

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> What is the expected behavior here, anyhow - given the script above, what
> font is the measureText call supposed to use? Should setting a user font as
> the current font on a canvas context cause it to stick around for subsequent
> use by that context, even if the rule that defined it is subsequently
> changed or removed? Or is ctx.font supposed to (in effect) be dynamically
> resolved each time it's used, so that changes to the user font set are
> reflected in canvas text behavior without needing to explicitly reset
> ctx.font?
> 
> cc'ing jdaggett and dbaron for any insights. Do you know whether the specs
> make it clear what's expected here, or is this an ill-defined area?

While there isn't any explicit text in the HTML5 spec to deal with this specific sequence, I think the effect of the stylesheet deletion should be immediate, anything that depends on it should be rebuilt.  So in this case measureText("A") uses 'font-family: X' and since X is no longer an available font it will end up using the default font.  Same if canvas.style.fontFeatureSettings is changed before the call to measureText, the ContextState needs to be updated and the new ContentState used for measureText.
(Assignee)

Updated

4 years ago
Depends on: 950590
(Assignee)

Comment 8

4 years ago
I've filed bug 950590 about the incorrect behavior, as that seems like something worth tracking in its own right, separately from the crash here (although fixing it should also resolve the crash, AFAICS).
(Assignee)

Comment 9

4 years ago
This is fixed by bug 950590; the patch here should no longer be needed.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-b2g18: --- → affected
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox29: --- → fixed
status-firefox-esr24: --- → affected
Flags: in-testsuite?
Target Milestone: --- → mozilla29
status-b2g18: affected → fixed
status-b2g-v1.1hd: affected → fixed
status-b2g-v1.2: affected → fixed
status-firefox-esr24: affected → fixed
Confirmed crash on FF28, 2013-11-13.
Verified fixed on 24esr, FF27, FF28 and FF29, 2014-01-18.
status-firefox27: fixed → verified
status-firefox28: fixed → verified
status-firefox29: fixed → verified
status-firefox-esr24: fixed → verified
tracking-firefox-esr24: --- → 27+
Whiteboard: [adv-main27+][adv-esr24.3+]
Keywords: csectype-uaf, sec-high
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a55b2d51bf
Group: core-security
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/70a55b2d51bf
You need to log in before you can comment on or make changes to this bug.