970710 - "ASSERTION: Invalid offset" in gfxSkipChars with MathML, LRO

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 13

Regression from bug 955957.

Might be related to bug 961859.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jonathan Kew [:jfkthame]]

Are you sure this is a regression? I tried backing out bug 955957 locally, and still hit the same assertion (it just moved to a new source line number):

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 61

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

I confess I don't really understand this code, but it seems like we're calling GetTrimmedOffsets from SetupJustificationSpacing with a default aPostReflow parameter, which doesn't match the explicit value used in InitializeForMeasure. Is that OK, or should the two calls be consistent? This patch prevents us hitting the assertion here, and doesn't break anything that I can see on tryserver... https://tbpl.mozilla.org/?tree=Try&rev=c5c2e99f3b96.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

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

This is a simple bug fix basically. InitializeForMeasure is called during reflow, so we should be passing false for aPostReflow in GetTrimmedOffsets.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not readily, afaics. The error occurs during measurement, not frame/textrun construction, so it seems unlikely to lead directly to an out-of-bounds write or anything like that; the risk is only that we do an out-of-bounds read, which might result in a crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, it seems far from obvious to me. The one clue is that "text-align:justify" may somehow be relevant, but that alone doesn't lead anywhere interesting.

Which older supported branches are affected by this flaw? mozilla-28 and later

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? patch applies cleanly on current mozilla-aurora and mozilla-beta

How likely is this patch to cause regressions; how much testing does it need? risk looks minimal to me

Comment 6

[Jonathan Kew [:jfkthame]]

The relevant code here was introduced in bug 415413; marking affected Firefox versions accordingly. I'm not sure exactly how that translates to b2g versions...

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

sec-approval+.

Please nominate the patch for Aurora and Beta after it goes in (assuming there are no issues) so we can avoid shipping the issue.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2430a5e4379

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Al Billings [:abillings] from comment #7)
> Please nominate the patch for Aurora and Beta after it goes in (assuming
> there are no issues) so we can avoid shipping the issue.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 415413

User impact if declined: potential crash (though not obvious whether it might be further exploitable)

Testing completed (on m-c, etc.): verified locally to avoid the inconsistency that may lead to out-of-bounds read; landed on m-c without problems

Risk to taking this patch (and alternatives if risky): minimal; simple bug-fix

String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/905799ec8a3f
https://hg.mozilla.org/releases/mozilla-beta/rev/cc9e070dd294

Comment 13

[Jesse Ruderman]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Are you sure this is a regression? I tried backing out bug 955957 locally,
> and still hit the same assertion (it just moved to a new source line number):
> 
> ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file
> gfx/thebes/gfxSkipChars.cpp, line 61

I see what happened. The assertion in comment 2, but not the assertion in comment 0, is in my ignore list (due to bug 847242). I ran my bisect tool in a way that used the ignore list, so it blamed the changeset that changed which assertion fired.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/cc9e070dd294

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assert on Fx29, 2014-02-10.
Verified fixed on Fx29, Fx30, 2014-03-25.

Comment 16

[Mats Palmgren (inactive)]

Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f7d8b2ca94

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/91f7d8b2ca94