New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 634217 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Buried. Ping if important.
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

CSP parser incorrectly uses equalIgnoringCase everywhere

Project Member Reported by esprehn@chromium.org, Aug 4 2016

Issue description

ex. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp/CSPSourceList.cpp?q=CSPSourceList.cpp&sq=package:chromium&l=157

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp?sq=package:chromium&l=816

Both CSPSourceList and CSPDirectiveList are using it to do stuff like:

equalIgnoringCase("allow", begin, position - begin)

but the third argument to equalIgnoringCase is actually the number of characters to compare so in the above statement if:

begin = "al"

then position - begin = 2

so we compare the first two chars of "allow" to "al" and you get true.
 
Owner: mkwst@chromium.org
Status: Assigned (was: Untriaged)
Note this is fixed by my patch here:
https://codereview.chromium.org/2148423003

WebKit also fixed this, but it looks like it may have been by accident:
https://github.com/WebKit/webkit/commit/a7f7319b978c47b2c8579f23ffa5bbdc1e32cd9c

when they switched from equalIgnoringCase(CharType, CharType, length) which compared at most length chars, to equalLettersIgnoringASCIICase(CharType a, aLength, const char (b)[bLength]). which takes both aLength and bLength.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e

commit 9a8e5154f97acb2d9ccdee61e464e0d2b33b100e
Author: esprehn <esprehn@chromium.org>
Date: Fri Aug 05 04:01:10 2016

Use StringView for equalIgnoringCase.

This lets us merge the code paths and simplifies the code a bunch.

In doing this I removed the ASCII fast path that was in the
equalsIgnoringCase(const StringImpl*, const LChar*) function, though
curiously it wasn't in the (const UChar*, const LChar*) one, but that
function is also used a lot less often. Lets try removing this ancient
fast path for now (from 2006 with no comments about why it exists [1])
and instead opt for simpler code.

For comparing LChar strings the new path should be just as fast if not
faster. For comparing LChar to UChar strings we end up doing a foldCase
call which will be slower, but we should see how bad it is first.

Long term almost all callers of equalIgnoringCase need to switch to
equalIgnoringASCIICase anyway since that's what the specs require
(and what Webkit does now), which will put this fast path back in
place. We can also add the fast path back in the (LChar, UChar) path
if needed.

Switching all of the callers over to the new StringView version of
equalIgnoringCase also exposed that the CSP parsing logic was
accidentally using the function incorrectly and doing prefix matches
instead of actually comparing to the expected values as described in
 issue 634217 . This patches fixes that mistake.

[1] https://chromium.googlesource.com/chromium/src/+/6f022a05a21fc5ed091d7bcb6b81104ea2afdf8e

BUG= 615174 ,  634217 

Review-Url: https://codereview.chromium.org/2148423003
Cr-Commit-Position: refs/heads/master@{#409987}

[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/core/frame/csp/CSPSourceList.cpp
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/platform/weborigin/KURL.h
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/AtomicString.h
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/StringHash.h
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/StringImpl.cpp
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/StringImpl.h
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/StringView.cpp
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/StringView.h
[modify] https://crrev.com/9a8e5154f97acb2d9ccdee61e464e0d2b33b100e/third_party/WebKit/Source/wtf/text/WTFString.h

Status: Fixed (was: Assigned)

Sign in to add a comment