Bug 52542 – SVG CSS property types with <number> don't support exponents

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

[Dirk Schulze]

In SVG the type <length> can have exponents: integer ([e|E] interger)? see http://www.w3.org/TR/SVG/types.html#DataTypeNumber
We support this for SVG attributes, but not for SVG CSS properties like 'stroke-width', because we use the CSS parser and that parser doesn't support this.
See attached link for an example.

[Dirk Schulze]

Rob do you want to take a look at this as well? ;-)

[Rob Buis]

Hi Dirk,

(In reply to comment #1)
> Rob do you want to take a look at this as well? ;-)

Good find! This is very interesting to work on, you can assign to me if you are busy with the animation stuff, I can look into it this week.
Cheers,

Rob.

[Dirk Schulze]

(In reply to comment #2)
> Hi Dirk,
> 
> (In reply to comment #1)
> > Rob do you want to take a look at this as well? ;-)
> 
> Good find! This is very interesting to work on, you can assign to me if you are busy with the animation stuff, I can look into it this week.
Sure, I'm happy if you're working on it :-)

[Dirk Schulze]

Have to extend this bug. It's not only <length> that can take exponents, all CSS properties with <number> can take exponents: <angle>, <length>, <frequency>, <icccolor>, <percentage>, <time>. Some of them are defined by CSS3 as well, but without of exponents. Means we have to differ <number> for CSS/HTML and for CSS/SVG during parsing numbers.

CSS3: http://www.w3.org/TR/css3-values/#ltnumbergt
SVG:  http://www.w3.org/TR/SVG/types.html#DataTypeNumber

[Dirk Schulze]

