Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: fail if connection terminated without END_STREAM #6736

Closed
wants to merge 1 commit into from

Conversation

@oxalica
Copy link
Contributor

@oxalica oxalica commented Mar 12, 2021

Emit an error if HTTP/2 stream is not correctly closed when underlying TCP/SSL connection is terminated.
In rfc7540, section 8.1. HTTP Request/Response Exchange,

An HTTP response is complete after the server sends -- or the client
receives -- a frame with the END_STREAM flag set (including any
CONTINUATION frames needed to complete a header block).

In case of the server or proxy server is SIGKILLed during data transfer, Linux kernel will terminated the TCP connection by FIN+ACK. And if the server doesn't provide Content-Length (eg. GitHub tarball archive URLs), our current behavior is to exit without error, leaving incomplete data.
The misbehavior makes downstream programs unexpectedly cache incomplete files (NixOS/nix#4533).

Note:

  1. HTTP/1.1 CAN correctly handle the case above (emit an error) by chunked transfer due to missing of final zero-size chunk.
  2. With HTTP/2 + TLS, TLS DO detect the unexpected termination of TCP connection. But we unfortunately choose to explicitly ignore this error in #4624. So at least we should still check in HTTP/2 layer for the data completeness.

Reproduce misbehavior: (reproduced in master 7b2f067)

  1. Setup any proxy server with protocol socks5 or http, and set environment variable https_proxy.
  2. Run curl -LO https://codeload.github.com/NixOS/nixpkgs/legacy.tar.gz/772406c2a4e22a85620854056a4cd02856fa10f0, or any other URL without Content-Length. (For GitHub, it may be cached if an archive is recently downloaded. You can try different commits, or wait for minutes between two fetch, until curl doesn't show total bytes, which indicates missing Content-Length)
  3. kill -9 the proxy server.
  4. The curl instance exited with 0 without any error, leaving the incomplete file.
@bagder bagder added the HTTP/2 label Mar 12, 2021
@bagder
bagder approved these changes Mar 12, 2021
Copy link
Member

@bagder bagder left a comment

Nice catch!

@bagder bagder closed this in d1f4007 Mar 12, 2021
@bagder
Copy link
Member

@bagder bagder commented Mar 12, 2021

Thanks!

@oxalica oxalica deleted the oxalica:fix/h2-partial-trans branch Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants