Bug 25645 – SVG - numeric overflow for very large elements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

If an element has a very big width or height of 1000000000000000000 (on qt more than ~500,000,000,000,000,000) webkit gets a overflow and tell us that width/height is negative.

[Dirk Schulze]

Created attachment 30132 [details]
SVG with a big sized rect

A test case for the overflow.

[Dirk Schulze]

It looks like the biggest number for real values is 2^31 -1 don't know why we just support 31bit numbers.

[David Yonge-Mallo]

Created attachment 51467 [details]
Patch to use floating point in SVG number parser

I've made a little patch and test case for this.  The patch prevents the overflow (positive into negative and negative into positive) at the parser.  This doesn't solve the problem entirely since the number returned by the parser may now be too big for the later rendering code, but it's a first step.

This bug appears to be the cause of http://crbug.com/825.

[Nikolas Zimmermann]

Hi David,

did you notice the -expected.txt results don't make much sense? Can you investigate the cause?
123456789 is correct, and afterwards it's broken...

 5     RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] [color=#0000FF]}] [data="M-1000.00,12345679395506094080.00 L200.00,200.00"]
 6     RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] [color=#00FF00]}] [data="M600.00,400.00 L1000.00,-98765435164048752640.00"]

CC'ing Darin, who might have more comments here!

[David Yonge-Mallo]

(In reply to comment #4)
> Hi David,
> 
> did you notice the -expected.txt results don't make much sense? Can you
> investigate the cause?
> 123456789 is correct, and afterwards it's broken...
> 
>  5     RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID]
> [color=#0000FF]}] [data="M-1000.00,12345679395506094080.00 L200.00,200.00"]
>  6     RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID]
> [color=#00FF00]}] [data="M600.00,400.00 L1000.00,-98765435164048752640.00"]
> 
> CC'ing Darin, who might have more comments here!

The results are like that because of FloatType's (lack of) precision.  The numbers are 1.234568e+19 and 9.876543e+19.  When these are formatted for printing (using a format string of "%.2f"), that is how they appear.  I checked the output on Mac and Linux, and perhaps surprisingly they are the same.  

If we want to prettify these up, we'll have to change the dump render tree code, which will involve different changes on Mac and Linux (since the Linux one uses Skia).

On the other hand, we could go to a higher precision representation of floats.  Changing the definition of FloatType, however, will have repercussions beyond just this bug.  The question is then whether around 7 decimal digits of precision (float) is enough, and if switching to double (around 16 digits) is worth it.  It's probable that we will rarely even need 7 significant digits on numbers sufficiently large that precision becomes a problem.  But that's a separate (though related) bug from overflow.

[David Yonge-Mallo]

By the way, the bug can be "solved" entirely by clamping the output from the parser to std::limits::max()-1 and std::limits::min()+1.  With the above suggested patch, the renderer will fail to display anything because the numbers are too big, but if the clamping hack is put in, it will display something that visually appears to be correct (because the coordinates are so big they are effectively the same from the view port).  This "fix" is of course a bad idea as transforms may later push the coordinates beyond the limits of integer storage again.  However, you may want to put such code in for debugging purposes.

[David Yonge-Mallo]

(In reply to comment #5)
> I checked the output on Mac and Linux, and perhaps surprisingly they are 
> the same.  

The reason I chose 123... and 987... is because they're easy to spot while debugging.  When I noticed the output was different than the input, I thought about using other numbers, but any number large enough to cause integer overflow is also large enough to cause floating point imprecision.  For example, 1000... changes to 999... and so on, and at least with the current choice of numbers the first few digits remain the same.

[David Yonge-Mallo]

Problems with large number output precision are also discussed here: https://bugs.webkit.org/show_bug.cgi?id=23927

[Simon Fraser (smfr)]

There's also some discussion of large number round-tripping through CSS in bug 20674.

[Simon Fraser (smfr)]

Comment on attachment 51467 [details]
Patch to use floating point in SVG number parser



>  template <typename FloatType> static bool _parseNumber(const UChar*& ptr, const UChar* end, FloatType& number, bool skip)
>  {
> -    int integer, exponent;
> -    FloatType decimal, frac;
> +    int exponent;
> +    FloatType integer, decimal, frac;

I'm not sure I agree with this. Why not use 'long long' for integer and exponent?

[David Yonge-Mallo]

(In reply to comment #10)
> (From update of attachment 51467 [details])
> > 
> >  template <typename FloatType> static bool _parseNumber(const UChar*& ptr, const UChar* end, FloatType& number, bool skip)
> >  {
> > -    int integer, exponent;
> > -    FloatType decimal, frac;
> > +    int exponent;
> > +    FloatType integer, decimal, frac;
> 
> I'm not sure I agree with this. Why not use 'long long' for integer and
> exponent?

The test case in the patch actually overflows 'long long' as well, so it will return the wrong result.

Also, I'm not sure that 'long long' for the exponent is realistic (even though we should aim to handle anything).  Even for enormous numbers, the exponent is relatively small (fits into an int).

[Simon Fraser (smfr)]

I'd argue that dimensions that overflow a long long are not going to be encountered in normal content.
Agreed on the size of exponent.

[Eric Seidel (no email)]

I wonder if ConformingHighQualitySVGViewers: http://www.w3.org/TR/SVG11/conform.html#ConformingHighQualitySVGViewers
has any relation to anything here.

[David Yonge-Mallo]

(In reply to comment #12)
> I'd argue that dimensions that overflow a long long are not going to be
> encountered in normal content.
> Agreed on the size of exponent.

Nothing is really gained by using long long for the representation of the integer portion of the coordinate.  I think any precision gained in doing so is lost in the subsequent conversion back into FloatType.

(In reply to comment #13)
> I wonder if ConformingHighQualitySVGViewers:
> http://www.w3.org/TR/SVG11/conform.html#ConformingHighQualitySVGViewers
> has any relation to anything here.

The relevant part would be: "Both a Conforming High-Quality Static SVG Viewer and a Conforming High-Quality Dynamic SVG Viewer must support the following additional features: ... At least double-precision floating point computation on coordinate system transformation numerical calculations."

Does WebKit aim to be a Conforming "High-Quality" SVG Viewer?  If so, then FloatType needs to be double rather than float, which would affect many things other than this bug.

[W. James MacLean]

Created attachment 53973 [details]
SVG file of tan(), all values in range of int

[W. James MacLean]

Are there any further thoughts on David's proposed patch? It seems like a reasonable approach, and it would be nice to get a fix for the _parseNumber() function, which is clearly broken at present.

That being said, there are further issues, as it is easy to create a sample SVG file that works with the current parser (and even stays in the range of 'int'), but still fails to render in the current release of WebKit under Chrome (see attached).

[W. James MacLean]

Created attachment 53974 [details]
SVG of tan() function, int's only

Sorry, here's the attachment.

[Nikolas Zimmermann]

Simon, can you recomment given you had concerns about the patch?

Very busy atm, but we should work towards resolving the issues :-)

[WebKit Commit Bot]

Comment on attachment 51467 [details]
Patch to use floating point in SVG number parser

Rejecting patch 51467 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12715 test cases.
svg/custom/mask-excessive-malloc.svg -> failed

Exiting early after 1 failures. 10198 tests run.
245.44s total testing time

10197 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
10 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1821054

[Nikolas Zimmermann]

Davi, can you have a look at the patch? It fails with commit queue.

[W. James MacLean]

(In reply to comment #20)
> Davi, can you have a look at the patch? It fails with commit queue.

I've taken over this bug from David. I'm looking at the failed test case now to see where the problem lies.

[W. James MacLean]

Created attachment 55237 [details]
Revised patch to fix SVG number parsing, large number handling, fix 2 layout test outputs

Here is a revised patch for discussion.

I apologize for the length of this post, but there are several inter-related issues to be considered. I would appreciate feedback on whether the proposed patch is reasonable (at least in the short term), and thoughts on the larger issue of unsafe float-to-int conversions elsewhere in the code base.

*** Issue: The previous patch failed a layout test (svg/custom/mask-excessive-malloc.svg)

*** Summary of Discussion points:

1) The layout tests 

svg/custom/mask-excessive-malloc.svg
svg/custom/pattern-excessive-malloc.svg 

both appear to have errors in their expected output files. 

2) The revised patch handles this test now (with the corrected layout test).

3) The original patch would also have failed svg/custom/pattern-excessive-malloc.svg. The revised patch passes the corrected version of this layout test.

4) Similar float-to-int conversion errors exist elsewhere in WebKit.

*** Detailed Discussion Points:

1) It appears two of the svg layout tests have errors in their expected outputs.

platform/mac/svg/custom/mask-excessive-malloc-expected.txt show a RenderPath object (child of RenderSVGResourceMasker) with a size of 0x0. I believe this should actually be 800x600 (the cause for this will be discussed in (2)). Note that in the similar case of pattern-excessive-malloc.svg, the RenderPath size is 800x600.

platform/mac/svg/custom/pattern-excessive-malloc-expected.txt shows multiple point coordinates of 1215752192.00 when the correct value should be 99999997952.00 .

In the original file (pattern-excessive-malloc.svg) the coordinate value is 100000000000. This value can be exactly represented by a double, but the closest representation as a float is 99999997952. The value 1215752192 arises when we truncate the leading bytes of the integer representation.

The proposed patch includes corrections for both expected output files.

2) The reason the original patch failed is that, although 2147483647 cannot be represented exactly by float, the closest representable value of 2147483648 will not be obtained when building digit-by-digit using float.

This proposed patch uses double rather than float for building the representation, which is then cast to float. While similar problems could also occur with double, fixing that would require a complete overhaul of the number parser.

3) The original patch also failed on pattern-excessive-malloc.svg, in that its render tree dump shows a RenderPath with size 0x0 (not unlike the expected output file for mask-excessive-malloc.txt). This occurs due to an incorrect conversion from int to float in FloatRect::enclosingIntRect(). The problem arises when doing

float Xf = <value larger than 2147483647>
int Xi = static_cast<int>(Xf); // now Xi = -2147483648

When the negative value is returned, later processing detects a rect with negative height & width, and changes them to 0.

A simple, although not entirely elegant, fix is to merely check to see if the value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the value to the max representable by an int). Similarly for values below -2147483647.

