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

add extra checking of header buffer, to support multi line header value #688

Merged
merged 1 commit into from
Dec 14, 2019

Conversation

tedli
Copy link
Contributor

@tedli tedli commented Nov 4, 2019

@erikdubbelboer
Copy link
Collaborator

Thank you for the pull request.
I'm afraid there is one issue. For multi line headers the \r\n that splits up the value should be removed.
See: https://play.golang.org/p/Kpmruh6fghm which is your test but then using net/http.
Can you please fix this?

@tedli
Copy link
Contributor Author

tedli commented Nov 7, 2019

@erikdubbelboer
Hi,
I'm glad to work on this. But since we need to modify the original value, it means we have to make a copy of a range of the original buffer, which will introduce some overhead.
If you have some ideas or thought about this, please share with me.

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.

Thanks for the changes. I'm afraid some more changes are needed before I can accept this.

header.go Outdated Show resolved Hide resolved
header.go Show resolved Hide resolved
header_timing_test.go Outdated Show resolved Hide resolved
@tedli
Copy link
Contributor Author

tedli commented Nov 11, 2019

@erikdubbelboer
Hi,
Thanks for reply. I'm working on this, but it's little complex, and will take some time.
Your point is that modify the request or response buffer in place, rather than alloc new space for multi line value. Am I got you wrong?
If so, we have to move the rest headers that haven't been parsed yet, and the body forward. If only one or two multi line value headers I think it's OK, but when several such header exists, we may move data many times.
And if just move content forward, it's not that harmless, but what if we have to move backward. I mean the net/http formatted the header value, like one=1;two=2 will be parsed into one=1; two=2, there is a space inserted, in this circumstances we need move backward, and we need extra space, what the buffer may don't have.

@erikdubbelboer
Copy link
Collaborator

I think if every time you get a newline within a header value you just skip that byte and copy everything after it back by one byte it should work.

@tedli
Copy link
Contributor Author

tedli commented Nov 20, 2019

@erikdubbelboer
Hi,
I've made new changes according to your suggestion.

@erikdubbelboer
Copy link
Collaborator

@tedli sorry for the slow reply. This is a big change with lots of edge cases and I haven't found the time to really review this yet. I'll try to do it this weekend.

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.

Sorry for the delay. Almost there, just some minor changes.

header.go Outdated Show resolved Hide resolved
header.go Show resolved Hide resolved
header_test.go Outdated Show resolved Hide resolved
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Dec 4, 2019

@tedli not sure if you noticed but the Travis CI build failed because your code panics on certain inputs: https://play.golang.org/p/e7m-bwnCMzf

@tedli
Copy link
Contributor Author

tedli commented Dec 4, 2019

@erikdubbelboer
Thanks for notice. I will dig into it.
Only one question: Is this case a valid case, or just need extra checking to make it robust when getting invalid data?

@erikdubbelboer
Copy link
Collaborator

This is just some random input generated by https://fuzzit.dev/ but it still shouldn't cause a panic of course. It needs to be robust against invalid data yes.

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.

Thanks for fixing the panic. Now there is only one thing left to fix with the test.

header_test.go Show resolved Hide resolved
@erikdubbelboer
Copy link
Collaborator

Thanks!

@erikdubbelboer
Copy link
Collaborator

@tedli I have made some more improvements, can you please check them? #708

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

Successfully merging this pull request may close these issues.

2 participants