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

HTTP/2 connection reuse is broken in 7.49.1 #855

Closed
gevaerts opened this issue Jun 2, 2016 · 7 comments
Closed

HTTP/2 connection reuse is broken in 7.49.1 #855

gevaerts opened this issue Jun 2, 2016 · 7 comments
Labels
connecting & proxies HTTP HTTP/2

Comments

@gevaerts
Copy link
Contributor

gevaerts commented Jun 2, 2016

curl --http1.1 -v https://http2bin.org/get https://http2bin.org/get
curl --http2 -v https://http2bin.org/get https://http2bin.org/get

The problem is that the version detection in lib/http.c around line 3310 assumes HTTP/major.minor, and 8243a95 changes that, so HTTP/2 gets detected as 10, which then makes a lot of code fall back to HTTP/1.0 behaviour such as connection closing.

Note that 8243a95 was committed between 7.49 and 7.49.1, so the current released version (7.49.1) is affected.
@bagder
Copy link
Member

bagder commented Jun 2, 2016

Ouch. And no test caught it... =( I guess it also tells us we should add a test for this together with the fix.

@gevaerts
Copy link
Contributor Author

gevaerts commented Jun 2, 2016

(found by adu on irc, by the way, not by me)

@andydude
Copy link

andydude commented Jun 2, 2016

I (I'm adu on irc) have a fix for this, but I'm still running the tests to see if there are any regressions.

@andydude andydude mentioned this issue Jun 3, 2016
Closed
@bagder bagder changed the title 8243a958 breaks connection reuse (and probably more) with http2 HTTP/2 connection reuse is broken in 7.49.1 Jun 3, 2016
jay added a commit that referenced this issue Jun 5, 2016
@jay
http: Fix HTTP/2 connection reuse
1aa899f 
- Change the parser to not require a minor version for HTTP/2.

HTTP/2 connection reuse broke when we changed from HTTP/2.0 to HTTP/2
in 8243a95 because the parser still expected a minor version.

Bug: #855
Reported-by: Andrew Robbins, Frank Gevaerts
@jay
Copy link
Member

jay commented Jun 5, 2016

We fixed this in 1aa899f, thanks.

@jay jay closed this as completed Jun 5, 2016
@Jesin
Copy link

Jesin commented Jul 9, 2016

This also caused the -L/--location flag to be ignored when curl 7.49.1 uses HTTP/2. This has broken a few scripts. The flag seems to work again as of f9eed59. Would this be worth making another bugfix release, or is 7.50 close to finished?

@bagder
Copy link
Member

bagder commented Jul 10, 2016

I don't see how this should cause the --location to get ignored, that sounds like a separate issue!

7.50.0 is due to be released on July 21 so there will be no 7.49.X patch release before that.

@jay
Copy link
Member

jay commented Jul 11, 2016

@bagder It looks as though that was a symptom of the parsing bug. When http2 status parsing failed the code defaulted to 200 (ouch indeed).

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
connecting & proxies HTTP HTTP/2
Milestone
No milestone
Development

No branches or pull requests
5 participants
@andydude @bagder @gevaerts @Jesin @jay