[http1] fix H/1 response pipelining#13983
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
antoniovicente
left a comment
There was a problem hiding this comment.
Question: I'm not sure why conditioning on active requests to redispatch breaks integration tests, it made sense to me, and would like to understand why not (or maybe it is right, and something else needs to be changed)
Thanks for the fix. redispatch breaking integration tests sounds weird and interesting. Your mention of 204 responses makes me think that the issue may be related to an upstream generating a response body for a response that can't have a body (e.i. 204, no content, or possibly some HEAD requests). I'm curious about which tests fail.
source/common/http/codec_client.cc
Outdated
| // The HTTP/1 codec will pause dispatch after a single response is complete. We redispatch if | ||
| // there is more data. | ||
| // TODO(asraa): Check that there are more active requests? This causes a failure in a response | ||
| // with 204 integration test. |
There was a problem hiding this comment.
Is it possible for data to be non-empty at this point when proxying H2 or H3 requests? I think the answer is no.
There was a problem hiding this comment.
I think the answer is no as well. If all of the data was not consumed it means codec had encountered an error.
There was a problem hiding this comment.
We could add an ASSERT and see if fuzzing turns something up.
There was a problem hiding this comment.
that's correct re H2. even if the connection closes, nghttp2 will still consume any unconsumed data.
about H3, I hope it also does this. It is opaque to me, but I can guess that it does because it currently loops through all available data until there is none. i don't know about connection closing.
There was a problem hiding this comment.
I think there are some conditions about remaining data. A lot of tests fail when I add the ASSERT. Looking into it.
|
/wait |
|
Thanks Antonio for pushing me to dig into why a 204 test ( envoy/test/integration/integration_test.cc Line 1546 in 5a87f1e So, no more re-dispatch. Draining and closing it is. |
Signed-off-by: Asra Ali <asraa@google.com>
antoniovicente
left a comment
There was a problem hiding this comment.
Looks good, just some very minor nits.
Signed-off-by: Asra Ali <asraa@google.com>
* master: (117 commits) vrp: allow supervisord to open its log file (envoyproxy#14066) [http1] fix H/1 response pipelining (envoyproxy#13983) wasm: make dependency clearer (envoyproxy#14062) docs: updating 100-continue docs (envoyproxy#14040) quiche: fix stream trailer decoding issue (envoyproxy#13871) tidy: use last_github_commit script instead of target branch (envoyproxy#14052) stats: use RE2 and a better pattern to accelerate a single stats tag-extraction RE (envoyproxy#8831) wasm: use static registration for runtimes (envoyproxy#14014) grpc-json-transcoder: Add support for configuring unescaping behavior (envoyproxy#14009) ci: fix CodeQL-build by removing deprecated set-env command (envoyproxy#14046) config: fix crash when type URL doesn't match proto. (envoyproxy#14031) Build: Propagate user-supplied tags to external headers library. (envoyproxy#14016) [test host utils] use make_shared to avoid memory leaks (envoyproxy#14042) jwt_authn: update to jwt_verify_lib with 1 minute clock skew (envoyproxy#13872) quiche: update QUICHE tar (envoyproxy#13949) sds: improve watched directory documentation. (envoyproxy#14029) log the internal error message from *SSL when the cert and private key doesn't match (envoyproxy#14023) wasm: fix CPE for Wasmtime. (envoyproxy#14024) docs: Bump sphinxext-rediraffe version (envoyproxy#13996) CDS: remove warming cluster if CDS response desired (envoyproxy#13997) ...
* Fix HTTP/1 response pipelining. Extraneous data after a response complete when the connection is still open should not be processed. Signed-off-by: Asra Ali <asraa@google.com>
* Fix HTTP/1 response pipelining. Extraneous data after a response complete when the connection is still open should not be processed. Signed-off-by: Asra Ali <asraa@google.com> Signed-off-by: Qin Qin <qqin@google.com>
Signed-off-by: Asra Ali asraa@google.com
Commit Message: Fix H/1 response pipelining. Pipelined responses would continue to process through http_parser and cause failures in double header allocations. Instead, pause http_parser after a response is complete, and add support to redispatch in the codec client if there is more data and the connection is still open.
Risk Level: Medium, changes some H/1 flow.
Testing: Added an integration test, regression tests from fuzzer that caught the problem.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=26838