Closed Bug 849606 Opened 7 years ago Closed 7 years ago

"ASSERTION: Invalid offset" and crash with textPath and "svg.text.css-frames.enabled" preffed on

Categories

(Core :: SVG, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- unaffected
firefox21 --- disabled
firefox22 + disabled
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-wildptr, sec-high, Whiteboard: currently behind a pref, see bug 839955)

Attachments

(3 files, 2 obsolete files)

Attached image testcase
With:
  user_pref("svg.text.css-frames.enabled", true);

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

###!!! ASSERTION: aPos out of range: 'aPos < GetLength()', file ../../dist/include/gfxFont.h, line 2471

Crash [@ gfxShapedText::CompressedGlyph::IsClusterStart]
Attached file stacks
Jonathan are you the right one to look at this?
Assignee: nobody → jfkthame
heycam would be the obvious person to have look at this, except he's on vacation at the moment. 

The next-obvious would be longsonr, since he's been working on improving this "svg.text.css-frames.enabled"-dependent code while heycam's been away.

longsonr, any chance you've got cycles to take this? :)
I don't yet know how to fix it. I was hoping that fixing other bugs would help but I'm just don't quite know enough yet. I'll keep trying.
This can't be sec-critial as it's preffed off though, must be only sec-high.
Keywords: sec-criticalsec-high
longsonr says this affects Aurora **if** you set the pref, and we chatted briefly on IRC about strategies for preventing this bug there.

My impression is that we don't actually need to worry about Aurora (which will eventually become beta and then release), because it's preffed off there -- only users who've manually toggled the pref in about:config would be at risk.  We could e.g. yank out this code, or forcibly prevent people from setting the pref & turn off all the tests that depend on being able to set the pref, if we need to, but I don't think we normally go to those measures for pref-dependent security bugs -- right?  (dveditz or dbolter, could you sanity-check my assessment there?)
Flags: needinfo?(dveditz)
Summary: "ASSERTION: Invalid offset" and crash with textPath → "ASSERTION: Invalid offset" and crash with textPath and "svg.text.css-frames.enabled" preffed on
(Also, reassigning to longsonr to reflect reality, since I believe he's working a fix for this on trunk, and jfkthame was only assigned as a guess.)
Assignee: jfkthame → longsonr
Attached patch patch (obsolete) — Splinter Review
So broadly speaking display="none" elements should work the same as if they don't exist.

The first problem is that when CharIterator::CharIterator is constructed mTextElementCharIndex is initialised but mGlyphStartTextElementCharIndex is not. 

Once I fixed that the crashtest then crashes in TextRenderedRunIterator::Next instead as it adds the 2 undisplayed characters to the index on every loop rather than just adding them once.

With those fixed we run into the reftest actually being wrong as it expects <text x="1 2 3 4 5 6">ab<tspan display="none">cd</tspan>ef</text> to use 5 and 6 as the locations of ef rather than 3 and 4. The other lines in the testcase are correct. Opera's rendering matches ours with this patch.
Attachment #725775 - Flags: review?(dholbert)
Thanks for looking at this while I'm away, Robert.  A quick comment while I'm online:

My intention was for display:none characters in the DOM to still consume the positions specified in x="" etc.  IIRC browsers were not consistent in this, and I think it makes sense to limit the effect of style property changes on which position attribute values correspond to which DOM characters.  (We still have white-space changing this mapping of position attribute values to DOM characters, but that should be the only one.)
back to the drawing board then :-(
Attachment #725775 - Flags: review?(dholbert)
Attached patch updated patch (obsolete) — Splinter Review
This one passes the svg/text reftests without changing them. Including the ones that check that display: none text still consumes positioning attributes.
Attachment #725775 - Attachment is obsolete: true
Attachment #725853 - Flags: review?(dholbert)
Attachment #725853 - Attachment is obsolete: true
Attachment #725853 - Flags: review?(dholbert)
Attachment #725854 - Flags: review?(dholbert)
Comment on attachment 725854 [details] [diff] [review]
right patch this time

I don't really know how these iterators are supposed to work & what the various edge-cases involved are, but it looks like jwatt reviewed this code (for bug 655877), so I suspect he can review this more efficiently & effectively than I can.  Kicking review over to him.
Attachment #725854 - Flags: review?(dholbert) → review?(jwatt)
Attachment #725854 - Flags: review?(jwatt) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)
> My impression is that we don't actually need to worry about Aurora (which
> will eventually become beta and then release), because it's preffed off
> there -- only users who've manually toggled the pref in about:config would
> be at risk.

That's right. If the feature is preffed-off we can mark that release "disabled" and not worry about it (which I've done for Firefox 21). I marked Firefox 22 as "affected" assuming you're trying to land bug 839955 in that time-frame. There's not much time left, however, so if you know it's not landing go ahead and mark that one "disabled" too.

(In reply to Robert Longson from comment #5)
> This can't be sec-critical as it's preffed off though

That rationale is intended for optional features that will always require a configuration change to trigger. This feature is intended to be standard behavior once it fully lands, and you should treat this bug as the severity it would be when it's landed as intended.

Do you ever write into these text runs? In the code you're patching it looks like you're only reading. If the worst case is incorporating random chunks of memory into displayed text we'd usually call that sec-hight (data leakage) at most. If we go wrong trying to read a more complex object it could sec-critical.
Blocks: 839955
Flags: needinfo?(dveditz)
Whiteboard: currently behind a pref, see bug 839955
Correct, we're not writing into the text runs here.

I doubt we we'll land bug 839955 for Firefox 22 as I think it's likely we'd want to fix more of the svgtext dependent bugs first and I suspect heycam and I can't complete that before Firefox 22 development ends.
This patch also fixes bug 842630. I'll close that as fixed once this lands on central.
https://hg.mozilla.org/mozilla-central/rev/3550c269a565
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I tried again and it seems that this doesn't fix bug 842630 after all.
Any plans to prioritize this for this cycle? FF22 (the first affected version, according to status flags) is being released in 5 weeks.
Flags: needinfo?(longsonr)
This is preffed off so I didn't think it necessary.
Flags: needinfo?(longsonr)
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.