4) While hunting down this bug, I noticed at least two other places where unsafe float-to-int conversions where taking place. The proposed patch does not address these, although they should be fixed. It seems likely that a concerted effort would uncover more problematic conversions.

The constructor IntRect::IntRect(FloatRect &) makes the same float-to-int conversion error as does FloatRect::enclosingIntRect().

In SVGRenderStyle::inflateForShadow(FloatRect &) internal computations are performed by (i) converting the FLoatRect to an IntRect, (ii) making the required changes, (iii) storing the result back into the FloatRect. This incurs the same error as mentioned above.

[Simon Fraser (smfr)]

(In reply to comment #22)

> 1) It appears two of the svg layout tests have errors in their expected
> outputs.
...
> The proposed patch includes corrections for both expected output files.

Sounds good.

> 2) The reason the original patch failed is that, although 2147483647 cannot be
> represented exactly by float, the closest representable value of 2147483648
> will not be obtained when building digit-by-digit using float.
> 
> This proposed patch uses double rather than float for building the
> representation, which is then cast to float. While similar problems could also
> occur with double, fixing that would require a complete overhaul of the number
> parser.

Looks reasonable.

> 3) The original patch also failed on pattern-excessive-malloc.svg, in that its
> render tree dump shows a RenderPath with size 0x0 (not unlike the expected
> output file for mask-excessive-malloc.txt). This occurs due to an incorrect
> conversion from int to float in FloatRect::enclosingIntRect(). The problem
> arises when doing
> 
> float Xf = <value larger than 2147483647>
> int Xi = static_cast<int>(Xf); // now Xi = -2147483648
> 
> When the negative value is returned, later processing detects a rect with
> negative height & width, and changes them to 0.
> 
> A simple, although not entirely elegant, fix is to merely check to see if the
> value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the
> value to the max representable by an int). Similarly for values below
> -2147483647.

