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 536 #538

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix 536 #538

wants to merge 2 commits into from

Conversation

cirka
Copy link

@cirka cirka commented Oct 2, 2018

Fixes connect tunnel proxy issue where SSL over proxy upgrade was unreliable because garbage was left in socket receive buffer.

@benoitc
Copy link
Owner

benoitc commented Oct 3, 2018

Thanks! This patch doesn't handle empty line which however. The main difference with #537 is using a buffer of 8192 where #537 is using a buffer of 4096, I wonder if it's not the initial issue.

@cirka
Copy link
Author

cirka commented Oct 3, 2018

Well empty line after headers section of response is matched with \r\n\r\n.
In case no headers are added you get response like: "HTTP/1.0 200 Connection Established\r\n\r\n" and with additional headers: "HTTP/1.0 200 Connection Established\r\nX-some-header: some_value\r\n\r\n"
One "\r\n" sequence terminates the line, and other "\r\n" sequence immediately following the first sequence signifies empty line. That is what I am matching.
I am not sure 8K is enough for all use cases, but it look sane limit. Who would want they HTTP response and headers weight more than 8K, right ?

@benoitc
Copy link
Owner

benoitc commented Oct 3, 2018

that's untrue, you can have empty line "\n" sent at the beginning :) Which is what I thought was the issue. Anyway let me show the spec about proxies but so far you patch is OK. I think reusing the hackney parser would also do the trick and may reuse more code but that cna be done in another iteration.

@cirka
Copy link
Author

cirka commented Oct 3, 2018

I am not sure it is allowed to have empty line at the beginning, but we do not need to worry about this in the scope of this ticket/contribution. I am only interested in gathering full response from the proxy before allowing a socket to be reused. Any '\n' at the beginning wont break this matching code. I was thinking about using gen_tcp:unrecv in case we overshoot with gen_tcp:recv but it is undocumented....and i wont happen to me ;-)

@cirka
Copy link
Author

cirka commented Oct 3, 2018

I am sorry I am not familiar with hackney code base to reuse existing response parsing code. I just wanted to communicate the problem and to propose working solution. The exact way of solving the problem is not an issue for me, please, feel free to implement your own. I already had my 'high' from understanding what causes the bug ;-)

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