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

Jump to bottom

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
Closed

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

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

Comments

@davidedec
Copy link

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
Copy link
Member

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 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

@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 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 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

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

@jay
Copy link
Member

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 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
@dscho
multi-ssl: reset the SSL backend on Curl_global_cleanup()
106d355 
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 dscho mentioned this issue Apr 17, 2020
Closed
@dscho
Copy link
Contributor

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

Just tested #5257 and worked as expected for me

@bagder bagder closed this as completed in ff7a310 Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
libcurl API
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dscho @jay @davidedec