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

HTTP/2 connection reuse is broken in 7.49.1 #855

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

Comments

Projects
None yet
5 participants
@gevaerts
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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 2, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@gevaerts

gevaerts Jun 2, 2016

Contributor

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

Contributor

gevaerts commented Jun 2, 2016

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

@andydude

This comment has been minimized.

Show comment
Hide comment
@andydude

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

@bagder bagder changed the title from 8243a958 breaks connection reuse (and probably more) with http2 to HTTP/2 connection reuse is broken in 7.49.1 Jun 3, 2016

jay added a commit that referenced this issue Jun 5, 2016

http: Fix HTTP/2 connection reuse
- 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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 5, 2016

Member

We fixed this in 1aa899f, thanks.

Member

jay commented Jun 5, 2016

We fixed this in 1aa899f, thanks.

@jay jay closed this Jun 5, 2016

@Jesin

This comment has been minimized.

Show comment
Hide comment
@Jesin

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

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 10, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jul 11, 2016

Member

@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).

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.