Hello!

On Thu, Sep 08, 2022 at 02:06:28PM +0200, Alejandro Colomar wrote:

> Hi Sergey,
>
> On 9/8/22 13:31, Sergey Kandaurov wrote:
> > details: https://hg.nginx.org/nginx/rev/ba5cf8f73a2d
> > branches:
> > changeset: 8070:ba5cf8f73a2d
> > user: Sergey Kandaurov <pluknet@nginx.com>
> > date: Thu Sep 08 13:53:49 2022 +0400
> > description:
> > 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.
>
> I think this workaround is incorrect, and the problem is in the buildsystem.
>
> See gcc(1):
>
> -I dir
> -iquote dir
> -isystem dir
> -idirafter dir
> ...
>
> You can use -I to override a system header file,
> substituting your own version, since these
> directories are searched before the standard system
> header file directories. However, you should not use
> this option to add directories that contain vendor‐
> supplied system header files; use -isystem for that.
>
> The -isystem and -idirafter options also mark the
> directory as a system directory, so that it gets the
> same special treatment that is applied to the
> standard system directories.
>
> ...
>
>
> Basically, -isystem works as -I, but disables warnings caused by system
> headers.

[...]

> Of course, this is considering that you normally don't want to get
> warnings from dubious system headers, which normally should be the case
> in user applications, but you may legitimately doubt the correctness of
> some dependencies, and may want to see the warnings...

In no particular order:

- The "-isystem" flag is GCC-specific and not portable.

- SSL library headers are not required to be in the system
headers, and can be instead provided by user via --with-cc-opt /
--with-ld-opt (and, in case of BoringSSL, this is usually the
case).

- Disabling warnings for system headers is at most a workaround,
and a bad one. If there are warnings, these should be addressed
(or silenced, or fixed in the compiler), and not ignored because
they happen to come from the system headers. In particular, this
ensures that the application code won't silently blow up if a
particular system implemented something slightly differently than
others, and the application do not take this into account.
Disabling warnings for system headers only make sense if there is
a bug in system headers you cannot reasonably fix or silence from
the application.

In this particular case, the warning is somewhat valid: the return
value of the SSL_CTX_set_ecdh_auto() call is ignored by nginx.
The only issue is that for some reason GCC isn't smart enough to
understand that this is a function(-like) call, and the computed
value isn't the only potential result.

Casting the returned value to "(void)" explicitly tells GCC that
we are not going to use the result. Another option would be to
add proper error checking, but this is rather unneeded, given that
errors here might only happen due to incorrect library version
being used at runtime, only potentially affects ancient library
versions, and we cannot do anything reasonable with this anyway.

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