[nginx] SSL: silenced GCC warnings when building with BoringSSL.

[Maxim Dounin]

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 at 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/