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

fix proto header for the case when the header is the last part of the… #294

Closed
wants to merge 4 commits into from
Closed

fix proto header for the case when the header is the last part of the… #294

wants to merge 4 commits into from

Conversation

pingwang
Copy link

@pingwang pingwang commented Jun 7, 2016

fix proto header for the case when the header is the last part of the payload

} else {
headerEnd = valueStart + lineEnd
if payload[headerEnd-1] == '\r' {
headerEnd -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

should replace headerEnd -= 1 with headerEnd--

@buger
Copy link
Owner

buger commented Jun 8, 2016

Thank you! Can you add a test for this case?

@buger
Copy link
Owner

buger commented Jun 8, 2016

HTTP protocol says that requests without body should end with \r\n anyway. If you experience such bug, can you tell the http server / tool generating such HTTP requests?

@pingwang
Copy link
Author

pingwang commented Jun 14, 2016

The issue I hit is when I first use gor to collect traffic into a file, then repay the file toward a box:
The request causing the issue looks like as below:

1 e1efa3e43e16d9c20bda986baef9b40f23c47e7b 1463501171780734926
222497 GET /edw/edw1x1.gif?eventtype=audience_segment&...&ua=Mozilla/5.0%20AppleWebKit/537.36%20(KHTML%2C%20like%20Gecko%3B%20compatible%3B%20Go oglebot/2.1%3B%20+http%3A//www.google.com/bot.html)%20Safari/537.36&edwscrres=1024X1024&ts=1461888000000 HTTP/1.0^M
Accept: /,/^M
Accept-Encoding: identity;q=1.0, *;q=0^M
Accept-Language: en^M
Cookie: EdmundsYear=; __utma=XXX;__utmb=XXX
🐵🙈🙉
1 ee5e42b5a51b67e14b0e71c66bf4b80acc5d1b88 1463501171785941015
...(another request)

But the same request was not like this in another log file. So it looks like the tool got some partial data when collecting...

@pingwang
Copy link
Author

test case added! Sorry for the late reply! I was bound to something else! Will try to be more responsive!

@buger
Copy link
Owner

buger commented Jun 14, 2016

I have the feeling that you are trying to fix the wrong issue. Instead, it's better to understand why it happens. Is there any chance you can give me file recorded by Gor? Does it happens for all your requests, or it is the rare issue? Is there any correlation?

For example, if we find out that for some requests Gor captures only partial request, it's better to not replay it at all, and add some checks that ensure request integrity.

Thanks!

@pingwang
Copy link
Author

Yeah, I agree that the right fix would be check the integrity of the request while replying.
The case is rare. It happens only once after I collect 1 hour traffic daily for about 2 months.
Sorry that I can't share the whole log file with you, but I sent the unchanged requests with partial data to your gmail.
Do you have time to add the integrity check? I could help out if you give me some suggestion about where to add the check.
Thank you!
Ping.

@pingwang
Copy link
Author

HI Buger, I updated the code and test case. It will skip the invalid head. It's similar as the header is not found. Could you please approve the pull request or tell me what to improve? The tool panics more often as we move to replay human traffic. The first day we replay human traffic it panicked due to this issue..

@buger
Copy link
Owner

buger commented Jun 27, 2016

Sorry for the long delay! I implemented proper request validation #317

Here are binaries:
gor_http_validation_mac.tar.gz
gor_http_validation_x64.tar.gz

@buger buger closed this Jun 27, 2016
@pingwang pingwang deleted the proto-header branch July 7, 2016 18:52
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.

3 participants