> On 6 Sep 2022, at 07:49, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Mon, Sep 05, 2022 at 10:44:06PM +0400, Sergey Kandaurov wrote:
>
>>> On 1 Sep 2022, at 20:49, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@mdounin.ru>
>>> # Date 1662050858 -10800
>>> # Thu Sep 01 19:47:38 2022 +0300
>>> # Node ID d73286c43b44f3161ca4de1d9d1cbb070c6da4a7
>>> # Parent 63a4b5ffd440c526bc96c6879dc1b6b489975d98
>>> Win32: fixed build on Windows with OpenSSL 3.0.x (ticket #2379).
>>
>> BTW, win32 build on Windows XP with OpenSSL 3.0.x is currently broken
>> for another reason: due to a missing InterlockedOr64 implementation.
>> See the related fix, expected to appear in upcoming OpenSSL 3.0.6:
>> ce3951fc30c7bc7c3dbacba19d87c79d9af9da0d
>
> Well, that's exactly the reason why I've fixed 64-bit build with
> MSVC 2010, the 2nd patch in this series. But see ticket's
> comment:1, the patch committed into OpenSSL is incomplete and only
> fixes MSVC 2008 and older, but won't fix Windows XP builds in newer
> compilers.

So, the checks for _MSC_VER in the committed fix
seem to be not enough if older WIN32_WINNT requested.

>
>> Now I have to configure OpenSSL with "no-threads" to pass to this error.
>
> We actually use "no-threads" in Unix builds, see b329c0ab1a48. On
> win32 we do use threads (for cache loader and cache manager), but
> this shouldn't require threads support in OpenSSL. That is, it
> might be a good idea to use "no-threads" in win32 builds too,
> especially given that this was the default before OpenSSL 1.1.0.
> Patch below.

At least, all tests passed.

>
>>> SSL_sendfile() expects integer file descriptor as an argument, but nginx
>>> uses OS file handles (HANDLE) to work with files on Windows, and passing
>>> HANDLE instead of an integer correctly results in build failure. Since
>>> SSL_sendfile() is not expected to work on Windows anyway, the code is now
>>> disabled on Windows with appropriate compile-time checks.
>>>
>>> diff -r 63a4b5ffd440 -r d73286c43b44 src/event/ngx_event_openssl.c
>>> --- a/src/event/ngx_event_openssl.c Thu Sep 01 19:45:22 2022 +0300
>>> +++ b/src/event/ngx_event_openssl.c Thu Sep 01 19:47:38 2022 +0300
>>> @@ -1770,7 +1770,7 @@ ngx_ssl_handshake(ngx_connection_t *c)
>>> #endif
>>> #endif
>>>
>>> -#ifdef BIO_get_ktls_send
>>> +#if (defined BIO_get_ktls_send && !NGX_WIN32)
>>>
>>> if (BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection)) == 1) {
>>> ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
>>> @@ -1915,7 +1915,7 @@ ngx_ssl_try_early_data(ngx_connection_t
>>> c->read->ready = 1;
>>> c->write->ready = 1;
>>>
>>> -#ifdef BIO_get_ktls_send
>>> +#if (defined BIO_get_ktls_send && !NGX_WIN32)
>>>
>>> if (BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection)) == 1) {
>>> ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
>>> @@ -2944,7 +2944,7 @@ ngx_ssl_write_early(ngx_connection_t *c,
>>> static ssize_t
>>> ngx_ssl_sendfile(ngx_connection_t *c, ngx_buf_t *file, size_t size)
>>> {
>>> -#ifdef BIO_get_ktls_send
>>> +#if (defined BIO_get_ktls_send && !NGX_WIN32)
>>>
>>> int sslerr, flags;
>>> ssize_t n;
>>>
>>
>> This could be simplified if replaced #ifdef with #if.
>> BIO_get_ktls_send is documented to be a macro (and so tested here).
>> When OpenSSL isn't configured with KTLS, the macro is explanded to 0.
>> Replacement allows optimize ngx_ssl_sendfile() at compile time, as well.
>
> While BIO_get_ktls_send is documented to be a macro, it's not
> required to expand to "(0)" if KTLS is not supported.

Well, this is sort of documented in SSL_write.pod:
SSL_sendfile() is available only when
Kernel TLS is enabled, which can be checked by calling BIO_get_ktls_send().

Then in BIO_ctrl.pod:
BIO_get_ktls_send() returns 1 if the BIO is using the Kernel TLS data-path for
sending. Otherwise, it returns zero.

>
> Further, it's not going to actually work for positive tests, since
> non-numeric macro values will evaluate to 0, effectively disabling
> SSL_sendfile() for all builds.

Missed that point, thnx.

>
>> I see that it's convention in nginx to test external macros using #ifdef.
>> In certain cases we use an exception there if it does or even does not
>> make sense, such as when testing SSL_CTRL_SET_ECDH_AUTO (though that's
>> rather a typo there). Using #if BIO_get_ktls_send looks reasonable to me.
>
> Sure, "#if SSL_CTRL_SET_ECDH_AUTO" looks like a typo (patch
> below), but it used to work, since SSL_CTRL_SET_ECDH_AUTO is a
> non-zero numeric constant when defined. This is not the case with
> BIO_get_ktls_send.
>
>> Another way (though, a less obvious for the reader) is to replace
>> #if/ifdef BIO_get_ktls_send with a more convenient #ifndef OPENSSL_NO_KTLS.
>> This macro is set when KTLS isn't supported and not configured for OpenSSL.
>> As per INSTALL.md in the root of OpenSSL distribution, the enable-ktls option
>> "is forced off on systems that do not support the Kernel TLS data-path".
>> This makes no matter how OpenSSL is configured, with or without this option,
>> if it's claimed in OpenSSL to be unsupported by platform.
>> I tested to configure enable-ktls on win32: that's appeared to be true.
>> Unfortunately, OPENSSL_NO_KTLS is used to be documented (even for runtime
>> BIO_get_ktls_send() checks) only in sources, such as in apps/s_server.c.
>
> This is not going to work for older OpenSSL versions without KTLS
> support. At most, you can replace the !NGX_WIN32 part with
> !defined OPENSSL_NO_KTLS, but I don't think it is a good change
> for this win32-specific issue.

Agreed. So, your proposed change is the only way.

>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1662432499 -10800
> # Tue Sep 06 05:48:19 2022 +0300
> # Node ID de9fe82b0c7789474bb6284f13ebd3f903b129b0
> # Parent 9cf231508a8dbc2492e0d1cd06d7b1258eb5f435
> SSL: fixed incorrect usage of #if instead of #ifdef.
>
> In 2014ed60f17f, "#if SSL_CTRL_SET_ECDH_AUTO" test was incorrectly used
> instead of "#ifdef SSL_CTRL_SET_ECDH_AUTO". There is no practical
> difference, since SSL_CTRL_SET_ECDH_AUTO evaluates to a non-zero numeric
> value when defined, but anyway it's better to correctly test if the value
> is defined.
>
> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -1426,7 +1426,7 @@ ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_s
>
> SSL_CTX_set_options(ssl->ctx, SSL_OP_SINGLE_ECDH_USE);
>
> -#if SSL_CTRL_SET_ECDH_AUTO
> +#ifdef SSL_CTRL_SET_ECDH_AUTO
> /* not needed in OpenSSL 1.1.0+ */
> SSL_CTX_set_ecdh_auto(ssl->ctx, 1);
> #endif
>

Looks good.

> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1662434808 -10800
> # Tue Sep 06 06:26:48 2022 +0300
> # Node ID 5e493208769dd754253f567405c05b3cf5f05c8a
> # Parent 2642d97294f0e09c1fd67e434aae759768805617
> Win32: disabled threads support in OpenSSL builds.
>
> Threads are disabled during UNIX builds (see b329c0ab1a48), and also not
> needed for Windows builds.
>
> This used to be the default before OpenSSL 1.1.0.
>
> diff -r 2642d97294f0 -r 5e493208769d auto/lib/openssl/makefile.msvc
> --- a/auto/lib/openssl/makefile.msvc Tue Sep 06 05:48:19 2022 +0300
> +++ b/auto/lib/openssl/makefile.msvc Tue Sep 06 06:26:48 2022 +0300
> @@ -6,7 +6,7 @@
> all:
> cd $(OPENSSL)
>
> - perl Configure VC-WIN32 no-shared \
> + perl Configure VC-WIN32 no-shared no-threads \
> --prefix="%cd%/openssl" \
> --openssldir="%cd%/openssl/ssl" \
> $(OPENSSL_OPT)
>
>

Looks good.
Does it make sense to apply this to makefile.bcc for consistency?

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org