Please file a new bug on this issue.

> 4) While hunting down this bug, I noticed at least two other places where
> unsafe float-to-int conversions where taking place. The proposed patch does not
> address these, although they should be fixed. It seems likely that a concerted
> effort would uncover more problematic conversions.
> 
> The constructor IntRect::IntRect(FloatRect &) makes the same float-to-int
> conversion error as does FloatRect::enclosingIntRect().
> 
> In SVGRenderStyle::inflateForShadow(FloatRect &) internal computations are
> performed by (i) converting the FLoatRect to an IntRect, (ii) making the
> required changes, (iii) storing the result back into the FloatRect. This incurs
> the same error as mentioned above.

Please file another new bug on these instances.

[Darin Adler]

(In reply to comment #23)
> (In reply to comment #22)
> > 2) The reason the original patch failed is that, although 2147483647 cannot be
> > represented exactly by float, the closest representable value of 2147483648
> > will not be obtained when building digit-by-digit using float.
> > 
> > This proposed patch uses double rather than float for building the
> > representation, which is then cast to float. While similar problems could also
> > occur with double, fixing that would require a complete overhaul of the number
> > parser.
> 
> Looks reasonable.

The same issue can exist with numbers at the extreme range of what double can represent, and in the case of double using a larger floating point number is not practical. One way to fix this is to build digit by digit in the opposite direction after counting digits. This is what the constant mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h are written to deal with. We might want to take a similar approach for float.

[W. James MacLean]

(In reply to comment #24)
> 
> The same issue can exist with numbers at the extreme range of what double can
> represent, and in the case of double using a larger floating point number is
> not practical. One way to fix this is to build digit by digit in the opposite
> direction after counting digits. This is what the constant
> mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h
> are written to deal with. We might want to take a similar approach for float.

I agree that a right-to-left parsing of the number is appropriate. It's also more effort, so the questions in my mind are:

1) Should we implement a double-based short-term fix before doing a full-blown parser (based on the notion that we will do the full-blown parser, but it will take longer)? 

and

2) In the full blown parser, what rules are appropriate for those numbers that cannot be represented (too large, too much precision, etc.)?

