557736 - gfxFontGroup::ForEachFontInternal mis-parses font-family lists with consecutive commas

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Stuart Cook]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.18) Gecko/2010021501 Ubuntu/9.04 (jaunty) Firefox/3.0.18
Build Identifier: 

If a font-family-list string passed to gfxFontGroup::ForEachFont contains two consecutive commas, the second comma will be treated as part of the following font family name.

For example, if the string is "a,,serif", it will be split into "a" and ",serif".

This is bad because under normal circumstances nsRuleNode::SetFont will add ",serif" (or ",sans-serif") to the end of the font list it encounters, to ensure that the list contains at least one valid font name. If a <font> element or font-family CSS rule contains a trailing comma, the resulting family list can potentially contain no valid font names.

In practice this is offset by the fact that each of the platform-specific font group classes tries pretty hard to never end up with an empty font list, but for custom embeddings/ports that don't do this the result can be a crash.

Reproducible: Always

Steps to Reproduce:
1. Place a breakpoint in gfxFontGroup::ForEachFontInternal
2. Visit a page containing <span style="font-family: bogus\,;">...</span>
3. Step through the code and observe the values of string variable "family".
Actual Results:  
The family strings produced are "bogus" and ",serif".

Expected Results:  
The family strings should be "bogus" and "serif".

Comment 1

[Stuart Cook]

Attachment: Example page triggering the mis-parse

Comment 2

[Stuart Cook]

On my system (WinXP, FF 3.6.3), the two spans in the test page are rendered in two different fonts.

I originally found this bug when it crashed a custom embedding of mozilla-1.9.0, but the relevant code seems to be unchanged in mozilla-central.

Comment 3

[Stuart Cook]

Attachment: Example page using <font> elements

Bugzilla was so clever that it stripped out the protective backslash needed to protect the trailing comma. Hopefully this <font>-based test page should be more robust.

Comment 4

[Stuart Cook]

Attachment: Screenshot of test page in FF 3.6.3 WinXP

Both elements should look identical, but here they are rendered using different fonts.

The first element doesn't attempt to trigger the bug, and renders in "serif". The second triggers the bug and renders in a sans-serif font, which I suspect is the last-ditch default font added in gfxWindowsFontGroup::InitFontList.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1 - ignore empty elements in the font family list

Seems like we could fix this trivially by skipping any leading commas, just like skipping inter-fontname spaces.

Comment 6

[Stuart Cook]

For any people like me who need to fix this in pre-1.9 versions, the corresponding change needs to be made in nsFont::EnumerateFamilies in gfx/src/nsFont.cpp instead.

(Actually that code is still in mozilla-central, but I don't know if it's used anywhere.)

Comment 7

[John Daggett (:jtd)]

Comment on attachment 437626 [details] [diff] [review]
patch, v1 - ignore empty elements in the font family list

Looks good but I would add a small reftest for this.

Comment 8

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/997626d0f0f3

I'll add a reftest as a followup.

Comment 9

[Jonathan Kew [:jfkthame]]

Pushed reftest based on the testcase in attachment 437499 [details].
http://hg.mozilla.org/mozilla-central/rev/d0d05d2cdcec