[PATCH/RFC] http_init: only initialize SSL for https

[Erik Faye-Lund]

Since ancient times, we have been calling curl_global_init with the
CURL_GLOBAL_ALL-flag, which initializes SSL (and the Win32 socket
stack on Windows).

Initializing SSL takes quite some time on Windows, so let's avoid
doing it when it's not needed.

timing of echo "" | ./git-remote-http.exe origin http://localhost

before

best of 10 runs:
real 0m1.634s
user 0m0.015s
sys 0m0.000s

worst of 10 runs:
real 0m2.701s
user 0m0.000s
sys 0m0.000s

after

best of 10 runs:
real 0m0.018s
user 0m0.000s
sys 0m0.000s

worst of 10 runs:
real 0m0.024s
user 0m0.000s
sys 0m0.015s

Signed-off-by: Erik Faye-Lund <erik.fa...@hue.no>
---
http.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 3b312a8..528a736 100644
--- a/http.c
+++ b/http.c
@@ -343,7 +343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)

git_config(http_options, NULL);

- curl_global_init(CURL_GLOBAL_ALL);
+ curl_global_init(CURL_GLOBAL_WIN32 | (prefixcmp(url, "https:") ? 0 :
+ CURL_GLOBAL_SSL));

http_proactive_auth = proactive_auth;

--
1.8.0.msysgit.0.3.gd0186ec

[Erik Faye-Lund]

Sorry, that sign-off has my wrong e-mail address. Please replace it with this:

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>

[Johannes Schindelin]

Hi kusma,

Good analysis!

> diff --git a/http.c b/http.c
> index 3b312a8..528a736 100644
> --- a/http.c
> +++ b/http.c
> @@ -343,7 +343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>
> git_config(http_options, NULL);
>
> - curl_global_init(CURL_GLOBAL_ALL);
> + curl_global_init(CURL_GLOBAL_WIN32 | (prefixcmp(url, "https:") ? 0 :
> + CURL_GLOBAL_SSL));
>
> http_proactive_auth = proactive_auth;

I wonder whether we want to have something like this instead:

flags = CURL_GLOBAL_ALL;
if (prefixcmp(url, "https:"))
flags &= ^CURL_GLOBAL_SSL;
curl_global_init(flags);

I do see that CURL_GLOBAL_ALL is #define'd as CURL_GLOBAL_WIN32 |
CURL_GLOBAL_SSL in curl.h, but that might change in the future, no?

Ciao,
Dscho

[Erik Faye-Lund]

Good suggestion. But perhaps we'd want to use CURL_GLOBAL_DEFAULT
instead? I'm thinking that this define is probably what they'd include
any essential flags, but not non-essential flags. CURL_GLOBAL_ALL
might be extended to include initialization bits for other transports,
for instance... but this feels a bit hand-wavy. Simply masking out the
CURL_GLOBAL_SSL-flag would probably be the smallest logical change.

I don't have any strong feeling on this, really. I'd like to hear what
other people think, though.

[Junio C Hamano]

Erik Faye-Lund <kusm...@gmail.com> writes:

> diff --git a/http.c b/http.c
> index 3b312a8..528a736 100644
> --- a/http.c
> +++ b/http.c
> @@ -343,7 +343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>
> git_config(http_options, NULL);
>
> - curl_global_init(CURL_GLOBAL_ALL);
> + curl_global_init(CURL_GLOBAL_WIN32 | (prefixcmp(url, "https:") ? 0 :
> + CURL_GLOBAL_SSL));

The first and obvious question is what the symbol with a name
specific to one single platform doing in this generic codepath.
In order to get convinced that the patch does not regress, one
somehow need to know that bits in ALL other than WIN32 and SSL
do not matter (or there is no such bit).

I'd understand if it were "ALL & ~SSL" though.

[Junio C Hamano]

Erik Faye-Lund <kusm...@gmail.com> writes:

