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

Close the http2 connection when no more requests may be sent. #5643

Closed
wants to merge 2 commits into from

Conversation

@laramiel
Copy link
Contributor

@laramiel laramiel commented Jul 3, 2020

Well-behaving HTTP2 servers send two GOAWAY messages. The first
message is a warning that indicates that the server is going to
stop accepting streams. The second one actually closes the stream.

nghttp2 reports this state (and the other state of no more stream
identifiers) via the call nghttp2_session_check_request_allowed().
When in this state the client should not create more streams on the
session (tcp connection), and in curl this means that the server
has requested that the connection is closed.

It would be also be possible to put the connclose() call into the
on_http2_frame_recv() function that triggers on the GOAWAY message.

This fixes a bug seen when the client sees the following sequence of
frames:

// advisory GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = -1]
... some additional frames

// final GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = N ]

Before this change, curl will attempt to reuse the connection even
after the last stream, will encounter this error:

* Found bundle for host localhost: 0x5595f0a694e0 [can multiplex]
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 10443 (#0)
* Using Stream ID: 9 (easy handle 0x5595f0a72e30)
> GET /index.html?5 HTTP/2
> Host: localhost:10443
> user-agent: curl/7.68.0
> accept: */*
>
* stopped the pause stream!
* Connection #0 to host localhost left intact
curl: (16) Error in the HTTP2 framing layer

This error may posion the connection cache, causing future requests
which resolve to the same curl connection to go through the same error
path.

Well-behaving HTTP2 servers send two GOAWAY messages. The first
message is a warning that indicates that the server is going to
stop accepting streams. The second one actually closes the stream.

nghttp2 reports this state (and the other state of no more stream
identifiers) via the call nghttp2_session_check_request_allowed().
In this state the client should not create more streams on the
session (tcp connection), and in curl this means that the server
has requested that the connection is closed.

It would be also be possible to put the connclose() call into the
on_http2_frame_recv() function that triggers on the GOAWAY message.

This fixes a bug seen when the client sees the following sequence of
frames:

// advisory GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = -1]
... some additional frames

// final GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = N ]

Before this change, curl will attempt to reuse the connection even
after the last stream, will encounter this error:

* Found bundle for host localhost: 0x5595f0a694e0 [can multiplex]
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 10443 (#0)
* Using Stream ID: 9 (easy handle 0x5595f0a72e30)
> GET /index.html?5 HTTP/2
> Host: localhost:10443
> user-agent: curl/7.68.0
> accept: */*
>
* stopped the pause stream!
* Connection #0 to host localhost left intact
curl: (16) Error in the HTTP2 framing layer

This error may posion the connection cache, causing future requests
which resolve to the same curl connection to go through the same error
path.
@laramiel
Copy link
Contributor Author

@laramiel laramiel commented Jul 3, 2020

I believe this fixes the error mentioned here:

#5389 (comment)

We have manually worked around this error in tensorstore:

https://github.com/google/tensorstore/blob/master/tensorstore/internal/http/curl_transport.cc#L274

I have reproduced the error with a GFE and verified that the fix no longer triggers the error when communicating with the same GFE.

@bagder bagder added the HTTP/2 label Jul 3, 2020
lib/http2.c Outdated Show resolved Hide resolved
@bagder bagder closed this in ef86daf Jul 3, 2020
@bagder
Copy link
Member

@bagder bagder commented Jul 3, 2020

Thanks!

@bagder
Copy link
Member

@bagder bagder commented Jul 3, 2020

ping @laramiel. This causes a fuzzer crash. I wonder if this isn't an nghttp2 bug...

==16==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000378da9 at pc 0x00000068d2e7 bp 0x7ffdfc1b6940 sp 0x7ffdfc1b6938
READ of size 1 at 0x603000378da9 thread T0
SCARINESS: 22 (1-byte-read-heap-buffer-overflow-far-from-bounds)
    #0 0x68d2e6 in nghttp2_session_check_request_allowed /src/nghttp2/lib/nghttp2_session.c:1471:20
    #1 0x5887e5 in Curl_http2_done /src/curl/lib/http2.c:1207:11
    #2 0x57bb4a in Curl_http_done /src/curl/lib/http.c:1537:3
    #3 0x5d3679 in rtsp_done /src/curl/lib/rtsp.c:209:16
    #4 0x5b16b2 in multi_done /src/curl/lib/multi.c:581:14
    #5 0x5b933a in multi_runsingle /src/curl/lib/multi.c:2192:9
    #6 0x5b5b5a in curl_multi_perform /src/curl/lib/multi.c:2406:14
    #7 0x5516a3 in fuzz_handle_transfer(fuzz_data*) /src/curl_fuzzer/curl_fuzzer.cc:383:5
    #8 0x5506ee in LLVMFuzzerTestOneInput /src/curl_fuzzer/curl_fuzzer.cc:93:3
    #9 0x459041 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:558:15
    #10 0x458785 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:470:3
    #11 0x45a727 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:770:7
    #12 0x45a929 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:799:3
    #13 0x44a655 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:846:6
    #14 0x472212 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
@bagder
Copy link
Member

@bagder bagder commented Jul 3, 2020

The fuzzer currently runs with nghttp2 1.33.0. @tatsuhiro-t, do you happen to know if this is a known older bug in nghttp2 we've run into?

@bagder
Copy link
Member

@bagder bagder commented Jul 3, 2020

We get the same crash even with nghttp2 1.41.0 (I updated the fuzzer):

==16==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000341e92 at pc 0x00000068d457 bp 0x7fff967bf520 sp 0x7fff967bf518
READ of size 1 at 0x603000341e92 thread T0
SCARINESS: 22 (1-byte-read-heap-buffer-overflow-far-from-bounds)
    #0 0x68d456 in nghttp2_session_check_request_allowed /src/nghttp2/lib/nghttp2_session.c:1482:20
    #1 0x5887e5 in Curl_http2_done /src/curl/lib/http2.c:1207:11
    #2 0x57bb4a in Curl_http_done /src/curl/lib/http.c:1537:3
    #3 0x5d3679 in rtsp_done /src/curl/lib/rtsp.c:209:16
    #4 0x5b16b2 in multi_done /src/curl/lib/multi.c:581:14
    #5 0x5b7114 in multi_runsingle /src/curl/lib/multi.c:1630:15
    #6 0x5b5b5a in curl_multi_perform /src/curl/lib/multi.c:2406:14
    #7 0x5516a3 in fuzz_handle_transfer(fuzz_data*) /src/curl_fuzzer/curl_fuzzer.cc:383:5
@bagder
Copy link
Member

@bagder bagder commented Jul 3, 2020

Created a new separate issue for this in #5646 to make it more visible.

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
You can’t perform that action at this time.