(In reply to comment #4)
> CSS3: http://www.w3.org/TR/css3-values/#ltnumbergt
> SVG:  http://www.w3.org/TR/SVG/types.html#DataTypeNumber

Ouch! Only if the value gets applied to the SVG _attribute_ it must allow exponent numbers, but it should _not_ on setting the property with CSS!

<rect width="100" height="100" stroke-width="1e1px"> is allowed
<rect width="100" height="100" style="stroke-width:1e1px;"> is not allowed.

[Dirk Schulze]

Will upload a fix for that soon. Have it running in a local build. Just needs more testing.

[Dirk Schulze]

Created attachment 134958 [details]
Patch

[Dirk Schulze]

Created attachment 134960 [details]
Patch

Forgot to add original author of the patch to the changelog :P.

[Nikolas Zimmermann]

Comment on attachment 134960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134960&action=review

Nice job, still some caveats, r- for test coverage:

> Source/WebCore/css/CSSParser.cpp:8603
> +            int offset = 1;

why int? I'd use unsigned.

> Source/WebCore/css/CSSParser.cpp:8605
> +            if (m_currentCharacter[offset] == '+') {

Is this safe to access? Is it guaranteed that m_currentCharacter isn't accessed out of boundaries, aka. that 'e' is always followed by another character.

> Source/WebCore/css/CSSParser.cpp:8606
> +                offset++;

++offset.

> Source/WebCore/css/CSSParser.cpp:8609
> +                offset++;

Ditto.

> Source/WebCore/css/CSSParser.cpp:8612
> +            double exponent;

No need for this local here, just declare it in the if (offset > digits) branch directly,

> Source/WebCore/css/CSSParser.cpp:8616
> +                exponent = charactersToDouble(m_currentCharacter + digits, offset - digits);

Can this yield "invalid" doubles, is this clamped somehow? Shall we sanitize the parsed exponent here? (thinking of NaN handling, or Inf)

> LayoutTests/svg/css/script-tests/scientific-numbers.js:13
> +shouldBeEqualToString("document.defaultView.getComputedStyle(text, null).fontSize", "16px");

You can just use "getComputedStyle(text).fontSize".

> LayoutTests/svg/css/script-tests/scientific-numbers.js:232
> +

This needs to be extended;
leading whitespace, trailing whitespace, mid white-space (" 5e10", " 5e1 0", etc. (also including negative values, for both number and exponent).
Arbitary characters inside of exponent, exponents leading to double overflow, etc.

[Dirk Schulze]

Created attachment 134986 [details]
Patch

New approach: Use SVG parser to parse numbers. The logic there is well tested and has a lot of more checks to make sure that we don't miss a case. Added more tests for some edge cases as well as overflow tests. isASCIIDigit() and equal checks prevent us from reading data outside the buffer.

[Nikolas Zimmermann]

(In reply to comment #10)
> Created an attachment (id=134986) [details]
> Patch
> 
> New approach: Use SVG parser to parse numbers. The logic there is well tested and has a lot of more checks to make sure that we don't miss a case. Added more tests for some edge cases as well as overflow tests.
Great, I'll look at thew new patch soon.

> isASCIIDigit() and equal checks prevent us from reading data outside the buffer.
This still worries me. I'll check if the new patch is safer.

[Nikolas Zimmermann]

Comment on attachment 134986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134986&action=review

I have some new comments:

> Source/WebCore/css/CSSParser.cpp:79
> +#if ENABLE(SVG)
> +#include "SVGParserUtilities.h"
> +#endif

I don't think you need to guard this.

> Source/WebCore/css/CSSParser.cpp:8606
> +            if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) {

At this point it's guaranteed that m_currentCharacter exists okay. So dereferencing the UChar* pointer is fine.

> Source/WebCore/css/CSSParser.cpp:8607
> +                if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) {

But who guarantees that the UChar* m_currentCharacter + 1 that you're dereferencing here exists?

The old code passes m_tokenStart to charactersToDouble as const UChar* parameter, and m_currentCharacter - m_tokenStart as size.
I'd strongly suggest to reuse that way of accessing the string.
const UChar* currentCharacter = m_tokenStart;
unsigned length = m_currentCharacter - m_tokenStart;
if (length > 1 && isASCIIAlphaCaselessEqual(*currentCharacter, 'e')) {
// Now its guaranteed that the passed in string is at least 'eX' (e + another item)
}

If I'm missing something, and this is already guaranteed by the CSSParser code, you should at least assert that the length is > 1.

> Source/WebCore/css/CSSParser.cpp:8612
> +                while (isASCIIDigit(m_currentCharacter[offset]))

If the branch above isn't taken that means the first character following the 'e' is not '+', not '-' nor an ascii digit - why do you continue parsing at this point? You could have exited already.

> Source/WebCore/css/CSSParser.cpp:8615
> +            if (!parseSVGNumber(m_tokenStart, m_currentCharacter + offset - m_tokenStart, yylval->number))

This would then read as if (!parseSVGNumber(m_tokenStart, length + offset, yylval->number).

> Source/WebCore/css/CSSParser.cpp:8616
> +                break;

If parseSVGNumber() fails, is yylval->number reset to a specific value? (Not sure about that, that's why I'm asking).

> Source/WebCore/svg/SVGParserUtilities.cpp:152
> +

One newline too much.

> LayoutTests/svg/css/script-tests/scientific-numbers.js:13
> +shouldBeEqualToString("document.defaultView.getComputedStyle(text).fontSize", "16px");

You still use the long form here, getComputedStyle(text).fontSize should work, without the document.defaultView prefix :-)

[Dirk Schulze]

(In reply to comment #12)
> (From update of attachment 134986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134986&action=review
> 
> I have some new comments:
> 
> > Source/WebCore/css/CSSParser.cpp:79
> > +#if ENABLE(SVG)
> > +#include "SVGParserUtilities.h"
> > +#endif
> 
> I don't think you need to guard this.
It is not necessary but prevent including the file on compiling. That was the intention.

> 
> > Source/WebCore/css/CSSParser.cpp:8606
> > +            if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) {
> 
> At this point it's guaranteed that m_currentCharacter exists okay. So dereferencing the UChar* pointer is fine.
> 
> > Source/WebCore/css/CSSParser.cpp:8607
> > +                if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) {
> 
> But who guarantees that the UChar* m_currentCharacter + 1 that you're dereferencing here exists?
That is the code before these lines:

        while (true) {
            if (!isASCIIDigit(m_currentCharacter[0])) {
                // Only one dot is allowed for a number,
                // and it must be followed by a digit.
                if (m_currentCharacter[0] != '.' || dotSeen || !isASCIIDigit(m_currentCharacter[1]))
                    break;
                dotSeen = true;
            }
            ++m_currentCharacter;
        }

So no extra check. The string gets terminated by a \n. Just checked it. Therefore is should be save to check the value for 1, since 0 was no /n.

> 
> The old code passes m_tokenStart to charactersToDouble as const UChar* parameter, and m_currentCharacter - m_tokenStart as size.
> I'd strongly suggest to reuse that way of accessing the string.
> const UChar* currentCharacter = m_tokenStart;
> unsigned length = m_currentCharacter - m_tokenStart;
> if (length > 1 && isASCIIAlphaCaselessEqual(*currentCharacter, 'e')) {
> // Now its guaranteed that the passed in string is at least 'eX' (e + another item)
> }
But just for the first char after the 'e'. Like I said, this is not necessary, since the last char is a \n on the string.

> 
> If I'm missing something, and this is already guaranteed by the CSSParser code, you should at least assert that the length is > 1.
Not necessary.

> 
> > Source/WebCore/css/CSSParser.cpp:8612
> > +                while (isASCIIDigit(m_currentCharacter[offset]))
> 
> If the branch above isn't taken that means the first character following the 'e' is not '+', not '-' nor an ascii digit - why do you continue parsing at this point? You could have exited already.
Well, not exiting. If I break I would break stuff. The unit is included in the token (10em wouldn't work, which gets tested by the new test as well). I could put the while loop in the else tree.

> 
> > Source/WebCore/css/CSSParser.cpp:8615
> > +            if (!parseSVGNumber(m_tokenStart, m_currentCharacter + offset - m_tokenStart, yylval->number))
> 
> This would then read as if (!parseSVGNumber(m_tokenStart, length + offset, yylval->number).
> 
> > Source/WebCore/css/CSSParser.cpp:8616
> > +                break;
> 
> If parseSVGNumber() fails, is yylval->number reset to a specific value? (Not sure about that, that's why I'm asking).
m_token get's not set and therefore the value is invalid.

> 
> > Source/WebCore/svg/SVGParserUtilities.cpp:152
> > +
> 
> One newline too much.
Thanks. Will fix that.

> 
> > LayoutTests/svg/css/script-tests/scientific-numbers.js:13
> > +shouldBeEqualToString("document.defaultView.getComputedStyle(text).fontSize", "16px");
> 
> You still use the long form here, getComputedStyle(text).fontSize should work, without the document.defaultView prefix :-)
That was a misunderstanding. I thought you meant the ", null)" on computed style which I fixed. Will fix that.

[Dirk Schulze]

Created attachment 135000 [details]
Patch

Patch. See previous comment.

[Nikolas Zimmermann]

Comment on attachment 135000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135000&action=review

Next round of comments, poor Dirk :-)

> Source/WebCore/css/CSSParser.cpp:8604
> +            if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) {

Are we sure that at least one element follows the 'e' ?
Can we ASSERT(m_currentCharacter - m_tokenStart > 1) here?
Before accessing: if (m_currentCharacter[1].. below

> Source/WebCore/css/CSSParser.cpp:8612
> +                if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) {
> +                    offset += 2;
> +                    while (isASCIIDigit(m_currentCharacter[offset]))
> +                        ++offset;
> +                    // Use FLOATTOKEN if we have exponents.
> +                    dotSeen = true;
> +                }
> +            }

++m_currentCharacter;
if (*m_currentCharacter == '-' || *m_currentCharacter == '+' || isASCIIDigit(*m_currentCharacter)) {
    ++m_currentCharacter;
    while (isASCIIDigit(*m_currentCharacter))
        ++m_currentCharacter;
    // Use FLOATTOKEN if the string contains exponents.
    dotSeen = true;
}

if (!parseNumber(m_tokenStart, m_currentCharacter, yylval->number, false))
    break;

You'll need a new parseNumber() variant taking a double&, but you can remove the new parseSVGNumber() method that you've added.

> Source/WebCore/svg/SVGParserUtilities.h:36
> +bool parseSVGNumber(UChar*& ptr, size_t length, double& number);

Can be avoided.

> LayoutTests/svg/css/script-tests/scientific-numbers.js:274
> +text.setAttribute("font-size", "50e0.0");
> +shouldBeEqualToString("getComputedStyle(text).fontSize", "16px");

Hard to understand the test, without seeing what you're trying to assign.

I propose following:
function setFontSize(valueString) {
    text.setAttribute("font-size", valueString);
}

function test(valueString, expectedString) {
    debug("text.setAttribute('font-size', '" + valueString + "');");
    setFontSize(valueString);
    shouldBeEqualToString("getComputedStyle(text).fontSize", expectedString);
}

and use:

debug("Valid values");
test("50px", "50px");
test("100px", "100px");

setFontSize("16px");

debug("");
debug("Invalid values");
test("50e0.0", "16px");
test("50e 0", "16px");
....

setFontSize("16px");

debug("");
debug("Next category...");

[Dirk Schulze]

(In reply to comment #15)

Sorry. I am looking at this after a long time.

> if (!parseNumber(m_tokenStart, m_currentCharacter, yylval->number, false))
>     break;
> 

IIRC the problem was that m_tokenStart and m_currentCharacter are of type UChar*, while parseNumber takes const UChar* (and const UChar*&). So I would need a type conversion which does not look pretty.

> You'll need a new parseNumber() variant taking a double&, but you can remove the new parseSVGNumber() method that you've added.

How could I avoid it? Even for parseNumber with a double I would need a new method. I choose parseSVGNumber to indicate from the name that it will be a number that is parsed according to the SVG rules. I still think that this is the proper solution. Especially here where it is not obvious that we call the method in SVGParserUtilities.

> Hard to understand the test, without seeing what you're trying to assign.

Thanks, I'll change the test and the other code suggestions.

[Dirk Schulze]

Created attachment 151721 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 151721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151721&action=review

Looks reasonable, r=me.

> Source/WebCore/ChangeLog:10
> +        The SVG parser is already well tested and has some extra checks for edge cases that prevent
> +        us e.g for overflows.

prevent from overflow, or rather protect from overflow

[Dirk Schulze]

Committed r122978: <http://trac.webkit.org/changeset/122978>

[Tony Chang]

Did you mean for this to be a pixel test?  It looks like including SVGTestCase.js sets "window.enablePixelTesting = true" which causes pixel results to be dumped.

[Dirk Schulze]

(In reply to comment #20)
> Did you mean for this to be a pixel test?  It looks like including SVGTestCase.js sets "window.enablePixelTesting = true" which causes pixel results to be dumped.

No, should be dumpAsText. thanks