Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source

[Laszlo Ersek <lersek@redhat.com>]

On 03/31/17 19:05, Qin Long wrote:
> This patch added some extra build options to suppress possible warnings
> when building openssl source under GCC48 and VS2010. Including:
> 
> Adding "-Wno-error=maybe-uninitialized" to suppress the following GCC48
> build warning:
>   OpensslLib/openssl/ssl/statem/statem_clnt.c:2543:9: error: "len" may
>      be used uninitialized in this function [-Werror=maybe-uninitialized]
>        len += pskhdrlen;
>            ^

This happens in the tls_construct_client_key_exchange() function. After
reviewing the function, the warning is indeed incorrect. I agree with
the fix (adding -Wno-error=maybe-uninitialized), but I propose the
following too:

- file an upstream OpenSSL bug report about this (the existent "len = 0"
assignment can be moved to the unconditional part of the code),
- file a TianoCore BZ as well, so that we don't forget removing
-Wno-error=maybe-uninitialized once the OpenSSL bug is fixed and we
adopt the next stable release with the OpenSSL change.

The point is, I think -Werror=maybe-uninitialized is a valuable warning,
and it might help us catch an actual bug in OpenSSL later on. So I don't
think it should be turned off permanently.

> And adding "/wd4306" to suppress the following VS2010 build warning:
>   openssl\crypto\asn1\tasn_dec.c(795) : warning C4306: 'type cast' :
>                conversion from 'int' to 'ASN1_VALUE *' of greater size

That again seems like a valid warning, the C standard doesn't guarantee
that (int) can be converted to a pointer type and vice versa. The code
should be using intptr_t or uintptr_t (in edk2 we use UINTN for that).

This is the code in question:

    case V_ASN1_NULL:
        if (len) {
            ASN1err(ASN1_F_ASN1_EX_C2I, ASN1_R_NULL_IS_WRONG_LENGTH);
            goto err;
        }
        *pval = (ASN1_VALUE *)1;
        break;

This is really awful. But, if it is necessary, it should be

  *pval = (ASN1_VALUE *)(uintptr_t)1;

Again, I think it's OK to suppress the warnings for now, but I think we
should have a longer term reminder to reenable them.

> 
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf       | 15 ++++++++++-----
>  CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 ++++++++++-----
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> index 42f72f4f1f..cbabb34bdd 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> @@ -535,21 +535,26 @@
>    #   C4244: conversion from type1 to type2, possible loss of data
>    #   C4245: conversion from type1 to type2, signed/unsigned mismatch
>    #   C4267: conversion from size_t to type, possible loss of data
> +  #   C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
>    #   C4389: 'operator' : signed/unsigned mismatch (xxxx)
>    #   C4702: unreachable code
>    #   C4706: assignment within conditional expression
>    #
>    MSFT:*_*_IA32_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> -  MSFT:*_*_X64_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> -  MSFT:*_*_IPF_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> +  MSFT:*_*_X64_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
> +  MSFT:*_*_IPF_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
>  
>    INTEL:*_*_IA32_CC_FLAGS  = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>    INTEL:*_*_X64_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>    INTEL:*_*_IPF_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>  
> -  GCC:*_*_IA32_CC_FLAGS    = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> -  GCC:*_*_X64_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
> -  GCC:*_*_IPF_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> +  #
> +  # Suppress the following build warnings in openssl so we don't break the build with -Werror
> +  #   -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.

If that was true, that would be an actual bug in the OpenSSL code base,
and suppressing the warning in such a case would also be a bug. We may
only suppress the warning because it is *invalid* -- there is no such
problematic code path, only the compiler (incorrectly) thinks there is.

So please update the comment. I would even go as far as including the
TianoCore BZ number in this comment, for the BZ which tracks the
longer-term re-enablement of this warning.

Looks good to me otherwise.

(Note: personally I can't reproduce the build issues that I reported, at
least with GCC48 that I use.)

Thanks!
Laszlo

> +  #
> +  GCC:*_*_IA32_CC_FLAGS    = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> +  GCC:*_*_X64_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> +  GCC:*_*_IPF_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
>    GCC:*_*_ARM_CC_FLAGS     = $(OPENSSL_FLAGS)
>    GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
>  
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> index cbbb456d70..026b551bca 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> @@ -496,21 +496,26 @@
>    #   C4244: conversion from type1 to type2, possible loss of data
>    #   C4245: conversion from type1 to type2, signed/unsigned mismatch
>    #   C4267: conversion from size_t to type, possible loss of data
> +  #   C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
>    #   C4389: 'operator' : signed/unsigned mismatch (xxxx)
>    #   C4702: unreachable code
>    #   C4706: assignment within conditional expression
>    #
>    MSFT:*_*_IA32_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> -  MSFT:*_*_X64_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> -  MSFT:*_*_IPF_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> +  MSFT:*_*_X64_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
> +  MSFT:*_*_IPF_CC_FLAGS    = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
>  
>    INTEL:*_*_IA32_CC_FLAGS  = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>    INTEL:*_*_X64_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>    INTEL:*_*_IPF_CC_FLAGS   = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>  
> -  GCC:*_*_IA32_CC_FLAGS    = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> -  GCC:*_*_X64_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
> -  GCC:*_*_IPF_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> +  #
> +  # Suppress the following build warnings in openssl so we don't break the build with -Werror
> +  #   -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.
> +  #
> +  GCC:*_*_IA32_CC_FLAGS    = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> +  GCC:*_*_X64_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> +  GCC:*_*_IPF_CC_FLAGS     = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
>    GCC:*_*_ARM_CC_FLAGS     = $(OPENSSL_FLAGS)
>    GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
>  
>