420243 - getSubStringLength calls can crash FireFox 3b3

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Eric Seidel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-us) AppleWebKit/523.10.5 (KHTML, like Gecko) Version/3.0.4 Safari/523.10.6
Build Identifier: 

getSubStringLength calls can crash FireFox 3b3

I have not attempted to reduce the bug.  This file (when placed inside the WebKit LayoutTests directory) will crash FireFox 3b3 (and Opera 9.5b) on load!

I assume it's related to the getSubStringLength calls.  We autogenerate HTML wrapper files to load these .js test files as part of our testing suite.


var svgNS = "http://www.w3.org/2000/svg";

var svgRoot = document.createElementNS(svgNS, "svg");
document.documentElement.appendChild(svgRoot);

var svgText = document.createElementNS(svgNS, "text");
svgText.style.fontFamily = "Ahem";
svgText.style.fontSize = "20px";
svgText.appendChild(document.createTextNode("abc"));
svgRoot.appendChild(svgText);

shouldThrow("svgText.getSubStringLength(-1, 2)");
shouldThrow("svgText.getSubStringLength(-1, 0)");
shouldThrow("svgText.getSubStringLength(1, 3)");
shouldThrow("svgText.getSubStringLength(0, 4)");
shouldThrow("svgText.getSubStringLength(3, 0)");

shouldBe("svgText.getSubStringLength(0, 0)", "0");
shouldBe("svgText.getSubStringLength(2, 0)", "0");

shouldBe("svgText.getSubStringLength(0, 1)", "20");
shouldBe("svgText.getSubStringLength(1, 1)", "20");
shouldBe("svgText.getSubStringLength(2, 1)", "20");
shouldBe("svgText.getSubStringLength(0, 3)", "60");

// We treat -1 as (unsigned)-1, thus its just a really big number, resulting in an error
shouldThrow("svgText.getSubStringLength(1, -1)");
shouldThrow("svgText.getSubStringLength(2, -1)");
shouldThrow("svgText.getSubStringLength(3, -1)");
shouldThrow("svgText.getSubStringLength(3, -3)");

// cleanup
document.documentElement.removeChild(svgRoot);

var successfullyParsed = true;



Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[x0]

Attachment: testcase (crashes!)

getSubStringLength( charnum, nchars ) seems to crash
iff nchars == -1 and 0 < charnum < number of chars

Comment 2

[x0]

The problem seems to be at
http://lxr.mozilla.org/seamonkey/source/layout/svg/base/src/nsSVGTextContainerFrame.cpp#190

|charnum + nchars| can overflow.

Comment 3

[x0]

BTW, why should getSubStringLength(3, 0) throw for <text>abc</text> ?
The spec says so, but for getSubStringLength(0, 3) too:

| INDEX_SIZE_ERR: Raised if the charnum is negative or if charnum+nchars is
| greater than or equal to the number of characters at this node.

Seems to be an error in the spec.

Comment 4

[Eric Seidel]

We decided getSubStringLength(0, 3) was an error in the spec, but getSubStringLength(3, 0) seemed OK to throw.  Look at selectSubString for a more sane rule:

INDEX_SIZE_ERR: Raised if the charnum is negative or if charnum is greater than or equal to the number of characters at this node.

However, the idea of any of these numbers ever being "negative" is bogus, since they're defined as unsigned longs, thus (at least in c++) that would mean the compiler/interpreter would silently convert them to unsigned values.  We chose to delay the conversion and throw if < 0 for charnum, we also explicitly check charnum + nchars for overflow to catch the nchars < 0 case.

Comment 5

[x0]

The rule for selectSubString doesn't really look sane to me either. There's no good reason to disallow a zero length at the end of the string only.

Note that there is an (unanswered) recent request for clarification on the w3c svg list: http://lists.w3.org/Archives/Public/www-svg/2008Feb/0051.html

Comment 6

[Jeff Walden [:Waldo]]

Yeah, easy crash.  Once past the check you start getting into out-of-bounds fairly quickly in the textrun code and gfx.  Below, glyphData starts out fine, then it reads past the end and starts getting garbage that quickly causes it to go astray, in this case for me into 344 and so on to crash fairly quickly.