>> I wonder whether we want to have something like this instead:
>>
>> flags = CURL_GLOBAL_ALL;
>> if (prefixcmp(url, "https:"))
>> flags &= ^CURL_GLOBAL_SSL;
>> curl_global_init(flags);
>>
>> I do see that CURL_GLOBAL_ALL is #define'd as CURL_GLOBAL_WIN32 |
>> CURL_GLOBAL_SSL in curl.h, but that might change in the future, no?
>
> Good suggestion. But perhaps we'd want to use CURL_GLOBAL_DEFAULT
> instead?

That as a follow-up suggestion may be fine but if you go that route,
you would need to explicitly flip SSL on when you know it is going
to an SSL destination.

The way to determine SSL-ness has to be rock solid and that is much
more important than ALL vs DEFAULT. Is prefixcmp(url, "https://")
the right way to do so? Do we use this codepath only for HTTPS, or
does anybody use other protocol cURL supports over SSL with this,
too?

[Johannes Schindelin]

Hi Junio,

Hence my earlier suggestion (with the obvious tyop '^' instead of '~').
You will also find the information in my mail (unless you plonk my mails)
that CURL_GLOBAL_ALL is defined as CURL_GLOBAL_WIN32 | CURL_GLOBAL_SSL,
and in kusma's response the suggestion to use DEFAULT & ~SSL instead.

Ciao,
Johannes

[Junio C Hamano]

Johannes Schindelin <Johannes....@gmx.de> writes:

> Hence my earlier suggestion (with the obvious tyop '^' instead of '~').
> You will also find the information in my mail (unless you plonk my mails)

> that ...

Our mails simply crossed. Comparing the two messages I think we are
in complete agreement.

[Johannes Schindelin]

Hi Junio,

Apparently, ftps is also handled by cURL and most likely requires SSL.

How about optimizing for the common case and instead of prefixcmp(url,
"https:")) ask for !prefixcmp(url, "http:")?

Ciao,
Dscho

[Junio C Hamano]

Johannes Schindelin <Johannes....@gmx.de> writes:

> Apparently, ftps is also handled by cURL and most likely requires SSL.
>
> How about optimizing for the common case and instead of prefixcmp(url,
> "https:")) ask for !prefixcmp(url, "http:")?

I think that is a very sensible way to go.

As to ALL vs DEFAULT, given that its manual page is riddled with a
scary warning:

This function must be called at least once within a program (a
program is all the code that shares a memory space) before the
program calls any other function in libcurl. The environment it sets
up is constant for the life of the program and is the same for every
program, so multiple calls have the same effect as one call. ... In
normal operation, you must specify CURL_GLOBAL_ALL. Don't use any
other value unless you are familiar with it and mean to control
internal operations of libcurl.

I think we should stick to ALL. So

flags = CURL_GLOBAL_ALL;
if (!prefixcmp(url, "http:"))
flags &= ~CURL_GLOBAL_SSL;

would be the way to go.

But this is assuming that nobody feeds our client a http:// URL to
the server that redirects us to the https:// version (or we do not
follow such a redirect). I offhand do not know if that is a valid
assumption, though.

[Erik Faye-Lund]

Thanks, both. Very sensible points. I'll re-roll a new version
tomorrow, but it could indeed be that the redirect-case can make this
a no-go.

[Daniel Stenberg]

On Thu, 14 Mar 2013, Junio C Hamano wrote:

> As to ALL vs DEFAULT, given that its manual page is riddled with a scary
> warning:
>
> This function must be called at least once within a program (a
> program is all the code that shares a memory space) before the
> program calls any other function in libcurl. The environment it sets
> up is constant for the life of the program and is the same for every
> program, so multiple calls have the same effect as one call. ... In
> normal operation, you must specify CURL_GLOBAL_ALL. Don't use any
> other value unless you are familiar with it and mean to control
> internal operations of libcurl.

(speaking from a libcurl perspective)

The "warning" is just there to scare people into actually consider what they
want and understand that removing bits will change behavior. I would say
that's exactly what you've done and I don't think people here need to be
scared anymore! :-)

As for how ALL vs DEFAULT will act or differ in the future, I suspect that we
will end up having them being the same (even when we add bits) as we've
encouraged "ALL" in the documentation like this for quite some time.

--

/ daniel.haxx.se

[Junio C Hamano]

Daniel Stenberg <dan...@haxx.se> writes:

