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

feat: add function to parse HTTP header parameters #1685

Merged
merged 2 commits into from
Jan 2, 2024

Conversation

nickajacks1
Copy link
Contributor

The implementation is based on RFC-9110 5.6.6.

https://www.rfc-editor.org/rfc/rfc9110#name-parameters

The primary use case for parsing header parameters is for content negotiation, including multipart type boundaries.

The functor passed to VisitHeaderParams has a bool return value to allow the user to stop processing parameters early.
Examples of this being useful are to find a specific parameter, stop parsing after the q value, or to put a hard limit on the number of parameters a client can include.

I had originally written this for a different project using fasthttp, but I figured it was generic enough that it may warrant being merged in for others to use as well. If not, no feelings will be hurt.

The implementation of validHeaderFieldByte is a little silly looking, but it's as fast as I could get it.
The Go standard library has its own implementation here, which is a bit slower: https://github.com/golang/go/blob/master/src/net/textproto/reader.go#L663
If the table solution is too arcane, we can copy the standard library instead.

Side note: VisitHeaderParams could also be used in MultipartFormBoundary to make fewer assumptions about how the client formatted their Content-Type header, but it would slow the function down quite a bit.
On my machine, it's 20 ns/op today vs 40 ns/op using VisitHeaderParams.

The implementation is based on RFC-9110 5.6.6.
Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

It's very hard to see if VisitHeaderParams has any bugs that might cause panics for example. Can you add a FuzzVisitHeaderParams to fuzz it and see if it crashes?

@nickajacks1
Copy link
Contributor Author

I see that existing fuzz tests use fuzzit, but given that the CI only builds on 1.18 and above and that there are no docs mentioning fuzzit, I just created a Fuzz test in header_test.go instead.

Ran a short 14 min fuzz:
fuzz: elapsed: 14m5s, execs: 27008124 (33905/sec), new interesting: 34 (total: 50)

@erikdubbelboer
Copy link
Collaborator

Yeah fuzzit used to fuzz fasthttp for free, I still need to rewrite the fuzz tests to the new Go fuzzers. Thanks for adding the fuzz test. Everything looks good, thanks!

@erikdubbelboer erikdubbelboer merged commit 868ee45 into valyala:master Jan 2, 2024
14 checks passed
Max-Cheng pushed a commit to Max-Cheng/fasthttp that referenced this pull request Feb 11, 2024
* feat: add function to parse HTTP header parameters

The implementation is based on RFC-9110 5.6.6.

* test: add fuzz for VisitHeaderParams
Max-Cheng pushed a commit to Max-Cheng/fasthttp that referenced this pull request Feb 12, 2024
* feat: add function to parse HTTP header parameters

The implementation is based on RFC-9110 5.6.6.

* test: add fuzz for VisitHeaderParams
Max-Cheng pushed a commit to Max-Cheng/fasthttp that referenced this pull request Feb 12, 2024
* feat: add function to parse HTTP header parameters

The implementation is based on RFC-9110 5.6.6.

* test: add fuzz for VisitHeaderParams
Max-Cheng pushed a commit to Max-Cheng/fasthttp that referenced this pull request Feb 12, 2024
* feat: add function to parse HTTP header parameters

The implementation is based on RFC-9110 5.6.6.

* test: add fuzz for VisitHeaderParams
Max-Cheng pushed a commit to Max-Cheng/fasthttp that referenced this pull request Feb 12, 2024
* feat: add function to parse HTTP header parameters

The implementation is based on RFC-9110 5.6.6.

* test: add fuzz for VisitHeaderParams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants