Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555813002:
Don't consider '\0' as whitespace when parsing HTTP headers. (Closed)
DescriptionDon't consider '\0' as whitespace when parsing HTTP headers.
BUG=669932
Committed: https://crrev.com/acf834a5675c89f8b4c3b44a4dfeed3fb4be4375
Cr-Commit-Position: refs/heads/master@{#436724}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address Matt's comments #Patch Set 3 : EmbeddedNull --> Blah (and update comment) #
Messages
Total messages: 21 (11 generated)
eroman@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2555813002/diff/1/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util.cc#newco... net/http/http_util.cc:429: base::StringPiece kLws(HTTP_LWS); Needs a const. Also, LWS is one thing, but kLws seems a bit much. kWhiteSpaceCharacters? k<anything with a vowel>? https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:691: "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: 2||" EmbeddedNull? Where's the magic for that. https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1433: EXPECT_TRUE(HttpUtil::IsLWS(' ')); Should we have a whitelist here? i.e. for (int c = 0; c < 0x100; c++) { EXPECT_EQ(c == '\t' || c =='\t', IsLWS(static_cast<char>(c))); } (Or go from -0x80 to < 0x80, if you want to avoid the overflow)
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the speedy review! https://codereview.chromium.org/2555813002/diff/1/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util.cc#newco... net/http/http_util.cc:429: base::StringPiece kLws(HTTP_LWS); On 2016/12/06 19:08:24, mmenke wrote: > Needs a const. Also, LWS is one thing, but kLws seems a bit much. > kWhiteSpaceCharacters? k<anything with a vowel>? Done. https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:691: "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: 2||" On 2016/12/06 19:08:24, mmenke wrote: > EmbeddedNull? Where's the magic for that. There is nothing special about "EmbeddedNull", I just chose that name to draw attention to what is going on in that line. The special part is the vertical pipe '|'. This is used as shorthand in the test for NUL [1] [1] https://cs.chromium.org/chromium/src/net/http/http_util_unittest.cc?q=http_ut... https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1433: EXPECT_TRUE(HttpUtil::IsLWS(' ')); On 2016/12/06 19:08:24, mmenke wrote: > Should we have a whitelist here? i.e. > for (int c = 0; c < 0x100; c++) { > EXPECT_EQ(c == '\t' || c =='\t', IsLWS(static_cast<char>(c))); > } > > (Or go from -0x80 to < 0x80, if you want to avoid the overflow) I think that is a bit overkill, but can switch to explicit enumeration if you would like. My thinking given the implementation (and bug fix) was to spot check a couple character classes
LGTM https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1433: EXPECT_TRUE(HttpUtil::IsLWS(' ')); On 2016/12/06 19:19:34, eroman (slow) wrote: > On 2016/12/06 19:08:24, mmenke wrote: > > Should we have a whitelist here? i.e. > > for (int c = 0; c < 0x100; c++) { > > EXPECT_EQ(c == '\t' || c =='\t', IsLWS(static_cast<char>(c))); > > } > > > > (Or go from -0x80 to < 0x80, if you want to avoid the overflow) > > I think that is a bit overkill, but can switch to explicit enumeration if you > would like. > > My thinking given the implementation (and bug fix) was to spot check a couple > character classes ok
https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:691: "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: 2||" On 2016/12/06 19:19:34, eroman (slow) wrote: > On 2016/12/06 19:08:24, mmenke wrote: > > EmbeddedNull? Where's the magic for that. > > There is nothing special about "EmbeddedNull", I just chose that name to draw > attention to what is going on in that line. > > The special part is the vertical pipe '|'. This is used as shorthand in the test > for NUL [1] > > [1] > https://cs.chromium.org/chromium/src/net/http/http_util_unittest.cc?q=http_ut... Want me to rename this to something more generic like "Blah:" ?
On 2016/12/06 19:25:33, eroman (slow) wrote: > https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc > File net/http/http_util_unittest.cc (right): > > https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... > net/http/http_util_unittest.cc:691: "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: > 2||" > On 2016/12/06 19:19:34, eroman (slow) wrote: > > On 2016/12/06 19:08:24, mmenke wrote: > > > EmbeddedNull? Where's the magic for that. > > > > There is nothing special about "EmbeddedNull", I just chose that name to draw > > attention to what is going on in that line. > > > > The special part is the vertical pipe '|'. This is used as shorthand in the > test > > for NUL [1] > > > > [1] > > > https://cs.chromium.org/chromium/src/net/http/http_util_unittest.cc?q=http_ut... > > Want me to rename this to something more generic like "Blah:" ? SGTM
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/2555813002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:691: "HTTP/1.0 200 OK|Foo: 1|EmbeddedNull: 3|Bar: 2||" On 2016/12/06 19:25:33, eroman (slow) wrote: > On 2016/12/06 19:19:34, eroman (slow) wrote: > > On 2016/12/06 19:08:24, mmenke wrote: > > > EmbeddedNull? Where's the magic for that. > > > > There is nothing special about "EmbeddedNull", I just chose that name to draw > > attention to what is going on in that line. > > > > The special part is the vertical pipe '|'. This is used as shorthand in the > test > > for NUL [1] > > > > [1] > > > https://cs.chromium.org/chromium/src/net/http/http_util_unittest.cc?q=http_ut... > > Want me to rename this to something more generic like "Blah:" ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2555813002/#ps40001 (title: "EmbeddedNull --> Blah (and update comment)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481058248349890,
"parent_rev": "4965dd853b2a90a5d9768855b7126a1602360546", "commit_rev":
"90d42e356165331c4bb1e79cdcd4ea78f0c8faa9"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't consider '\0' as whitespace when parsing HTTP headers. BUG=669932 ========== to ========== Don't consider '\0' as whitespace when parsing HTTP headers. BUG=669932 Committed: https://crrev.com/acf834a5675c89f8b4c3b44a4dfeed3fb4be4375 Cr-Commit-Position: refs/heads/master@{#436724} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/acf834a5675c89f8b4c3b44a4dfeed3fb4be4375 Cr-Commit-Position: refs/heads/master@{#436724} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
