Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710053002:
HTTP/2 Check header names in HeaderCoalescer (Closed)
DescriptionHTTP/2 Check header names in HeaderCoalescer
According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII
characters. SpdyStream::SaveResponseHeaders() checks whether header
names contain uppercase ASCII characters, but not whether header names
are valid tokens.
This CL makes HeaderCoalescer to enforce an header name validity check
by HttpUtil::IsValidHeaderName().
BUG=691243
Review-Url: https://codereview.chromium.org/2710053002
Cr-Commit-Position: refs/heads/master@{#453749}
Committed: https://chromium.googlesource.com/chromium/src/+/21b64f4da725d81384c3558f96d9d8b045c99633
Patch Set 1 #
Total comments: 2
Patch Set 2 : use asanka's suggestion #
Total comments: 4
Patch Set 3 : fix pseudo and add test #
Total comments: 6
Patch Set 4 : add tests #
Total comments: 17
Patch Set 5 : fix tests and address comments #Patch Set 6 : minor optimization #Messages
Total messages: 51 (27 generated)
xunjieli@chromium.org changed reviewers: + birenroy@chromium.org, bnc@chromium.org
Bence and Biren: Is this the right way to solve the issue (crbug.com/691243)? I can't find anywhere in the codebase that enforces an ASCII check on the header names (SpdyStream::SaveResponseHeaders only checks uppercase ASCII). The net_spdy_session_fuzzer fed non-UTF8 characters into a mock socket and Chrome crashes upon converting the non-UTF8 header name to a base::Value. PTAL.
The CQ bit was checked by xunjieli@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...
On 2017/02/22 19:03:59, xunjieli wrote: > Bence and Biren: Is this the right way to solve the issue (crbug.com/691243)? > I can't find anywhere in the codebase that enforces an ASCII check on the header > names (SpdyStream::SaveResponseHeaders only checks uppercase ASCII). The > net_spdy_session_fuzzer fed non-UTF8 characters into a mock socket and Chrome > crashes upon converting the non-UTF8 header name to a base::Value. > > PTAL. I think it is possible to be more specific. Header field names must be tokens, which are defined here: http://httpwg.org/specs/rfc7230.html#rule.token.separators (Header field reference: http://httpwg.org/specs/rfc7230.html#header.fields)
https://codereview.chromium.org/2710053002/diff/1/net/spdy/header_coalescer.cc File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/1/net/spdy/header_coalescer.c... net/spdy/header_coalescer.cc:28: if (!base::IsStringASCII(key)) { Consider using something like HttpUtil::IsValidHeaderName(). RFC 7540 prescribes an additional restriction where the header name should be lowercase.
The CQ bit was unchecked by xunjieli@chromium.org
Description was changed from ========== HTTP/2 Restrict header names to ASCII characters. According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names contain non-ASCII characters. This CL makes HeaderCoalescer to enforce an ASCII check. BUG=691243 ========== to ========== HTTP/2 Check header names in HeaderCoalescer According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names are valid tokens. This CL makes HeaderCoalescer to enforce an header name validity check by HttpUtil::IsValidHeaderName(). BUG=691243 ==========
Description was changed from ========== HTTP/2 Check header names in HeaderCoalescer According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names are valid tokens. This CL makes HeaderCoalescer to enforce an header name validity check by HttpUtil::IsValidHeaderName(). BUG=691243 ========== to ========== HTTP/2 Check header names in HeaderCoalescer According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names are valid tokens. This CL makes HeaderCoalescer to enforce an header name validity check by HttpUtil::IsValidHeaderName(). BUG=691243 ==========
Done. PTAL. https://codereview.chromium.org/2710053002/diff/1/net/spdy/header_coalescer.cc File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/1/net/spdy/header_coalescer.c... net/spdy/header_coalescer.cc:28: if (!base::IsStringASCII(key)) { On 2017/02/22 19:44:13, asanka wrote: > Consider using something like HttpUtil::IsValidHeaderName(). RFC 7540 prescribes > an additional restriction where the header name should be lowercase. Done. Thanks for the suggestion!
lgtm https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:40: if (key[0] == ':') { This code block appears intended to handle pseudo-header fields. If pseudo-header fields are consumed by another layer, it may be possible to remove this code block and any associated code paths. (Could be done as a future clean-up change.)
asanka@chromium.org changed reviewers: + asanka@chromium.org
Let's add a couple of tests for this. Also why are we deferring the lowercase test until SpdyStream::OnHeadersReceived ?
https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:40: if (key[0] == ':') { On 2017/02/22 20:26:23, Biren Roy wrote: > This code block appears intended to handle pseudo-header fields. If > pseudo-header fields are consumed by another layer, it may be possible to remove > this code block and any associated code paths. > > (Could be done as a future clean-up change.) I did a brief scan of the spdy code, but I didn't any pseudo-header handling like this anywhere else. I might not be looking at the right places. Bence, do you know?
https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:40: if (key[0] == ':') { On 2017/02/22 22:19:58, xunjieli wrote: > On 2017/02/22 20:26:23, Biren Roy wrote: > > This code block appears intended to handle pseudo-header fields. If > > pseudo-header fields are consumed by another layer, it may be possible to > remove > > this code block and any associated code paths. > > > > (Could be done as a future clean-up change.) > > I did a brief scan of the spdy code, but I didn't any pseudo-header handling > like this anywhere else. I might not be looking at the right places. > Bence, do you know? Yeah, I think you're right. Nothing before this layer will consume pseudo-headers. It looks like you will have to explicitly allow a leading ':' in the conditional above. One easy way to do that might be to copy |key| and remove a leading ':', if present, before passing the StringPiece to HttpUtils::IsValidHeaderName().
xunjieli, thank you for fixing this. birenroy and asanka, thank you for the reviews. +1 to using HttpUtil::IsValidHeaderName(). +1 to adding tests. +1 to moving lowercase enforcement from SpdyStream::SaveResponseHeaders() to HeaderCoalescer, either in this CL or in a separate one. I think HeaderCoalescer is the first (only?) place where pseudo header order is checked, so that check should probably stay. Note that HttpUtil::IsValidHeaderName() is okay with colons, so no need to handle pseudo headers separately.
> +1 to adding tests. Done. > +1 to moving lowercase enforcement from SpdyStream::SaveResponseHeaders() to HeaderCoalescer, either in this CL or in a separate one. I will file a crbug in a moment. I think we also have lowercase logic in QUIC which probably could be standardized as well. > Note that HttpUtil::IsValidHeaderName() is okay with colons, so no need to handle pseudo headers separately. No, unfortunately it's not. colon is not a valid token. PTAL. https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/20001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:40: if (key[0] == ':') { On 2017/02/22 22:34:20, Biren Roy wrote: > On 2017/02/22 22:19:58, xunjieli wrote: > > On 2017/02/22 20:26:23, Biren Roy wrote: > > > This code block appears intended to handle pseudo-header fields. If > > > pseudo-header fields are consumed by another layer, it may be possible to > > remove > > > this code block and any associated code paths. > > > > > > (Could be done as a future clean-up change.) > > > > I did a brief scan of the spdy code, but I didn't any pseudo-header handling > > like this anywhere else. I might not be looking at the right places. > > Bence, do you know? > > Yeah, I think you're right. Nothing before this layer will consume > pseudo-headers. > > It looks like you will have to explicitly allow a leading ':' in the conditional > above. One easy way to do that might be to copy |key| and remove a leading ':', > if present, before passing the StringPiece to HttpUtils::IsValidHeaderName(). Done. Good catch!
The CQ bit was checked by xunjieli@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:53: if (value.find("\r\n") != base::StringPiece::npos) { HttpUtil::IsValidHeaderValue(value) According to RFC 7230 § 3.2 only allows VCHAR / obs-text / SP, and HTAB in the field value. IsValidHeaderValue() just checks that the value doesn't contain LF, CR, or NUL. https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:79: TEST_F(HeaderCoalescerTest, HeaderNameNotValid) { There should be a test that stakes out RFC 7230's definition of header-name here. I.e. have a test that asserts that a header name containing all possible tchars is considered valid. We want the tests to make sure we won't reject a valid header name. https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:83: } Similar to the above test requirement. Add one for a header value made up of all valid header value chars. That header should be considered valid by HeaderCoalescer. The goal is to ensure that the doesn't block a valid header.
On 2017/02/23 16:10:42, xunjieli wrote: > > > Note that HttpUtil::IsValidHeaderName() is okay with colons, so no need to > handle pseudo headers separately. > > No, unfortunately it's not. colon is not a valid token. > Oops, you are right. Sorry.
Thanks Asanka for walking me through the spec. Asanka + Bence : PTAL. https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:53: if (value.find("\r\n") != base::StringPiece::npos) { On 2017/02/23 17:45:13, asanka wrote: > HttpUtil::IsValidHeaderValue(value) > > According to RFC 7230 § 3.2 only allows VCHAR / obs-text / SP, and HTAB in the > field value. IsValidHeaderValue() just checks that the value doesn't contain LF, > CR, or NUL. Done. https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:79: TEST_F(HeaderCoalescerTest, HeaderNameNotValid) { On 2017/02/23 17:45:13, asanka wrote: > There should be a test that stakes out RFC 7230's definition of header-name > here. I.e. have a test that asserts that a header name containing all possible > tchars is considered valid. We want the tests to make sure we won't reject a > valid header name. Done. https://codereview.chromium.org/2710053002/diff/40001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:83: } On 2017/02/23 17:45:13, asanka wrote: > Similar to the above test requirement. Add one for a header value made up of all > valid header value chars. That header should be considered valid by > HeaderCoalescer. The goal is to ensure that the doesn't block a valid header. Done.
The CQ bit was checked by xunjieli@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/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:52: // Line folding, RFC 7230 Section 3.2.4., is a special case of this. Comment no longer necessary. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:88: // where token is any visible US ASCII characters. |token| is defined in RFC 7230 Appendix B as: tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA token = 1*tchar Which is what the string below is based on. Visible US ASCII characters (VCHARS) also include delimiters like " ( ) : etc.. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:121: "-.^_`|~"); This should be VCHAR, which is defined in RFC 5234: VCHAR = %x21-7E ; visible (printing) characters The string you used is for tchars :-).
LGTM modulo asanka's and my nits. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:9: #include "base/strings/string_piece.h" In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it takes base::StringPiece arguments by value. Please move this include there. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:88: // where token is any visible US ASCII characters. On 2017/02/23 23:18:54, asanka wrote: > |token| is defined in RFC 7230 Appendix B as: > > tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / > "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA > token = 1*tchar > > Which is what the string below is based on. Visible US ASCII characters (VCHARS) > also include delimiters like " ( ) : etc.. > Section 3.2.6 might be a more specific reference for the same definition (they are repeated in Appendix B for convenience). There is no need to repeat the definition in the comment, |header_name| lists all allowed characters anyway. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:108: // Add two header, one with an HTAB and one with a SP. s/header/headers/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xunjieli@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...
I filed a follow-up bug on moving ascii check from SpdyStream to HeaderCoalescer at crbug.com/695495 PTAL. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:52: // Line folding, RFC 7230 Section 3.2.4., is a special case of this. On 2017/02/23 23:18:54, asanka wrote: > Comment no longer necessary. Asanka, I reverted this. HttpUtil::IsValidHeaderValue() doesn't allow '\0', which Spdy uses to separate multi-value headers. SpdyNetworkTransactionTest.ResponseHeaders is an example. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:9: #include "base/strings/string_piece.h" On 2017/02/23 23:33:04, Bence wrote: > In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it takes > base::StringPiece arguments by value. Please move this include there. base::StringPiece is used on line 70. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:88: // where token is any visible US ASCII characters. On 2017/02/23 23:33:04, Bence wrote: > On 2017/02/23 23:18:54, asanka wrote: > > |token| is defined in RFC 7230 Appendix B as: > > > > tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / > > "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA > > token = 1*tchar > > > > Which is what the string below is based on. Visible US ASCII characters > (VCHARS) > > also include delimiters like " ( ) : etc.. > > > > Section 3.2.6 might be a more specific reference for the same definition (they > are repeated in Appendix B for convenience). There is no need to repeat the > definition in the comment, |header_name| lists all allowed characters anyway. Done. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:88: // where token is any visible US ASCII characters. On 2017/02/23 23:33:04, Bence wrote: > On 2017/02/23 23:18:54, asanka wrote: > > |token| is defined in RFC 7230 Appendix B as: > > > > tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / > > "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA > > token = 1*tchar > > > > Which is what the string below is based on. Visible US ASCII characters > (VCHARS) > > also include delimiters like " ( ) : etc.. > > > > Section 3.2.6 might be a more specific reference for the same definition (they > are repeated in Appendix B for convenience). There is no need to repeat the > definition in the comment, |header_name| lists all allowed characters anyway. Acknowledged. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:108: // Add two header, one with an HTAB and one with a SP. On 2017/02/23 23:33:04, Bence wrote: > s/header/headers/ Done. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:121: "-.^_`|~"); On 2017/02/23 23:18:54, asanka wrote: > This should be VCHAR, which is defined in RFC 5234: > > VCHAR = %x21-7E > ; visible (printing) characters > > The string you used is for tchars :-). Done.
https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:52: // Line folding, RFC 7230 Section 3.2.4., is a special case of this. On 2017/02/24 16:33:32, xunjieli wrote: > On 2017/02/23 23:18:54, asanka wrote: > > Comment no longer necessary. > > Asanka, I reverted this. HttpUtil::IsValidHeaderValue() doesn't allow '\0', > which Spdy uses to separate multi-value headers. > > SpdyNetworkTransactionTest.ResponseHeaders is an example. This might be specific to test code. I can file a bug to investigate if you'd like me to.
https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer.cc:52: // Line folding, RFC 7230 Section 3.2.4., is a special case of this. On 2017/02/24 16:35:33, xunjieli wrote: > On 2017/02/24 16:33:32, xunjieli wrote: > > On 2017/02/23 23:18:54, asanka wrote: > > > Comment no longer necessary. > > > > Asanka, I reverted this. HttpUtil::IsValidHeaderValue() doesn't allow '\0', > > which Spdy uses to separate multi-value headers. > > > > SpdyNetworkTransactionTest.ResponseHeaders is an example. > > This might be specific to test code. I can file a bug to investigate if you'd > like me to. I propose not to introduce rejecting \0 in this CL, because (1) it would not match the CL description, and (2) if that change breaks things, then the entire CL would need to be reverted. It could be done in a follow-up CL, possible after next branch point to be on the safe side. BTW RFC 7540 Section 10.3 says valid characters for header value are "field-content" from RFC 7230 Section 3.2, and my impression is that \0 is not included in that. Cookie crumbling in HTTP/2 is described in RFC 7540 Section 8.1.2.5, so possibly SpdyNetworkTransactionTest.ResponseHeaders is incorrect. But I would need more time to verify into this. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:9: #include "base/strings/string_piece.h" On 2017/02/24 16:33:32, xunjieli wrote: > On 2017/02/23 23:33:04, Bence wrote: > > In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it takes > > base::StringPiece arguments by value. Please move this include there. > > base::StringPiece is used on line 70. header_coalescer.h also uses base::StringPiece, and thus should include string_piece.h. It does not, this is a pre-existing bug. While you are working with these files, you might want to move this include from header_coalescer_test.cc to header_coalescer.h where it belongs. Or if you'd prefer, I can do it in a separate two-line CL.
https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:9: #include "base/strings/string_piece.h" On 2017/02/24 16:51:30, Bence wrote: > On 2017/02/24 16:33:32, xunjieli wrote: > > On 2017/02/23 23:33:04, Bence wrote: > > > In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it takes > > > base::StringPiece arguments by value. Please move this include there. > > > > base::StringPiece is used on line 70. > > header_coalescer.h also uses base::StringPiece, and thus should include > string_piece.h. It does not, this is a pre-existing bug. While you are working > with these files, you might want to move this include from > header_coalescer_test.cc to header_coalescer.h where it belongs. Or if you'd > prefer, I can do it in a separate two-line CL. Bence, I think this test file should include string_piece.h even if header_coalescer.h includes that file, because this test file used StringPiece. I am not able to find this in the "include what you use" rule, but I believe we should include it. Am I missing something?
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 xunjieli@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...
LGTM. https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... File net/spdy/header_coalescer_test.cc (right): https://codereview.chromium.org/2710053002/diff/60001/net/spdy/header_coalesc... net/spdy/header_coalescer_test.cc:9: #include "base/strings/string_piece.h" On 2017/02/24 17:36:55, xunjieli wrote: > On 2017/02/24 16:51:30, Bence wrote: > > On 2017/02/24 16:33:32, xunjieli wrote: > > > On 2017/02/23 23:33:04, Bence wrote: > > > > In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it > takes > > > > base::StringPiece arguments by value. Please move this include there. > > > > > > base::StringPiece is used on line 70. > > > > header_coalescer.h also uses base::StringPiece, and thus should include > > string_piece.h. It does not, this is a pre-existing bug. While you are > working > > with these files, you might want to move this include from > > header_coalescer_test.cc to header_coalescer.h where it belongs. Or if you'd > > prefer, I can do it in a separate two-line CL. > > Bence, I think this test file should include string_piece.h even if > header_coalescer.h includes that file, because this test file used StringPiece. > > I am not able to find this in the "include what you use" rule, but I believe we > should include it. Am I missing something? Good question. The style guide (https://google.github.io/styleguide/cppguide.html) says: "...dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h..." "...any includes present in the related header do not need to be included again in the related cc (i.e., foo.cc can rely on foo.h's includes)." Based on this, I understand header_coalescer.h to be the related header to header_coalescer_test.cc, and I do not include files in foo_test.cc that were included in foo.h. But I do not feel strongly about this, so it is okay with me if you do not change the string_piece include here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM thanks!
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from birenroy@chromium.org Link to the patchset: https://codereview.chromium.org/2710053002/#ps100001 (title: "minor optimization")
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": 100001, "attempt_start_ts": 1488314979782530,
"parent_rev": "4c6a28dc38c7cb48f9f0a29a2e832fe8acdeaf3f", "commit_rev":
"21b64f4da725d81384c3558f96d9d8b045c99633"}
Message was sent while issue was closed.
Description was changed from ========== HTTP/2 Check header names in HeaderCoalescer According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names are valid tokens. This CL makes HeaderCoalescer to enforce an header name validity check by HttpUtil::IsValidHeaderName(). BUG=691243 ========== to ========== HTTP/2 Check header names in HeaderCoalescer According to RFC 7540 Section 8.1.2, HTTP/2 header names are ASCII characters. SpdyStream::SaveResponseHeaders() checks whether header names contain uppercase ASCII characters, but not whether header names are valid tokens. This CL makes HeaderCoalescer to enforce an header name validity check by HttpUtil::IsValidHeaderName(). BUG=691243 Review-Url: https://codereview.chromium.org/2710053002 Cr-Commit-Position: refs/heads/master@{#453749} Committed: https://chromium.googlesource.com/chromium/src/+/21b64f4da725d81384c3558f96d9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/21b64f4da725d81384c3558f96d9... |