I would think any compiler has standard code to solve this type of problem.

[W. James MacLean]

(In reply to comment #23)
> > A simple, although not entirely elegant, fix is to merely check to see if the
> > value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the
> > value to the max representable by an int). Similarly for values below
> > -2147483647.
> 
> Please file a new bug on this issue.

If we file this as a separate bug, I presume we would then resolve the new bug before fixing bug 25645? The proposed fixes for 25645 won't pass the layout tests without a fix for this issue.

[Simon Fraser (smfr)]

(In reply to comment #26)
> If we file this as a separate bug, I presume we would then resolve the new bug
> before fixing bug 25645?

It's fine for it to be "Depends on" this bug. That's what those fields are for.

[W. James MacLean]

(In reply to comment #24)
> 
> The same issue can exist with numbers at the extreme range of what double can represent, and in the case of double using a larger floating point number is not practical. One way to fix this is to build digit by digit in the opposite direction after counting digits. This is what the constant mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h are written to deal with. We might want to take a similar approach for float.

I'm happy enough to implement code for SVGParserUtilities that is equivalent to what's in JSGlobalObjectFunctions if that's what you and Simon feel is best. Just let me know if this is what you prefer.

I'm curious ... what would happen if 'Inf'/'-Inf' float values started propagating into WebKit?

[Adam Barth]

Comment on attachment 51467 [details]
Patch to use floating point in SVG number parser

Clearing Simon Fraser's R+ from this patch because it appears to cause a test failure.

Please upload a single patch that includes all the changes that need to landed at once so that we don't break anything.

[W. James MacLean]

Created attachment 57913 [details]
Revised patch to fix SVG number parsing of large numbers

Revised patch.

This patch updates two expected results in the layout tests. I have modified the number parsing to parse the integer portion from right-to-left, allowing an as-accurate-as-possible representation using FloatType instead of double.

This should pass the layout tests for Mac. The Windows/Linux chromium layout tests may break until an upstream Skia patch (changelist submitted) is committed.

[Nikolas Zimmermann]

Hi W. James,

can you mark your patch for review and set the "Patch" flag, otherwhise I can't officially review it. Please obsolete the old patches, it's hard to find out which one is valid :-)

Cheers,
Niko

[WebKit Review Bot]

Attachment 57913 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGParserUtilities.cpp:83:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/svg/SVGParserUtilities.cpp:85:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/svg/SVGParserUtilities.cpp:88:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/svg/SVGParserUtilities.cpp:89:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/svg/SVGParserUtilities.cpp:92:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/FloatRect.cpp:116:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/FloatRect.cpp:118:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/FloatRect.cpp:126:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/FloatRect.cpp:127:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 9 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[W. James MacLean]

Done ... I've asked David to mark his patch as obsolete.

Cheers,

James

(In reply to comment #31)
> Hi W. James,
> 
> can you mark your patch for review and set the "Patch" flag, otherwhise I can't officially review it. Please obsolete the old patches, it's hard to find out which one is valid :-)
> 
> Cheers,
> Niko

[Nikolas Zimmermann]

Comment on attachment 57913 [details]
Revised patch to fix SVG number parsing of large numbers

Hi W. James, almost r+. Your patch is missing a proper ChangeLog (use prepare-Changelog --bug=25645, to generate a template to be filled out).
The style bot also reported issues regarding trailing spaces that need to be fixed.Some more comments:

WebCore/platform/graphics/FloatRect.cpp:124
 +      float lf = floorf(rect.x());
Please use descriptive variable names like "left" "right" "top" "bottom" here.

WebCore/platform/graphics/FloatRect.cpp:126
 +      float wf = ceilf(rect.right()) - lf ;
Why do we need to use right - left here to figure out the width? Why is using width() not sufficient?

WebCore/svg/SVGParserUtilities.cpp:83
 +      const UChar *ptr2 = ptr ;
Wrong style: you need to use "const UChar*" (star next to type). Also "ptr2" is not a good name, please use longer descriptive names.

Waiting your new patch :-)
I guess you tested running all webkit tests - enclosingIntRect() is used in lots of places - and I am afraid of side-effects in tests. Please rule that out.

[W. James MacLean]