GetAdvanceForGlyphs (aTextRun=0x3625f5a0, aStart=1, aEnd=4) at /Users/jwalden/moz/trunk/mozilla/gfx/thebes/src/gfxFont.cpp:340
337         const gfxTextRun::CompressedGlyph *glyphData = aTextRun->GetCharacterGlyphs() + aStart;
338         PRInt32 advance = 0;
339         PRUint32 i;
340         for (i = aStart; i < aEnd; ++i, ++glyphData) {
341             if (glyphData->IsSimpleGlyph()) {
342                 advance += glyphData->GetSimpleAdvance();   
343             } else {
344                 PRUint32 glyphCount = glyphData->GetGlyphCount();
345                 const gfxTextRun::DetailedGlyph *details = aTextRun->GetDetailedGlyphs(i);
346                 PRUint32 j;
347                 for (j = 0; j < glyphCount; ++j, ++details) {
348                     advance += details->mAdvance;
349                 }
350             }
351         }
352         return advance;

Comment 7

[Eric Seidel]

(In reply to comment #5)
> The rule for selectSubString doesn't really look sane to me either. There's no
> good reason to disallow a zero length at the end of the string only.
> 
> Note that there is an (unanswered) recent request for clarification on the w3c
> svg list: http://lists.w3.org/Archives/Public/www-svg/2008Feb/0051.html

I'm OK implementing it either way.  We should get the w3c to clarify, then hopefully make WebKit, Moz + Opera agree. :)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Patch

With this patch |text.getSubStringLength(textLength, 0)| throws; changing this is a matter of changing <= in the patch to < and changing the (3, 0) test to expect 0.  I tend to think (3, 0) should just return 0, but I'm nowhere near invested enough in this to care much one way or another (certainly not enough to go to the SVG list).  SVG's not my baby; this was just low-hanging fruit and scary out-of-bounds memory access.

Having Ahem available for the lengths would be nice, but I think the way I did it works just as well and is just as correct.

Please review the SVG tests carefully; I've never written even an attribute of SVG before in my life.  :-)

Comment 9

[Robert Longson [:longsonr]]

Can you give the test a proper file name e.g. text-getSubStringLength-01.svg and stick it in /mozilla/layout/reftests/svg. The stuff in the bugs subdirectory is crashtests and even that does not belong there ;-)

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Better reftest names, don't forget the reftest.list change

Comment 11

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

Wouldn't this make more sense as a mochitest?

Thanks for filing this Eric, much appreciated.

Comment 12

[Jeff Walden [:Waldo]]

Possibly; I don't know how the SVG file would communicate its results out to the Mochitest window, tho.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: As Mochitests

It's rather odd the location I used for the tests in here doesn't already exist.

Comment 14

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

Comment on attachment 306953 [details] [diff] [review]
As Mochitests

Looks good. You didn't really need to put the SVG in an IFRAME though, you could have put it inline.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 306953 [details] [diff] [review]
As Mochitests

Fixes a buffer overflow, always good stuff...

Comment 16

[Daniel Veditz [:dveditz]]

Is it really a buffer overflow? I didn't dig into what GetSubStringLengthNoValidation() does but I would imagine from the functionality this is an out-of-bounds read which would be much less dangerous.

Functionality doesn't seem to exist on the 1.8 branch

Comment 17

[Jeff Walden [:Waldo]]

Ah, sorry -- hadn't realized that terminology referred explicitly to writing past the end of a buffer and didn't include reading past the end.

Comment 18

[Robert Longson [:longsonr]]

(In reply to comment #13)
> It's rather odd the location I used for the tests in here doesn't already
> exist.
> 

You are the first writer of an SVG mochitest!

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306953 [details] [diff] [review]
As Mochitests

Will consider after beta 4.

Comment 20

[Robert Sayre]

(In reply to comment #18)
> 
> You are the first writer of an SVG mochitest!

Lies! the gECBN tests include a document that contains XUL, XHTML, and SVG. :)

Comment 21

[Robert Longson [:longsonr]]

I stand corrected.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306953 [details] [diff] [review]
As Mochitests

a1.9=beltzner

Comment 23

[Jeff Walden [:Waldo]]

Fixed on trunk.  Commit message was vaguely obfuscatory, but it's sort of pointless as realistically it's not going to take anyone who looks at the commit any effort to figure out what the problem was and how to trigger it.

This patch preserves the text.getSubStringLength(text.getNumberOfChars(), 0) throwing behavior the spec currently wants, in line with WebKit behavior; this will have to be changed if the spec ever changes.  As noted, this is basically a one-liner plus a test change.

Comment 24

[Jeff Walden [:Waldo]]

So we released a beta with the fix for this in place, stable releases were never affected, and it wasn't obvious that anything bad (other than a crash or bogus return value) could happen due to the buffer overread.  Is there a good reason not to unhide this bug now?

Comment 25

[Jeff Walden [:Waldo]]

Unhiding for the reasons in comment 24, not to mention that the beta with the fix has been superseded by a major release with the fix.