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

multissl: curl_global_cleanup doesn't restore Curl_ssl as its default (Curl_ssl_multi) #5255

Closed
davidedec opened this issue Apr 17, 2020 · 10 comments
Labels

Comments

@davidedec
Copy link

@davidedec davidedec commented Apr 17, 2020

I noticed that after a curl_global_cleanup it's not possible to curl_global_sslset and curl_global_init again in order to change the ssl backend.

This can be achieved in vtls.c by setting back the Curl_ssl = &Curl_ssl_multi after the Curl_ssl_cleanup

The idea is to be able to change the backend in this sequence:

curl_global_sslset(CURLSSLBACKEND_SCHANNEL, NULL, NULL);
curl_global_init(CURL_GLOBAL_DEFAULT);
//....
curl_global_cleanup();

curl_global_sslset(CURLSSLBACKEND_OPENSSL, NULL, NULL);
curl_global_init(CURL_GLOBAL_DEFAULT);
//....
curl_global_cleanup();

Thanks

@jay jay added the libcurl API label Apr 17, 2020
@jay
Copy link
Member

@jay jay commented Apr 17, 2020

The documentation says it can only be set once, but I would think it could be set after cleanup, in other words once each time it's uninitialized. @dscho

@dscho
Copy link
Contributor

@dscho dscho commented Apr 17, 2020

Makes sense.

I think it should be as easy as adding the lines

#ifdef CURL_SSL_MULTI
    Curl_ssl = &Curl_ssl_multi;
#endif

to

curl/lib/vtls/vtls.c

Lines 179 to 186 in 9c703ea

void Curl_ssl_cleanup(void)
{
if(init_ssl) {
/* only cleanup if we did a previous init */
Curl_ssl->cleanup();
init_ssl = FALSE;
}
}

@davidedec could you test that?

@davidedec
Copy link
Author

@davidedec davidedec commented Apr 17, 2020

@dscho thanks for your prompt reply.
I'll test that tomorrow and I'll come back to you.

Thanks again

@dscho
Copy link
Contributor

@dscho dscho commented Apr 17, 2020

Actually, I think those three lines should go into Curl_none_cleanup(). Consequently, that function should be renamed to Curl_multissl_cleanup(), and it should be moved directly after Curl_multissl_init().

@dscho
Copy link
Contributor

@dscho dscho commented Apr 17, 2020

Ah! And of course it should call Curl_ssl->cleanup(), unless Curl_ssl still points to Curl_ssl_multi.

@davidedec
Copy link
Author

@davidedec davidedec commented Apr 17, 2020

Right. Tomorrow I will tell you if it works and pass tests

@jay
Copy link
Member

@jay jay commented Apr 17, 2020

If it works can one of you turn it into a PR for review please

@dscho
Copy link
Contributor

@dscho dscho commented Apr 17, 2020

I actually pulled out the computer and am doing precisely that.

dscho added a commit to dscho/curl that referenced this issue Apr 17, 2020
When cURL is compiled with support for multiple SSL backends, it is
possible to configure an SSL backend via `curl_global_sslset()`, but
only *before* `curl_global_init()` was called.

If another SSL backend should be used after that, a user might be
tempted to call `curl_global_cleanup()` to start over. However, we did
not foresee that use case and forgot to reset the SSL backend in that
cleanup.

Let's allow that use case.

This addresses curl#5255

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Contributor

@dscho dscho commented Apr 17, 2020

Phew, that took longer than I hoped for. I'm glad I tried to verify that my patch works, as my initial version (the one modifying Curl_none_cleanup()) totally did not work, like, at all.

@davidedec would you mind testing #5257?

@davidedec
Copy link
Author

@davidedec davidedec commented Apr 18, 2020

Just tested #5257 and worked as expected for me

@bagder bagder closed this in ff7a310 Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.