Fix HTTPS proxy regressions caused by the MultiSSL patches #1871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ever since 70f1db3 (vtls: encapsulate SSL backend-specific data, 2017-07-28), the code handling HTTPS proxies was broken because the pointer to the SSL backend data was not swapped between conn->ssl[sockindex] and conn->proxy_ssl[sockindex] as intended, but instead set to NULL (causing segmentation faults). [jes: provided the commit message, tested and verified the patch] Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In d65e6cc (vtls: prepare the SSL backends for encapsulated private data, 2017-06-21), this developer prepared for a separation of the private data of the SSL backends from the general connection data. This conversion was partially automated (search-and-replace) and partially manual (e.g. proxy_ssl's backend data). Sadly, there was a crucial error in the manual part, where the wrong handle was used: rather than connecting ssl[sockindex]' BIO to the proxy_ssl[sockindex]', we reconnected proxy_ssl[sockindex]. The reason was an incorrect location to paste "BACKEND->"... d'oh. Reported by Jay Satiro in curl#1855. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Another mistake in my manual fixups of the largely mechanical
search-and-replace ("connssl->" -> "BACKEND->"), just like the previous
commit concerning HTTPS proxies (and hence not caught during my
earlier testing).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
https-proxy-fixes
branch
from
739cb80
to
281b30c
Compare
bagder
approved these changes
Sep 7, 2017
|
Thanks a lot! |
|
Thank you! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As reported in #1855, HTTPS proxy support was broken by my patches in #1601.
@jay kindly investigated, diagnosed correctly an issue in my design where a pointer would be reset incorrectly by a
memset(..., 0, ...)call and provided the diff of the first commit of this Pull Request.It took me a while to figure out how to test cURL with HTTP proxies, as the command line provided in the ticket did not work for me. I had to use this command line instead:
(note the "repeated"
https:, but it seems that this has been somehow fixed inmastersince I started investigating), after I enabledproxy_connectin my local Apache installation, turnedProxyRequests onand allowed access from localhost viaRequire ip 127.0.0.1in a new<Proxy *>section.Turns out that my manual fixups of the largely mechanical conversion from
connssl->toBACKEND->in preparation for encapsulating the SSL backends' private data out ofstruct ssl_connect_datawere not quite right.In particular, these two replacements were incorrect, inserting
BACKEND->into the wrong line.Clearly a late-night snafu, so I decided to revisit the entire commit, turning up another, similar problem in
Curl_ossl_data_pending().I also ran into a problem in that commit's changes to
lib/vtls/polarssl.cwhereconnwas replaced withBACKENDby mistake, but @bagder already fixed that in 5734f73 (along with another bug that I did not catch, where I must have mistypedssl[sockindex]assock[sockindex]while trying to avoid copy/paste).Of course I hope that there are no other bugs lurking in that commit, I really tried my best to spot issues. At least with this PR, three problems get addressed.