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 crash on remote connection when GIT_PROXY_AUTO is set but no proxy is detected #4934

Merged
merged 1 commit into from
Jan 14, 2019

Conversation

hackhaslam
Copy link
Contributor

This broke recently. If no proxy is detected, the URL remains unset but it still gets into gitno_connection_data_from_url which has assert(url).

@ethomson
Copy link
Member

If there's no proxy detected, we should be returning GIT_ENOTFOUND... 🤔

@hackhaslam
Copy link
Contributor Author

Then http_connect would have to be taught to ignore GIT_ENOTFOUND and continue connecting anyway. It's not obvious to me that GIT_PROXY_AUTO not detecting a proxy should result in GIT_ENOTFOUND, but I can change it along those lines if you think it makes more sense.

@ethomson
Copy link
Member

Right - I was reading git_remote__get_http_proxy too quickly and missed the part where we clear the error when the proxy environment variable is not found.

Sorry about that, indeed I agree with your reasoning here.

I worry that the test is a bit hidden though and I might miss this when I revisit this code to add support for proxies over SSL. What about breaking it into its own test:

		if ((error = git_remote__get_http_proxy(t->owner->owner,
			    !!t->server.url.use_ssl, &t->proxy_url)) < 0)
				return error;

		if (!t->proxy_url)
				return 0;

@hackhaslam
Copy link
Contributor Author

Okay, I made that change. I also added the failing test case.

@ethomson
Copy link
Member

I also added the failing test case.

💥 Thanks for doing this.

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