Re: [PATCH] imap-send: Fix compilation without deprecated OpenSSL APIs

[Junio C Hamano <gitster@pobox.com>]

Rosen Penev <rosenp@gmail.com> writes:

> On Wed, Dec 26, 2018 at 10:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Rosen Penev <rosenp@gmail.com> writes:
>>
>> > Initialization in OpenSSL has been deprecated in version 1.1.
>>
>> https://www.openssl.org/docs/man1.0.2/ssl/SSL_library_init.html says
>>
>>         SSL_library_init() must be called before any other action takes
>>         place.
>>
>> https://www.openssl.org/docs/man1.1.0/ssl/SSL_library_init.html says
>> the same.
> Later on in the document it mentions that it is deprecated.
>>
>> Which makes it necessary for us to defend the following claim
>>
>> > This makes
>> > compilation fail when deprecated APIs for OpenSSL are compile-time
>> > disabled.
>>
>> as a valid problem description more rigorously.  To me, the cursory
>> web-serfing I did above makes me suspect that an OpenSSL
>> implementation with such a compile-time disabling _is_ buggy, as it
>> forbids the API users to call an API function they are told to call
>> before doing anything else.
> I agree the man page is misleading. The changelog for 1.1.0 is very
> clear though:
>
> Added support for auto-initialisation and de-initialisation of the library.
>      OpenSSL no longer requires explicit init or deinit routines to be called,
>      except in certain circumstances. See the OPENSSL_init_crypto() and
>      OPENSSL_init_ssl() man pages for further information.
>      [Matt Caswell]

All it says is explicit calls to SSL_library_init() is now optional.
It does not say it is a compile-time-error worthy offense to make
explicit init/deinit calls.  Which still means that an OpenSSL
implementation with such a compile-time disabling _is_ buggy, as it
forbids the API users to call an API function they are told to call
before doing anything else.

>> > diff --git a/imap-send.c b/imap-send.c
>> > index b4eb886e2..21f741c8c 100644
>> > --- a/imap-send.c
>> > +++ b/imap-send.c
>> > @@ -284,8 +284,10 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
>> >       int ret;
>> >       X509 *cert;
>> >
>> > +#if (OPENSSL_VERSION_NUMBER < 0x10000000L)
>>
>> https://www.openssl.org/docs/man1.1.0/crypto/OPENSSL_VERSION_NUMBER.html
>>
>> says that OPENSSL_VERSION_NUMBER is of form 0xMNNFFPPS where M is
>> major, NN is minor, FF is fix, PP is patch and S is status, and
>> gives an example that 0x00906023 stands for 0.9.6.b beta 3 (M=0,
>> NN=09, FF=06, PP=02 and S=3).  So "< 0x10000000L" means "anything
>> with M smaller than 1".  IOW, we would no longer call _init() for
>> e.g. "version 1.0.0 beta 0".  That contradicts with the first claim
>> of the proposed log message ("deprecated in 1.1" implying that it is
>> not yet deprecated in say 1.0.2).
> This is a mistake. I will send a v2 to fix.
>
> Oh I see what I did wrong. I mistakenly copied the above
> OPENSSL_VERSION_NUMBER check without looking carefully at the number.

Yup, please be careful next time.

>>
>>
>>
>> >       SSL_library_init();
>> >       SSL_load_error_strings();
>> > +#endif
>> >
>> >       meth = SSLv23_method();
>> >       if (!meth) {