(In reply to comment #34)
> (From update of attachment 57913 [details])
> 
> WebCore/platform/graphics/FloatRect.cpp:124
>  +      float lf = floorf(rect.x());
> Please use descriptive variable names like "left" "right" "top" "bottom" here.

I will improve these names ... I just re-used the existing names.

> WebCore/platform/graphics/FloatRect.cpp:126
>  +      float wf = ceilf(rect.right()) - lf ;
> Why do we need to use right - left here to figure out the width? Why is using width() not sufficient?
> 

I think we need 'right - left' since we want an enclosing rect, and 

ceil(right) - floor(left) 

is not going to equal 

round/ceil/floor (right - left)

in general (where width = right - left).

> Waiting your new patch :-)
> I guess you tested running all webkit tests - enclosingIntRect() is used in lots of places - and I am afraid of side-effects in tests. Please rule that out.

I believe I am running the full set of tests, but will verify.

Cheers,

James

[W. James MacLean]

Created attachment 58066 [details]
Revised (v3) patch to fix SVG number parsing of large numbers

Revised as per Nikolas' suggestions, although kept "right-left" as opposed to "width" as per my comment #35. Re-added David's test cases.

[Nikolas Zimmermann]

Comment on attachment 58066 [details]
Revised (v3) patch to fix SVG number parsing of large numbers

Almost there, sorry for forcing you to another iteration: LayoutTests is also missing a ChangeLog.
A last suggestion: Can you just name the variables "left" / "top" / etc. in enclosingIntRect(), we don't use type prefixes in variable names like "int iFoo" / "float fFoo" in WebKit.
Can you land on your own, or do you need cq+ after I set r+ on the new patch?

[Nikolas Zimmermann]

Comment on attachment 58066 [details]
Revised (v3) patch to fix SVG number parsing of large numbers

Oops, hit Submit too fast, some more comments:
Please don't change the order in the ChangeLog, the "Bug description" and bug number fields should stay in their current location. Your text needs to go below them. So:

Reviewed by NOBODY (OOPS!).

SVG - numeric overflow
https://...

Revised to improve...

Another style issue (no idea why the style bot did not complain):

WebCore/svg/SVGParserUtilities.cpp:83
 +      const UChar *ptrStartIntPart = ptr;
"const UChar* ptrStartIntPart = ptr";

[Nikolas Zimmermann]

Comment on attachment 58066 [details]
Revised (v3) patch to fix SVG number parsing of large numbers

Oops, hit Submit too fast, some more comments:
Please don't change the order in the ChangeLog, the "Bug description" and bug number fields should stay in their current location. Your text needs to go below them. So:

Reviewed by NOBODY (OOPS!).

SVG - numeric overflow
https://...

Revised to improve...

Another style issue (no idea why the style bot did not complain):

WebCore/svg/SVGParserUtilities.cpp:83
 +      const UChar *ptrStartIntPart = ptr;
"const UChar* ptrStartIntPart = ptr";

WebCore/svg/SVGParserUtilities.cpp:88
 +        const UChar *ptrScanIntPart = ptr - 1;
const UChar* ptrScanIntPart = ptr - 1;

[W. James MacLean]

Created attachment 58072 [details]
Revised (v4) patch to fix SVG number parsing of large numbers

- fixed var names in enclosingIntRect()
- fixed 'UChar*' style issue
- ChangeLog files fixed (for some reason my git commit comments are ending up ahead of bug number, etc., but I fixed this manually)

"or do you need cq+ after I set r+ on the new patch?"

I assume I'll need 'cq+' - I'm not sure of the exact steps to finish landing the patch ...

[Nikolas Zimmermann]

Comment on attachment 58072 [details]
Revised (v4) patch to fix SVG number parsing of large numbers

Hi W. James,

almost there! Your WebCore/ChangeLog contains unrelated entries though, you need to remove them, before I can set r+ and cq+. Setting cq+ will auto-land the patch for you.
I don't think you need to mention twice that you've revised the variable naming. Just make it one statement.

Thanks for your patience,
Niko

[W. James MacLean]

Created attachment 58167 [details]
Revised (v5) patch to fix SVG number parsing of large numbers

Fixed change log issues.

[Nikolas Zimmermann]

Comment on attachment 58167 [details]
Revised (v5) patch to fix SVG number parsing of large numbers

Looks good, r=me. Thanks for fixing this bug!

[WebKit Commit Bot]

Comment on attachment 58167 [details]
Revised (v5) patch to fix SVG number parsing of large numbers

Rejecting patch 58167 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
e
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19073 test cases.
svg/custom/mask-excessive-malloc.svg -> failed

Exiting early after 1 failures. 16523 tests run.
316.95s total testing time

16522 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
15 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3176104

[W. James MacLean]

Created attachment 58370 [details]
Revised (v6) patch to fix SVG number parsing of large numbers

