[Webkit-unassigned] [Bug 25645] SVG - numeric overflow for very large elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25645


W. James MacLean <wjmaclean at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53973|0                           |1
        is obsolete|                            |
  Attachment #53974|0                           |1
        is obsolete|                            |




--- Comment #22 from W. James MacLean <wjmaclean at chromium.org>  2010-05-06 07:52:04 PST ---
Created an attachment (id=55237)
 --> (https://bugs.webkit.org/attachment.cgi?id=55237)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.