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 curl 7.67.0 w/OpenSSL: Missing TLS shutdown from server may cause error #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
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

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

bagder commented Mar 12, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants