Bug 28938 – CSS string value is not correctly serialized when it contains binary characters

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yuta Kitamura]

When WebKit serializes a CSS string value, it does not escape binary characters in that value.

For example, suppose that we have an element with the following style:
    font-family: '\0\1\2';
When we obtain this style using JavaScript (like element.style.fontFamily), it becomes an invisible string, instead of the correctly escaped string. WebKit should escape these characters as specified in CSS spec.

[Yuta Kitamura]

Created attachment 38979 [details]
Escape control characters in CSS string value when it is serialized.

When WebKit serializesa CSS string value that contains binary characters
('\0\1\2' for example), it did not escape these characters. As a result,
users got (invisible) control characters through scripts, which was not good.
This change fixes this issue.

As a side effect, two separate codes for escaping CSS strings are merged, and
become a public function (escapeCSSString).
---
 7 files changed, 185 insertions(+), 52 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 38979 [details]
Escape control characters in CSS string value when it is serialized.

This would be much better as a js test/dom only test:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

YOu'd get shouldBeEqualToString for fre, instead of having to make your own pass/fail functions.

This also doesn't need to be run from onload, you can run this from an inline script tag (which making this a js test will do automatically).

No argument names needed here since tehy don't add clarity:
6 String quoteCSSString(const String& string);
 217 String quoteCSSStringIfNeeded(const String& string);
 218 String quoteCSSURLIfNeeded(const String& string);

[Yuta Kitamura]

Thank you for your review, Eric. I'm going to try again.

[Yuta Kitamura]

Created attachment 39050 [details]
Escape control characters in CSS string value when it is serialized.

When WebKit serializesa CSS string value that contains binary characters
('\0\1\2' for example), it did not escape these characters. As a result,
users got (invisible) control characters through scripts, which was not good.
This change fixes this issue.

As a side effect, two separate codes for escaping CSS strings are merged, and
become a public function (escapeCSSString).
---
 8 files changed, 144 insertions(+), 53 deletions(-)

[Yuta Kitamura]

Comment on attachment 39050 [details]
Escape control characters in CSS string value when it is serialized.

Oops, I overlooked the first part of Eric's comment. Let me do this again.

[Yuta Kitamura]

Created attachment 39051 [details]
Escape control characters in CSS string value when it is serialized.

When WebKit serializesa CSS string value that contains binary characters
('\0\1\2' for example), it did not escape these characters. As a result,
users got (invisible) control characters through scripts, which was not good.
This change fixes this issue.

As a side effect, two separate codes for escaping CSS strings are merged, and
become a public function (escapeCSSString).
---
 9 files changed, 151 insertions(+), 53 deletions(-)

[Yuta Kitamura]

Created attachment 39052 [details]
Escape control characters in CSS string value when it is serialized.

When WebKit serializesa CSS string value that contains binary characters
('\0\1\2' for example), it did not escape these characters. As a result,
users got (invisible) control characters through scripts, which was not good.
This change fixes this issue.

As a side effect, two separate codes for escaping CSS strings are merged, and
become a public function (escapeCSSString).
---
 9 files changed, 156 insertions(+), 53 deletions(-)

[Yuta Kitamura]

Sorry for uploading patches again and again (I should have been more careful).

I converted the test to a JS test, and fixed some issues as suggested in Eric's comment.

[Eric Seidel (no email)]

Comment on attachment 39052 [details]
Escape control characters in CSS string value when it is serialized.

I think this will be much slower than the old implementation.  We should at least use StringBuilder instead of String::append() because String::append() will make a new buffer every time. :(

In general the change looks OK, assuming it does not cause a PLT regression.

Could we pre-allocate the length of the new string all at once instead of doing so many mallocs?  StringBuilder will be better, but still slower than the old replace code was I think.

[Eric Seidel (no email)]

Vector<UChar> also would have better append behavior than String does.

[Simon Fraser (smfr)]

It would be nice to move forward on this.

[Yuta Kitamura]

(In reply to comment #11)
> It would be nice to move forward on this.

This patch slipped off from my mind long ago... I'll continue.

[Yuta Kitamura]

Created attachment 52372 [details]
Patch v5.

[Darin Adler]

Comment on attachment 52372 [details]
Patch v5.

Great work. It's good to handle these cases correctly.

Looking at the tokenizer it seems that tab characters are legal in a CSS quoted string. But we are choosing to escape them as "\9". I guess that's OK.

> +            String escape = String::format("\\%x", static_cast<int>(ch));
> +            for (unsigned j = 0; j < escape.length(); ++j)
> +                buffer[index++] = escape[j];

This is excessive memory allocation and complexity to convert a two digit number to hex.

Instead something like this:

    static const char hexDigits[17] = "0123456789ABCDEF";
    buffer[index++] = '\\';
    if (ch >= 0x10)
        buffer[index++] = hexDigits[ch >> 4];
    buffer[index++] = hexDigits[ch & 0xF];

> +            // Space characeter may be required to separate backslash-escape sequence and normal characters.

Typo: "characeter".

These functions don't belong in CSSPrimitiveValue.h. I think they should move to CSSParser.h.

review- because I'd like to see the code changed to not use String::format.

[Yuta Kitamura]

Created attachment 52516 [details]
Patch v6 (Applying darin's comment).

[Yuta Kitamura]

(In reply to comment #15)
> Created an attachment (id=52516) [details]
> Patch v6 (Applying darin's comment).

I think I addressed all issues raised so far. Darin, could you please take another look?

Thanks,

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:5437:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


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

[Yuta Kitamura]

(In reply to comment #17)
> Attachment 52516 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/css/CSSParser.cpp:5437:  A case label should not be indented, but line
> up with its switch statement.  [whitespace/indent] [4]
> Total errors found: 1 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Oops, I've forgotten to run check-webkit-style. My bad.

However, I'm wondering if I need to fix this error, because I've found there are a lot of both usages (indented and not-indented case labels).

[Darin Adler]

Comment on attachment 52516 [details]
Patch v6 (Applying darin's comment).

> +    if (p == end || !(p[0] == '_' || p[0] >= 128 || isASCIIAlpha(p[0])))

In a later patch (I don't suggest delaying this one) we should use !isASCII(p[0]) instead of p[0] >= 128 since it’s clearer and probably more efficient as well.

> +        if (!(p[0] == '_' || p[0] == '-' || p[0] >= 128 || isASCIIAlphanumeric(p[0])))

Same here.

> +                if (c < 128)
> +                    return false;

And isASCII(c) here.

> +            static const char hexDigits[17] = "0123456789abcdef";

I personally prefer capital hex.

> +    String quoteCSSString(const String&);

I think we should make this function be private to the .cpp file instead of exporting it. I can't think of any case where we'd want other files to use it.

r=me without any changes -- you could make one or more of the changes above in a follow-up patch

[Yuta Kitamura]

(In reply to comment #19)
> r=me without any changes -- you could make one or more of the changes above in
> a follow-up patch

Thanks, I'd be happy to write a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 52516 [details]
Patch v6 (Applying darin's comment).

Clearing flags on attachment: 52516

Committed r57105: <http://trac.webkit.org/changeset/57105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.