> (speaking from a libcurl perspective)
>

> As for how ALL vs DEFAULT will act or differ in the future, I suspect
> that we will end up having them being the same (even when we add bits)
> as we've encouraged "ALL" in the documentation like this for quite
> some time.

Thanks, then we should stick to starting from ALL like everybody
else who followed the suggestion in the documentation. Do you have
recommendations on the conditional dropping of SSL?

[Daniel Stenberg]

On Fri, 15 Mar 2013, Junio C Hamano wrote:

>> As for how ALL vs DEFAULT will act or differ in the future, I suspect that
>> we will end up having them being the same (even when we add bits) as we've
>> encouraged "ALL" in the documentation like this for quite some time.
>
> Thanks, then we should stick to starting from ALL like everybody else who
> followed the suggestion in the documentation. Do you have recommendations
> on the conditional dropping of SSL?

Not really, no.

SSL initing is as has been mentioned alredy only relevant with libcurl if an
SSL powered protocol is gonna be used, so if checking the URL for the protocol
is enough to figure this out then sure that should work fine.

--

/ daniel.haxx.se

[Jeff King]

On Fri, Mar 15, 2013 at 05:23:27PM +0100, Daniel Stenberg wrote:

> >Thanks, then we should stick to starting from ALL like everybody
> >else who followed the suggestion in the documentation. Do you have
> >recommendations on the conditional dropping of SSL?
>
> Not really, no.
>
> SSL initing is as has been mentioned alredy only relevant with
> libcurl if an SSL powered protocol is gonna be used, so if checking
> the URL for the protocol is enough to figure this out then sure that
> should work fine.

But are we correct in assuming that curl will barf if it gets a redirect
to an ssl-enabled protocol? My testing seems to say yes:

[in one terminal]
$ nc -lCp 5001 <<\EOF
HTTP/1.1 301
Location: https://github.com/peff/git.git

EOF

[in another, git compiled with Erik's patch]
$ git ls-remote http://localhost:5001
error: SSL: couldn't create a context: error:140A90A1:lib(20):func(169):reason(161) while accessing http://localhost:5001/info/refs?service=git-upload-pack
fatal: HTTP request failed

-Peff

[Daniel Stenberg]

On Sat, 16 Mar 2013, Jeff King wrote:

> But are we correct in assuming that curl will barf if it gets a redirect to
> an ssl-enabled protocol? My testing seems to say yes:

Ah yes. If it switches over to an SSL-based protocol it will pretty much
require that it had been initialized previously.

With redirects taken into account, I can't think of any really good way around
avoiding this init...

--

/ daniel.haxx.se

[Antoine Pelisse]

> With redirects taken into account, I can't think of any really good way
> around avoiding this init...

Is there any way for curl to initialize SSL on-demand ?

[Daniel Stenberg]

Yes, but not without drawbacks.

If you don't call curl_global_init() at all, libcurl will notice that on first
use and then libcurl will call global_init by itself with a default bitmask.

That automatic call of course will prevent the application from being able to
set its own bitmask choice, and also the global_init function is not
(necessarily) thread safe while all other libcurl functions are so the
internal call to global_init from an otherwise thread-safe function is
unfortunate.

--

/ daniel.haxx.se

[Junio C Hamano]

So in short, unless you are writing a custom application to talk to
servers that you know will never redirect you to HTTPS, passing
custom masks such as ALL&~SSL to global-init is not going to be a
valid optimization.

I think that is a reasonable API; your custom application may want
to go around your intranet servers all of which serve their status
over plain HTTP, and it is a valid optimization to initialize the
library with ALL&~SSL. It is just that such an optimization does
not apply to us---we let our users go to random hosts we have no
control over, and they may redirect us in ways we cannot anticipate.

[Erik Faye-Lund]

I wonder. Our libcurl is build with "-winssl" (USE_WINDOWS_SSPI=1), it
seems. Perhaps switching to openssl (which we already have libraries
for) would make the init-time better?

[Erik Faye-Lund]

It does indeed. So this is probably a better solution, and is
something we're considering doing in Git for Windows anyway (for a
different reason). Thanks for all the feed-back!