http2: Don't send header fields prohibited by HTTP/2 spec #1092

Closed
wants to merge 4 commits into
from

Projects

None yet

4 participants

@tatsuhiro-t
Contributor

Previously, we just ignored "Connection" header field. But HTTP/2
specification actually prohibits few more header fields. This commit
ignores all of them so that we don't send these bad header fields.

@tatsuhiro-t tatsuhiro-t http2: Don't send header fields prohibited by HTTP/2 spec
Previously, we just ignored "Connection" header field.  But HTTP/2
specification actually prohibits few more header fields.  This commit
ignores all of them so that we don't send these bad header fields.
16763a9
@mention-bot

@tatsuhiro-t, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Andersbakken, @jay and @bagder to be potential reviewers.

@bagder bagder added the HTTP/2 label Oct 30, 2016
@jay

trailers can be anywhere so I think we will have to account for this, for example
TE: trailers, deflate
TE: deflate, trailers;qabc

@tatsuhiro-t
Contributor

Good point.. Yes, we need to parse it. Does curl have convenient function to parse this comma separated list header fields?

@jay
Member
jay commented Nov 3, 2016

Does curl have convenient function to parse this comma separated list header fields?

hm offhand I can't think of one. There's always strtok_r. Also I don't think we have to check for the semi-colon in trailers if I'm reading this right:

       TE        = "TE" ":" #( t-codings )
       t-codings = "trailers" | ( transfer-extension [ accept-params ] )
       transfer-coding         = "chunked" | transfer-extension
       transfer-extension      = token *( ";" parameter )
tatsuhiro-t added some commits Nov 6, 2016
@tatsuhiro-t tatsuhiro-t Detect "trailers" in te
ab8f3fa
@tatsuhiro-t tatsuhiro-t Merge branch 'master' into http2-ignore-h2-prohibited-headers d6f20b9
@tatsuhiro-t tatsuhiro-t Fix compile error due to strncasecompare
bb5a697
@tatsuhiro-t
Contributor
tatsuhiro-t commented Nov 6, 2016 edited

Thank you. I ended up not to use strtok_r since it requires NULL-terminated string, and it replaces delimiter with NUL byte. Instead, I added hand crafted "trailers" detection code.
Conflict has been resolved as well.

@jay jay added a commit that closed this pull request Nov 7, 2016
@tatsuhiro-t @jay tatsuhiro-t + jay http2: Don't send header fields prohibited by HTTP/2 spec
Previously, we just ignored "Connection" header field.  But HTTP/2
specification actually prohibits few more header fields.  This commit
ignores all of them so that we don't send these bad header fields.

Bug: https://curl.haxx.se/mail/archive-2016-10/0033.html
Reported-by: Ricki Hirner

Closes #1092
0269f64
@jay jay closed this in 0269f64 Nov 7, 2016
@jay
Member
jay commented Nov 7, 2016

I ended up not to use strtok_r since it requires NULL-terminated string, and it replaces delimiter with NUL byte. Instead, I added hand crafted "trailers" detection code.

Oops I forgot about that. Your changes just landed. I made a few corrections:

  • Typo. Searching for tab had used 't' instead of '\t'
  • Unnecessary goto. We discourage goto when not necessary so I redid the logic.
  • enum names too generic. (For example IGNORE is defined to 0 somewhere in my includes so I prefixed them with HEADERINST_)
  • Unreachable code. Some compilers may warn that since the for(;;) is never exited the return after that block will never be hit, so I removed it.
  • Minor style changes

Thanks!

@tatsuhiro-t
Contributor

The commit looks good. Thank you for merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment