ASSERTION FAILED: name[0] == '@' && length >= 2 in WebCore::CSSTokenizer::detectAtToken

Source/core/css/CSSTokenizer-in.cpp

 /*
  * Copyright (C) 2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Nicholas Shanks <webkit@nickshanks.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
  * Copyright (C) 2012 Intel Corporation. All rights reserved.
  *
@@ skipping 1463 matching lines @@
             currentCharacter<SrcCharacterType>() += 3;
             m_token = SGML_CD;
         }
         break;
 
     case CharacterAt:
         if (isIdentifierStart<SrcCharacterType>()) {
             m_token = ATKEYWORD;
             ++result;
             parseIdentifier(result, resultString, hasEscape);
-            detectAtToken<SrcCharacterType>(result - tokenStart<SrcCharacterType>(), hasEscape);
+            detectAtToken<SrcCharacterType>(resultString.length() + 1, hasEscape);

[Julien - ping for review, 2014/05/08 15:26:29]

This seems like a big footgun that you are neutering in one case, which is good
but IMO falls too short as we will fight this problem again in the future.

Looking at the rest of the code, there are other callers to parseIdentifier (and
other code paths that could follow when tokenizing) that are also doing pointer
arithmetic. Are they not also open to a similar bug? Shouldn't we be consistent
to avoid copying and pasting some bad pattern? Isn't there something we could do
to avoid more of these bugs?

I have little experience with this code so maybe my questions don't make sense,
so please bear with me.

[reni, 2014/05/22 22:04:38]

This is (was) the only place where the pointer arithmetic on result could be
faulty, because the arithmetic can give wrong length only if the identifier
contains a unicode escape. However, everywhere else the pointer arithmetic is
guarded by "if (!hasEscape)" constructs. It is specific to the at-rules that the
standard enables unicode escapes at some places. This is why we don't have such
a guard around the call to detectAtToken() and why we have to replace the
arithmetic.

[Julien - ping for review, 2014/05/23 10:42:09]

OK, it seems worth a comment here so that people are careful.

         }
         break;
 
     case CharacterBackSlash:
         if (isCSSEscape(*currentCharacter<SrcCharacterType>())) {
             --currentCharacter<SrcCharacterType>();
             parseIdentifier(result, yylval->string, hasEscape);
             m_token = IDENT;
         }
         break;
@@ skipping 85 matching lines @@
     m_dataStart16[length - 1] = 0;
 
     m_is8BitSource = false;
     m_currentCharacter8 = 0;
     m_currentCharacter16 = m_dataStart16.get();
     setTokenStart<UChar>(m_currentCharacter16);
     m_lexFunc = &CSSTokenizer::realLex<UChar>;
 }
 
 } // namespace WebCore