Bug 213631 – Convert ContentSecurityPolicy related parsers over to using StringParsingBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Convert ContentSecurityPolicy related parsers over to using StringParsingBuffer

[Sam Weinig]

Created attachment 402836 [details]
Patch

[Sam Weinig]

Created attachment 402839 [details]
Patch

[Sam Weinig]

Created attachment 402874 [details]
Patch

[Darin Adler]

Comment on attachment 402874 [details]
Patch

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

> Source/WebCore/html/parser/ParsingUtilities.h:38
> +template<typename CharType>
> +inline bool isNotASCIISpace(CharType c)

CharacterType, single line, constexpr?

> Source/WebCore/html/parser/ParsingUtilities.h:44
> +template<typename CharType, typename DelimiterType>
> +bool skipExactly(const CharType*& position, const CharType* end, DelimiterType delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:54
> +template<typename CharType, typename DelimiterType>
> +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType delimiter)

CharacterType, single line?

I suggest naming the local variable just "buffer". Single words seem to work well in short functions.

> Source/WebCore/html/parser/ParsingUtilities.h:74
> +template<typename CharType, bool characterPredicate(CharType)>
> +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line?

I suggest naming the local variable just "buffer". Single words seem to work well in short functions.

> Source/WebCore/html/parser/ParsingUtilities.h:84
> +template<typename CharType, typename DelimiterType>
> +void skipUntil(const CharType*& position, const CharType* end, DelimiterType delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:91
> +template<typename CharType, typename DelimiterType>
> +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:105
>  template<typename CharType, bool characterPredicate(CharType)>
> +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line, "buffer"?

> Source/WebCore/html/parser/ParsingUtilities.h:119
>  template<typename CharType, bool characterPredicate(CharType)>
> +void skipWhile(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line, "buffer"?

> Source/WebCore/html/parser/ParsingUtilities.h:143
> +template<typename CharacterType, unsigned lowercaseLettersLength>

I think that "length" is not a great name for the size of an array that contains a null-terminated string. We use it elsewhere, but it seems like a mistake.

> Source/WebCore/html/parser/ParsingUtilities.h:146
> +    if (parsingBuffer.lengthRemaining() < lowercaseLettersLength - 1)

Seems like the whole function would be better if it said:

    constexpr auto lowercaseLettersLength = lowercaseLettersArraySize - 1;
    ASSERT(!lowercaseLetters[lowercaseLettersLength]);

Maybe with some kind of comment, and might need to be the right kind of assert, maybe not ASSERT.

> Source/WebCore/html/parser/ParsingUtilities.h:150
> +    parsingBuffer += (lowercaseLettersLength - 1);

Not sure the parentheses make this clearer.

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:37
> +template<typename CharacterType> static Optional<ResourceCryptographicDigest::Algorithm> parseHashAlgorithmAdvancingPosition(StringParsingBuffer<CharacterType>& parsingBuffer)

"buffer"

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:52
> +template<typename CharacterType> static Optional<ResourceCryptographicDigest> parseCryptographicDigestImpl(StringParsingBuffer<CharacterType>& parsingBuffer)

"buffer"

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:67
> +    skipExactly<CharacterType>(parsingBuffer, '=');
> +    skipExactly<CharacterType>(parsingBuffer, '=');

Compiler can’t deduce CharacterType?

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:82
> +Optional<ResourceCryptographicDigest> parseCryptographicDigest(StringParsingBuffer<UChar>& parsingBuffer)

"buffer" (I’ll stop commenting this each time)

> Source/WebCore/page/csp/ContentSecurityPolicyMediaListDirective.cpp:39
> +template<typename CharacterType>
> +static bool isMediaTypeCharacter(CharacterType c)

Single-line?

[Sam Weinig]

Created attachment 402884 [details]
Patch

[Darin Adler]

Comment on attachment 402884 [details]
Patch

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

> Source/WebCore/html/parser/ParsingUtilities.h:37
> +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c)

Just curious, why not constexpr? Also, really no need to write inline here — can’t imagine it has any effect.

> Source/WebCore/html/parser/ParsingUtilities.h:127
> +    if (!WTF::equalLettersIgnoringASCIICase(position, lowercaseLettersLength - 1, lowercaseLetters))

I accidentally tricked you into breaking the code! The "-1" here is now wrong.

> Source/WebCore/html/parser/ParsingUtilities.h:129
> +    position += (lowercaseLettersLength - 1);

Ditto.

> Source/WebCore/html/parser/ParsingUtilities.h:141
> +    buffer += (lowercaseLettersLength);

Parentheses here are peculiar.

[Darin Adler]

Comment on attachment 402884 [details]
Patch

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

> Source/WebCore/html/parser/ParsingUtilities.h:37
> +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c)