Previous patch forgot to change two test outputs that were previously discussed (Comment #22) but I missed putting into the patch submission (sorry!)

[Nikolas Zimmermann]

Comment on attachment 58370 [details]
Revised (v6) patch to fix SVG number parsing of large numbers

Patch looks good. I made up my mind regarding safeFloatToInt, I find it rather dangerous to hardcode these values. How about using:
static inline int safeFloatToInt(float x)
{
    static const int s_intMax = std::numeric_limit<int>::max();
    static const int s_intMax = std::numeric_limit<int>::min();

    if (x >= float(intMax) + 1.0f)
        return intMax;
    if (x < float(intMin)
        return intMin; // NOTE: You've returned intMin + 1, I don't know why intMin, is not allowed. But if you see errors with intMin, just return intMin+1
    return static_cast<int>(x);
}

Sorry for bringing up this so late :(

[W. James MacLean]

Created attachment 58470 [details]
Revised (v7) patch to fix SVG number parsing of large numbers

Agreed about using the values from numeric_limits ... I should have done this in the first place :-)

[WebKit Review Bot]

Attachment 58470 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/FloatRect.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[W. James MacLean]

Created attachment 58472 [details]
Revised (v8) patch to fix SVG number parsing of large numbers

Alphabetised includes in FloatRect.cpp.

[Nikolas Zimmermann]

Comment on attachment 58472 [details]
Revised (v8) patch to fix SVG number parsing of large numbers

Hi W. James, still r-, sorry:

ChangeLog order is wrong again. Your comments have to go below the bug number & description, just have a look at WebCore/ChangeLog to see how it works.
Also the ChangeLog entries are confusing. You've basically added a line for every new patch you've uploaded here, instead you should only the describe the "final patch"

> +        Revised safeIntToFloat() to use values from <limits>.
> +
> +        Fixed two test outputs involving large coordinate values.
> +
> +        Two of the expected test outputs were incorrect now that parsing of large values
> +        is handled correctly.
> +
> +        Revised to improve variable names in enclosingIntRect(), _parseNumber(), and fix style issues.
> +
> +        - Revised FloatRect to remove bad float-to-int conversions in enclosingIntRect()
> +        - Revised _parseNumber to do right-to-left float-based parsing of input value

Just use:

> +        Two of the expected test outputs were incorrect now that parsing of large values
> +        is handled correctly.
> +        - Revised FloatRect to remove bad float-to-int conversions in enclosingIntRect()
> +        - Revised _parseNumber to do right-to-left float-based parsing of input value


> +static inline int safeFloatToInt(float x)
> +{
> +    static int intMax = std::numeric_limits<int>::max();
> +    static int intMin = std::numeric_limits<int>::min();

Recently the naming of these statics has been discussed on webkit-dev: use s_intMax, s_intMin here.

Other than those last two issues, it's r+ :-)
You must hate me by now, sorry for the numerous of reiterations ;-)

[W. James MacLean]

Created attachment 58476 [details]
Revised (v9) patch to fix SVG number parsing of large numbers

Fixed changelogs, change static var names.

[Nikolas Zimmermann]

Comment on attachment 58476 [details]
Revised (v9) patch to fix SVG number parsing of large numbers

Looks great, r=me! Thanks for taking the time to fix it properly.

[WebKit Commit Bot]

Comment on attachment 58476 [details]
Revised (v9) patch to fix SVG number parsing of large numbers

Rejecting patch 58476 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
e
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19084 test cases.
svg/custom/mask-excessive-malloc.svg -> failed

Exiting early after 1 failures. 16534 tests run.
335.48s total testing time

16533 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
15 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3220222

[W. James MacLean]

(In reply to comment #53)
> (From update of attachment 58476 [details])
> Rejecting patch 58476 from commit-queue.
> 
> svg/custom/mask-excessive-malloc.svg -> failed
> 
> Exiting early after 1 failures. 16534 tests run.
> 335.48s total testing time

Is there any way I can see the actual output that failed? I've tested this case myself, and it passed, so I need more info to see what's going on.
 
> Full output: http://webkit-commit-queue.appspot.com/results/3220222

This link has no more info than the comment I'm replying to :-(

[Nikolas Zimmermann]

(In reply to comment #54)
> (In reply to comment #53)
> > (From update of attachment 58476 [details] [details])
> > Rejecting patch 58476 from commit-queue.
> > 
> > svg/custom/mask-excessive-malloc.svg -> failed
> > 
> > Exiting early after 1 failures. 16534 tests run.
> > 335.48s total testing time
> 
> Is there any way I can see the actual output that failed? I've tested this case myself, and it passed, so I need more info to see what's going on.
> 
> > Full output: http://webkit-commit-queue.appspot.com/results/3220222
> 
> This link has no more info than the comment I'm replying to :-(

Hm, can you find someone @ Google with access to Safari to run the test? I probably can't test it for another two days... if you still need someone, drop me a line then.

Cheers,
Niko

[Eric Seidel (no email)]

Comment on attachment 58167 [details]
Revised (v5) patch to fix SVG number parsing of large numbers

Cleared Nikolas Zimmermann's review+ from obsolete attachment 58167 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[W. James MacLean]

I'd like to re-submit this patch, but I'm a bit confused about expected output files. It seems the expected output from some tests is a bit different on mac than other platforms: do I then need to provide expected output for *all* other platforms? (gtk, chromium-linux, ... there are 19 platforms other than 'mac' listed in the Layout/platform directory). What's the best way to handle this?

[Simon Fraser (smfr)]

(In reply to comment #57)
> I'd like to re-submit this patch, but I'm a bit confused about expected output files. It seems the expected output from some tests is a bit different on mac than other platforms: do I then need to provide expected output for *all* other platforms?

No. You can provide cross-platform expected results, then override those with platform-specific results when necessary (e.g. in LayoutTests/platform/mac/...).

[W. James MacLean]

Created attachment 62859 [details]
Revised (vA) patch to fix SVG number parsing of large numbers

Revised to update GTK and QT expected outputs to match those for Chromium.

[W. James MacLean]

Created attachment 62866 [details]
Revised (vB) patch to fix SVG number parsing of large numbers

Previous patch missing binary info.

[WebKit Review Bot]

Attachment 62866 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/FloatRect.cpp:128:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/graphics/FloatRect.cpp:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/graphics/FloatRect.cpp:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/graphics/FloatRect.cpp:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[W. James MacLean]

Created attachment 62869 [details]
Revised (vC) patch to fix SVG number parsing of large numbers

Fixed spaces in FloatRect.cpp.

[Simon Fraser (smfr)]

Comment on attachment 62869 [details]
Revised (vC) patch to fix SVG number parsing of large numbers

> +static inline int safeFloatToInt(float x)
> +{
> +    static int s_intMax = std::numeric_limits<int>::max();
> +    static int s_intMin = std::numeric_limits<int>::min();

Is caching in global data really worth it? std::numeric_limits<int>::max() may just 'return MAX_INT' which is much faster than accessing global data.

[Darin Adler]

(In reply to comment #63)
> (From update of attachment 62869 [details])
> > +static inline int safeFloatToInt(float x)
> > +{
> > +    static int s_intMax = std::numeric_limits<int>::max();
> > +    static int s_intMin = std::numeric_limits<int>::min();
> 
> Is caching in global data really worth it? std::numeric_limits<int>::max() may just 'return MAX_INT' which is much faster than accessing global data.

Should just be static const int, and then it will not involve any global data at all in the compilers we use.

[W. James MacLean]

Created attachment 62940 [details]
Revised (vD) patch to fix SVG number parsing of large numbers

Made int vars 'const' in safeFloatToInt().

[Simon Fraser (smfr)]

Comment on attachment 62940 [details]
Revised (vD) patch to fix SVG number parsing of large numbers

They are still static, so I think this is still a global memory access. Maybe you can disassemble to find out.

[Darin Adler]

(In reply to comment #66)
> They are still static, so I think this is still a global memory access. Maybe you can disassemble to find out.

I can disassemble and did, and it is not a global memory access.

[Nikolas Zimmermann]

Comment on attachment 62940 [details]
Revised (vD) patch to fix SVG number parsing of large numbers

r-, ChangeLogs are missing. And some additional issues:

WebCore/svg/SVGParserUtilities.cpp:70
 +        ++ptr; // Advance to first non-digit.
Four-spaces indention please.

WebCore/svg/SVGParserUtilities.cpp:73
 +        const UChar* ptrScanIntPart = ptr - 1;
Ditto.

WebCore/svg/SVGParserUtilities.cpp:76
 +          integer = integer + multiplier * static_cast<FloatType>(*(ptrScanIntPart--) - '0');
Ditto. And you can use integer += multipler * ... here.

[W. James MacLean]

Created attachment 63063 [details]
Revised (vE) patch to fix SVG number parsing of large numbers

Fixed spacing, re-added CL entries

[Nikolas Zimmermann]

Comment on attachment 63063 [details]
Revised (vE) patch to fix SVG number parsing of large numbers

Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results.

[WebKit Commit Bot]

Comment on attachment 63063 [details]
Revised (vE) patch to fix SVG number parsing of large numbers

Rejecting patch 63063 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 63063, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=63063&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=25645&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 63063 from bug 25645.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[W. James MacLean]

Created attachment 63096 [details]
Revised (vF) patch to fix SVG number parsing of large numbers

Added reviewer line to LayoutTests/Changelog (sigh).

[Nikolas Zimmermann]

Comment on attachment 63096 [details]
Revised (vF) patch to fix SVG number parsing of large numbers

2nd try :-)

[WebKit Commit Bot]

Comment on attachment 63096 [details]
Revised (vF) patch to fix SVG number parsing of large numbers

Clearing flags on attachment: 63096

Committed r64379: <http://trac.webkit.org/changeset/64379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64379 might have broken GTK Linux 32-bit Release, GTK Linux 64-bit Debug, and Qt Linux Release

[Nikolas Zimmermann]

(In reply to comment #70)
> (From update of attachment 63063 [details])
> Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results.

James, please have a look at:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html
http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html

Both Qt/Gtk have problems, bots are red.

[Nikolas Zimmermann]

(In reply to comment #77)
> (In reply to comment #70)
> > (From update of attachment 63063 [details] [details])
> > Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results.
> 
> James, please have a look at:
> http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html
> http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html
> 
> Both Qt/Gtk have problems, bots are red.

Same with the windows bot:
http://build.webkit.org/results/Windows%20Release%20(Tests)/r64419%20(2158)/svg/custom/massive-coordinates-pretty-diff.html

Please look into the problem ASAP.

[W. James MacLean]

This looks to be just a re-baselining issue for Qt/GTK/Windows ... I'll re-submit with updated -expected.txt files for this test on Tuesday morning when I'm back in the office. I'm assuming these are the only broken tests?

(In reply to comment #78)
> (In reply to comment #77)
> > (In reply to comment #70)
> > > (From update of attachment 63063 [details] [details] [details])
> > > Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results.
> > 
> > James, please have a look at:
> > http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html
> > http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html
> > 
> > Both Qt/Gtk have problems, bots are red.
> 
> Same with the windows bot:
> http://build.webkit.org/results/Windows%20Release%20(Tests)/r64419%20(2158)/svg/custom/massive-coordinates-pretty-diff.html
> 
> Please look into the problem ASAP.

[Simon Fraser (smfr)]

I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea.

[Dirk Schulze]

(In reply to comment #80)
> I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea.

Brings us to platform independent LayoutTests.. Something we could do, once SVGPath uses SVGPathByteStream.

[W. James MacLean]

I'm happy enough to revise the tests, but I'm not sure how we would go about making it insensitive to rounding/precision errors. Isn't that sort of dictated by using DumpRenderTree? If there's a way to devise a different style of test that doesn't use DumpRenderTree and someone can aim me at an example, I can look after it.

Otherwise, I'll proceed with re-baselining the tests according to the output from the Qt/GTK/Win bots.


(In reply to comment #80)
> I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea.

[W. James MacLean]

Created attachment 63464 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win.

This patch re-baselines the expected output for LayoutTests/svg/custom/massive-coordinates.svg for platform/win. The change appears to just be a numeric precision/round-off issue.

This patch also removes the expected output for LayoutTests/svg/custom/pattern-excessive-malloc.svg for the GTK platform, as the current expected output is *very broken*. This test, along with that for LayoutTests/svg/custom/massive-coordinates.svg needs to be re-baselined for GTK, but I think it is likely better not to test against a known bad -expected.txt file in the meantime. Since all other platforms seem to be in close agreement, I suspect this is a debugging-output issue in GTK.

Both tests expectations for Qt were re-baselined in https://bugs.webkit.org/show_bug.cgi?id=43361 (thanks Martin Robinson!).

[W. James MacLean]

Created attachment 63584 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK.

Revised to skip pattern-excessive-malloc.svg test on GTK.

[Nikolas Zimmermann]

Comment on attachment 63584 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK.

r-, you need to modify platform/gtk/Skipped, not test_expectations.txt

[W. James MacLean]

Created attachment 63587 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK.

Moved 'skip' request to platform/gtk/Skipped.

[Nikolas Zimmermann]

Comment on attachment 63587 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK.

r=me.

[Eric Seidel (no email)]

Comment on attachment 58476 [details]
Revised (v9) patch to fix SVG number parsing of large numbers

Cleared Nikolas Zimmermann's review+ from obsolete attachment 58476 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 63063 [details]
Revised (vE) patch to fix SVG number parsing of large numbers

Cleared Nikolas Zimmermann's review+ from obsolete attachment 63063 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 63587 [details]
Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK.

Clearing flags on attachment: 63587

Committed r64815: <http://trac.webkit.org/changeset/64815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/8777860>

[Vincent Danen]

This was assigned CVE-2011-0147.