Fix for CSS identifier related assert

Fix for CSS escaped identifier assert

Escaped identifiers are stored as 16-bit string so using the 8-bit
string storage for them causes unexpected errors in tokenToLowerCase(). 
In debug there was an ASSERT that crashed the browser. In release
any non-ascii chars in the string would have been partly garbled.

This also removes the "const" from an argument because the function
does mutate the argument so calling it const was not very honest.

BUG=358750
R=eseidel

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170841

[Daniel Bratell, 2014-04-02 10:17:12]

Please take a look at this. I think this is a crash that can easily be triggered
by malicious pages but also by well intended pages using non-ascii class names.

[rune, 2014-04-02 11:11:09]

Tip: use --no-find-copies with git cl upload if you haven't moved any files (to
avoid those unrelated A+).

In description:

"are stores as 16-bit string" -> "are stored as 16-bit strings, "

https://codereview.chromium.org/213743018/diff/1/LayoutTests/fast/css/css-esc...
File LayoutTests/fast/css/css-escaped-identifier.html (right):

https://codereview.chromium.org/213743018/diff/1/LayoutTests/fast/css/css-esc...
LayoutTests/fast/css/css-escaped-identifier.html:12: <script>

Indentation looks funny. Use 4 space indentation consistently.

https://codereview.chromium.org/213743018/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/213743018/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser-in.cpp:1792:
makeLower(token.characters16(), const_cast<UChar*>(token.characters16()),
length);

Excuse my unicode ignorance, but are you guaranteed that upper/lower case
variants use the same number of code units?

[Daniel Bratell, 2014-04-02 11:26:52]

On 2014/04/02 11:11:09, rune - CET wrote:

> Excuse my unicode ignorance, but are you guaranteed that upper/lower case
> variants use the same number of code units?

See the comment in the code above this code. I wondered the same but it's
apologizing for being too simple.

[rune, 2014-04-02 12:14:02]

On 2014/04/02 11:26:52, Daniel Bratell wrote:
> On 2014/04/02 11:11:09, rune - CET wrote:
> 
> > Excuse my unicode ignorance, but are you guaranteed that upper/lower case
> > variants use the same number of code units?
> 
> See the comment in the code above this code. I wondered the same but it's
> apologizing for being too simple.

I see. The Unicode::toLower uses a 1-1 UTF-32 mapping, it seems. At least, there
should be no buffer overrun issue there.

[eseidel, 2014-04-02 18:35:28]

This no longer really has anything to do with the tokenizer, so can be moved
onto CSSParserString, or?

https://codereview.chromium.org/213743018/diff/20001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/213743018/diff/20001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser.h:221: void
tokenToLowerCase(CSSParserString& token);
This seems like it belongs on CSSParserString, no?

[eseidel, 2014-04-02 18:37:04]

I'm not sure what buffer these point into.  CSSParserString does not own its
data.  I don't think this method is safe.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Daniel Bratell, 2014-04-02 20:04:51]

On 2014/04/02 18:37:04, eseidel wrote:
> I'm not sure what buffer these point into.  CSSParserString does not own its
> data.  I don't think this method is safe.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

As far as I know they will point to somewhere in
CSSTokenizer::m_currentCharacter8, CSSTokenizer::m_currentCharacter16 or (since
the recent memory usage patch) to a buffer in the m_cssStrings16 vector. All of
those are writable work buffers (though it's good if the tokenizer doesn't touch
memory that hasn't yet been parsed).

I did consider moving the code to CSSParserString, but just because
CSSParserString doesn't own the memory it doesn't feel right. The current code
doesn't feel right either though so I can do that move. Tomorrow. Late now.

[eseidel, 2014-04-02 20:06:03]

Just more evidence that we need to re-think this subsystem. :)

[Daniel Bratell, 2014-04-03 08:36:28]

On 2014/04/02 20:06:03, eseidel wrote:
> Just more evidence that we need to re-think this subsystem. :)

