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

ngtcp2 and altsvc fixes #4875

Closed
wants to merge 2 commits into from
Closed

ngtcp2 and altsvc fixes #4875

wants to merge 2 commits into from

Conversation

piru
Copy link

@piru piru commented Feb 3, 2020

This PR fixes the build for non-IPv6 targets.

In addition Alt-Svc header parsing was pretty much broken:

  • before the flags parsing assumed there are single flags section that applied to all alternative protocols, resulting in the code parsing the flags from wrong location on multi-entry header.
  • the code would get stuck in a busyloop if protocol wasn't followed by an equal sign.
  • the parser would terminate on unknown protocols rather than ignoring them.

@bagder bagder added build HTTP/3 h3 or quic related labels Feb 3, 2020
@@ -23,6 +23,7 @@
#include "curl_setup.h"

#ifdef USE_NGTCP2
#include <assert.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ngtcp2.c includes curl_setup.h and there on line 337:

#include <assert.h>

Can you figure out why that isn't enough?

(Also, we should probably convert the assert() calls to DEBUGASSERT() to unify with other code.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. that header looks like a mistake indeed.

@piru
Copy link
Author

piru commented Feb 3, 2020

I git incompetence aside, now it should just do the one thing that is really needed, add the #ifdef ENABLE_IPV6 that is.

@piru piru changed the title ngtcp2: include assert.h and #ifdef ENABLE_IPV6 ngtcp2: use #ifdef ENABLE_IPV6 to guard AF_INET6 usage Feb 3, 2020
@piru piru changed the title ngtcp2: use #ifdef ENABLE_IPV6 to guard AF_INET6 usage ngtcp2 and altsvc fixes Feb 4, 2020
@piru
Copy link
Author

piru commented Feb 4, 2020

Got a bit carried away with the alt-svc stuff I spotted. Comments welcome.

… only, as per RFC. The earlier code would also get totally confused by multiprotocol header, parsing flags from the wrong part of the header. Fixed the parser terminating on unknown protocols, instead of skipping them. Fixed a busyloop when protocol-id was present without an equal sign.
@piru
Copy link
Author

piru commented Feb 4, 2020

fixed regression on unit test 1654

@bagder
Copy link
Member

bagder commented Feb 4, 2020

Did you detect those problems with the existing test case(s) or should we extend them to make sure?

@piru
Copy link
Author

piru commented Feb 4, 2020

Did you detect those problems with the existing test case(s) or should we extend them to make sure?

Existing test cases didn't show these issues. This is a real world header response I got from LiteSpeed server that choked the parser:

quic=":443"; ma=2592000; v="39,43,46", h3-Q039=":443"; ma=2592000, h3-Q043=":443"; ma=2592000, h3-Q046=":443"; ma=2592000, h3-23=":443"; ma=2592000, h3-24=":443"; ma=2592000

The old parser completely gives up on this one at the "quic" entry. It could be extended to include h2 entry at the end:

quic=":443"; ma=2592000; v="39,43,46", h3-Q039=":443"; ma=2592000, h3-Q043=":443"; ma=2592000, h3-Q046=":443"; ma=2592000, h3-23=":443"; ma=2592000, h3-24=":443"; ma=2592000, h2="foo.example.com:443"

If unit test 1654 was adjusted to check for success of parsing this header it'd detect at least this case. To add check for failing to parse flags correctly would require checking the actual parsed items. For example:
h2="foo.example.com:443"; persist=1, h2="bar.example.com:443; persist=0

With old parser both entries will have persist of 1, whereas correct result in that first to have 1, 2nd one have 0. Similar check could be done for ma, but checking for correct value is time dependant.

@bagder bagder closed this in 9e1f720 Feb 4, 2020
@bagder
Copy link
Member

bagder commented Feb 4, 2020

Thanks, landing this and then we can work on polishing this further.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build HTTP/3 h3 or quic related
Development

Successfully merging this pull request may close these issues.

2 participants