Hello!

On Wed, Sep 07, 2022 at 07:33:23PM +0400, Sergey Kandaurov wrote:

> > On 6 Sep 2022, at 07:49, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > On Mon, Sep 05, 2022 at 10:44:06PM +0400, Sergey Kandaurov wrote:
> >
> >> 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.
> >
> > [..]
> > # 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
> >
>
> Just noted that BoringSSL uses stubs, which makes GCC unhappy.
>
> // SSL_CTX_set_ecdh_auto returns one.
> #define SSL_CTX_set_ecdh_auto(ctx, onoff) 1
> #define SSL_CTRL_SET_ECDH_AUTO doesnt_exist
>
> Patch to address this below:
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1662564729 -14400
> # Wed Sep 07 19:32:09 2022 +0400
> # Node ID e7997671018932c6cc97e1debf95b515e4d39e3c
> # Parent a423e314c22fe99fe9faf28f033c266426993105
> SSL: silenced GCC warnings when building with BoringSSL.
>
> BoringSSL uses macro stub for SSL_CTX_set_ecdh_auto that expands to 1,
> which triggers -Wunused-value "statement with no effect" warnings.
>
> diff -r a423e314c22f -r e79976710189 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c Wed Sep 07 00:47:31 2022 +0300
> +++ b/src/event/ngx_event_openssl.c Wed Sep 07 19:32:09 2022 +0400
> @@ -1428,7 +1428,7 @@ ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_s
>
> #ifdef SSL_CTRL_SET_ECDH_AUTO
> /* not needed in OpenSSL 1.1.0+ */
> - SSL_CTX_set_ecdh_auto(ssl->ctx, 1);
> + (void) SSL_CTX_set_ecdh_auto(ssl->ctx, 1);
> #endif
>
> if (ngx_strcmp(name->data, "auto") == 0) {

Looks good.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org