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 sends spurious PRIORITY packet on duplicated handles #4303

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

Comments

@kinnison
Copy link

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

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

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
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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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
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 in 8a00560 Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.