eseidel, I looked at moving the function to CSSParserString but that became a
worse ugliness since it would either have to touch memory it knew nothing about
or the whole system would have to be changed to a non-const storage. Changing to
non-const storage meant a lot of other changes that would uglify code. I think
that comes more under "re-think this subsystem" than anything else.

Btw, as for being a Bison parser this doesn't look too bad. I think it's more
"Bison meets Modern C++" than anything inherently broken. Now I'm not a big fan
of Bison parsers but I'm not sure it's worth the time to rewrite it if the only
crime is that it's ugly.

But that was a side-track. I've added an explanation in the code to why it's
safe to do what it does. And it doesn't actually do anything ugly after this
patch that it didn't do before this patch. Except that instead of a const_cast
it used a loophole in C++ to convert a const pointer to a non-const pointer.

char* a = NULL;
const char* b = "Protected";
a = a + (a - b); // Now a points to the write protected memory despite being
non-const.

[Daniel Bratell, 2014-04-03 08:36:29]

On 2014/04/02 20:06:03, eseidel wrote:
> Just more evidence that we need to re-think this subsystem. :)

eseidel, I looked at moving the function to CSSParserString but that became a
worse ugliness since it would either have to touch memory it knew nothing about
or the whole system would have to be changed to a non-const storage. Changing to
non-const storage meant a lot of other changes that would uglify code. I think
that comes more under "re-think this subsystem" than anything else.

Btw, as for being a Bison parser this doesn't look too bad. I think it's more
"Bison meets Modern C++" than anything inherently broken. Now I'm not a big fan
of Bison parsers but I'm not sure it's worth the time to rewrite it if the only
crime is that it's ugly.

But that was a side-track. I've added an explanation in the code to why it's
safe to do what it does. And it doesn't actually do anything ugly after this
patch that it didn't do before this patch. Except that instead of a const_cast
it used a loophole in C++ to convert a const pointer to a non-const pointer.

char* a = NULL;
const char* b = "Protected";
a = a + (a - b); // Now a points to the write protected memory despite being
non-const.

[Daniel Bratell, 2014-04-03 08:41:46]

On 2014/04/03 08:36:29, Daniel Bratell wrote:
> 
> char* a = NULL;
> const char* b = "Protected";
> a = a + (a - b); // Now a points to the write protected memory despite being
> non-const.

Make that:

char* a = NULL;
const char* b = "Protected";
a = a + (b - a); // Now non-const |a| points to the write protected memory.

[eseidel, 2014-04-04 06:46:56]

yeah, i'm in no rush to replace this parser, but i expect we eventually will. 
Right now cssyyparse is just a black box we can't optimize.

crbug.com/330389

[eseidel, 2014-04-04 06:47:37]

lgtm

bleh.  thanks for fixing.  how long have we had this crash?  do we need to
backport this fix?

[eseidel, 2014-04-04 06:54:30]

https://codereview.chromium.org/213743018/diff/40001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/213743018/diff/40001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1792: if (token.is8Bit()) {
when is the token bits different from the source?

[eseidel, 2014-04-04 06:55:20]

https://codereview.chromium.org/213743018/diff/40001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/213743018/diff/40001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1792: if (token.is8Bit()) {
On 2014/04/04 06:54:30, eseidel wrote:
> when is the token bits different from the source?

i see, only for escape chars.

[eseidel, 2014-04-04 07:05:11]

lgtm

the parser allocates buffers for these strings.  the only bug here is
correctness, no crash or security.

[commit-bot: I haz the power, 2014-04-04 07:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/213743018/40001

[commit-bot: I haz the power, 2014-04-04 07:27:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 07:27:02]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[Daniel Bratell, 2014-04-04 07:42:00]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-04-04 07:42:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/213743018/40001

[commit-bot: I haz the power, 2014-04-04 08:10:25]

Change committed as 170841