Just curious, why not constexpr? Also, really no need to write inline here — can’t imagine it has any effect.

> Source/WebCore/html/parser/ParsingUtilities.h:127
> +    if (!WTF::equalLettersIgnoringASCIICase(position, lowercaseLettersLength - 1, lowercaseLetters))

I accidentally tricked you into breaking the code! The "-1" here is now wrong.

> Source/WebCore/html/parser/ParsingUtilities.h:129
> +    position += (lowercaseLettersLength - 1);

Ditto.

> Source/WebCore/html/parser/ParsingUtilities.h:141
> +    buffer += (lowercaseLettersLength);

Parentheses here are peculiar.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 402884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402884&action=review
> 
> > Source/WebCore/html/parser/ParsingUtilities.h:37
> > +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c)
> 
> Just curious, why not constexpr? Also, really no need to write inline here —
> can’t imagine it has any effect.

No good reason, just didn't think of it, will fix.

> 
> > Source/WebCore/html/parser/ParsingUtilities.h:127
> > +    if (!WTF::equalLettersIgnoringASCIICase(position, lowercaseLettersLength - 1, lowercaseLetters))
> 
> I accidentally tricked you into breaking the code! The "-1" here is now
> wrong.
> 
> > Source/WebCore/html/parser/ParsingUtilities.h:129
> > +    position += (lowercaseLettersLength - 1);
> 
> Ditto.

Yikes.  

> 
> > Source/WebCore/html/parser/ParsingUtilities.h:141
> > +    buffer += (lowercaseLettersLength);
> 
> Parentheses here are peculiar.

Yeah, those are dumb. Will remove.

[Sam Weinig]

(In reply to Sam Weinig from comment #8)
> (In reply to Darin Adler from comment #7)
> > Comment on attachment 402884 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=402884&action=review
> > 
> > > Source/WebCore/html/parser/ParsingUtilities.h:37
> > > +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c)
> > 
> > Just curious, why not constexpr? Also, really no need to write inline here —
> > can’t imagine it has any effect.
> 
> No good reason, just didn't think of it, will fix.

Oh, actually, now I remember why. isASCIISpace is not constexpr yet. Was going to address that in a follow up.

[Sam Weinig]

Created attachment 402889 [details]
Patch

[Darin Adler]

Occurs to me that having this tightly bound pair of pointer/end can be used not just for refactoring, but for additional hardening. If some of those assertions are changed to RELEASE_ASSERT.

Of course, then it makes it a consequential decision if you get a pointer from the buffer and use it, since that gets rid of the hardening.

Seems like this is not our current goal, but an interesting future possibility.

[EWS]

Committed r263581: <https://trac.webkit.org/changeset/263581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402889 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64820967>

[Radar WebKit Bug Importer]

<rdar://problem/64820968>

[Sam Weinig]

(In reply to Darin Adler from comment #11)
> Occurs to me that having this tightly bound pair of pointer/end can be used
> not just for refactoring, but for additional hardening. If some of those
> assertions are changed to RELEASE_ASSERT.
> 
> Of course, then it makes it a consequential decision if you get a pointer
> from the buffer and use it, since that gets rid of the hardening.
> 
> Seems like this is not our current goal, but an interesting future
> possibility.

I think there are additional assertions (not necessarily release ones) beyond what is there so far such as adding the UnderlyingString type checks that StringView has to StringParsingBuffer.