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

Jump to bottom

HTTP2 sends spurious PRIORITY packet on duplicated handles #4303

Closed
kinnison opened this issue Sep 7, 2019 · 4 comments
Closed

HTTP2 sends spurious PRIORITY packet on duplicated handles #4303

kinnison opened this issue Sep 7, 2019 · 4 comments
Assignees
@bagder
Labels
HTTP/2

Comments

@kinnison
Copy link

kinnison commented Sep 7, 2019

I did this

  1. I prepared a curl_easy_init()ed handle and stashed it away.
  2. I duplicated that handle with curl_easy_duphandle()
  3. I made an HTTP2 request to https://twitter.com/ using that duplicated handle

I expected the following

  1. An SSL connection to establish
  2. An HTTP2 session over that connection
  3. Successful twittering

What happened instead

  1. An SSL connection was established
  2. An HTTP2 session began over that connection
  3. An unexpected PRIORITY message was sent for stream -1
  4. Twitter shut the connection down on an RST_STREAM for stream -1

curl/libcurl version

curl 7.64.0 (x86_64-pc-linux-gnu) libcurl/7.64.0 OpenSSL/1.1.1c zlib/1.2.11 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) libssh2/1.8.0 nghttp2/1.36.0 librtmp/2.3

operating system

Debian Buster (amd64)

Diagnosis

I spent a few hours working through this before discovering the unexpected PRIORITY message. Once I had tracked this down, I pared back why it was occurring and discovered that in http2.c inside h2_session_send() where the new weight and/or dependency detection logic exists, the value of data->state.stream_weight was zero whereas the value of data->set.stream_weight was 16 (which I determined was the default weight from nghttp2).

Tracking through the places where state.stream_weight is set, only Curl_http2_init_state() and the duphandle() called by push_promise() look useful. (Both in http2.c)

Possible ameliorations

One approach is simply in h2_session_send() after the call to h2_pri_spec() to only queue the PRIORITY packet if stream_id is not -1. Using a patch to this effect, I was able to fetch twitter.com usefully in my program.

Another approach, and perhaps the preferable one, would be to find the right place for Curl_http2_init_state() to be called during the point at which the HTTP connection is upgraded to HTTP2. Perhaps during Curl_http2_setup_conn() ?

I am not well-enough versed in the core of the library to make the right call here, but certainly sending a PRIORITY message to stream -1 seems very broken to me, and also to twitter.com's web server 😁
@bagder bagder added the HTTP/2 label Sep 9, 2019
@kinnison
Copy link
Author

It looks like twitter.com is no longer failing when this spurious message is sent, but there's still an argument that it shouldn't be sent.

@bagder bagder self-assigned this Sep 27, 2019
bagder added a commit that referenced this issue Sep 30, 2019
@bagder
http2: move state-init from creation to pre-transfer
9c51fec 
To make sure that the HTTP/2 state is initialized correctly for
duplicated handles. It would otherwise easily generate "spurious"
PRIORITY frames to get sent over HTTP/2 connections when duplicated easy
handles were used.

Reported-by: Daniel Silverstone
Fixes #4303
@bagder bagder mentioned this issue Sep 30, 2019
Closed
@bagder
Copy link
Member

bagder commented Sep 30, 2019

Thanks for this report and sorry for being slow on the ball. Can you please try out the patch in #4442 and see if that fixes the problem for you?

@kinnison
Copy link
Author

kinnison commented Oct 2, 2019

Irritatingly Twitter have made themselves less succeptible to this, so I can't directly test the fix in #4442 -- however if cURL has assert() type stuff in it, then you could simply ensure you never send PRIORITY messages to channel -1. By inspection, I think the change you made should be good enough though.

@bagder
Copy link
Member

bagder commented Oct 3, 2019

Cool, thanks. I'll add an assert to the fix as you suggest and merge.

bagder added a commit that referenced this issue Oct 3, 2019
@bagder
http2: move state-init from creation to pre-transfer
7bc3905 
To make sure that the HTTP/2 state is initialized correctly for
duplicated handles. It would otherwise easily generate "spurious"
PRIORITY frames to get sent over HTTP/2 connections when duplicated easy
handles were used.

Reported-by: Daniel Silverstone
Fixes #4303
Closes #4442
@bagder bagder closed this as completed in 8a00560 Oct 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bagder bagder

Labels
HTTP/2
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http2: move state-init from creation to pre-transfer curl/curl
2 participants
@bagder @kinnison