wwbmmm commented on code in PR #3644:
URL: https://github.com/apache/brpc/pull/3644#discussion_r4357549399
##########
src/brpc/details/http_parser.cpp:
##########
@@ -1605,6 +1605,10 @@ size_t http_parser_execute (http_parser *parser,
break;
case h_transfer_encoding:
+ /* Multiple `Transfer-Encoding` headers are one list (RFC 7230
+ * 3.2.2), so `chunked` from an earlier line is no longer final.
+ */
+ parser->flags &= ~F_CHUNKED;
Review Comment:
[replied by brpc-oncall robot] The flag is only cleared when the new
`Transfer-Encoding` line has a non-empty value. For `Transfer-Encoding:
chunked\r\nTransfer-Encoding:\r\n` the parser goes through
`s_header_value_discard_ws` -> `s_header_value_discard_ws_almost_done` ->
`s_header_value_discard_lws` and never enters `s_header_value_start`, so
`F_CHUNKED` stays set and the body is still read as chunked, while the
equivalent single line `Transfer-Encoding: chunked, ` is rejected with
`HPE_INVALID_TRANSFER_ENCODING`. I verified this against the patched parser:
the two-line form parses as one chunked message with `HPE_OK`. A front-end that
merges the lines (or uses only the last value) therefore still sees a different
boundary than bRPC, which is the discrepancy this patch is meant to remove.
Clearing the flag where the field name is fully matched
(`h_matching_transfer_encoding` -> `h_transfer_encoding` in `s_header_field`,
next to `parser->uses_transfer_encoding = 1`) instead of at
value start covers the empty value as well; please also add a test case for
it.
##########
src/brpc/details/http_parser.cpp:
##########
@@ -1605,6 +1605,10 @@ size_t http_parser_execute (http_parser *parser,
break;
case h_transfer_encoding:
+ /* Multiple `Transfer-Encoding` headers are one list (RFC 7230
+ * 3.2.2), so `chunked` from an earlier line is no longer final.
+ */
+ parser->flags &= ~F_CHUNKED;
/* looking for 'Transfer-Encoding: chunked' */
if ('c' == c) {
parser->header_state = h_matching_transfer_encoding_chunked;
Review Comment:
[replied by brpc-oncall robot] This concern does not apply.
`h_transfer_encoding` is only entered from `s_header_value_start`, once per
`Transfer-Encoding` line, and `s_header_value` has `assert(0 && "Shouldn't get
here.")` for that state, so the clear runs at most once per header line (plus
once more for an allowed obs-fold continuation, which is harmless because the
flag is only set again when a value ends in `chunked`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
