Re: [PATCH v1 3/9] CryptoPkg: Fix handling of &strcmp function pointers

[Gary Lin <glin@suse.com>]

On Tue, Mar 21, 2017 at 11:56:06PM +0800, Qin Long wrote:
> In a couple of places, OpenSSL code uses the address of the strcmp()
> function, and assigns it to another comparator function pointer.
> 
> Unfortunately, this falls foul of the inconsistent function ABI that we
> use in EDKII. We '#define strcmp AsciiStrCmp' but AsciiStrCmp is an
> EFIAPI function with the Microsoft ABI. And we're assigning its address
> to a non-EFIAPI function, which may well have a different ABI.
> 
> The compiler *should* have complained about this error, thus:
> 
> …/crypto/objects/o_names.c: In function ‘OBJ_NAME_new_index’:
> …/crypto/objects/o_names.c:94:30: error: assignment from incompatible
>  pointer type [-Werror=incompatible-pointer-types]
>          name_funcs->cmp_func = OPENSSL_strcmp;
>                               ^
> There's another one in crypto/lhash/lhash.c::lh_new() which has an
> explicit cast so even with compiler warnings we wouldn't have seen it.
> 
> Fix this by providing an actual strcmp() function in the default ABI.
> We already *had* a prototype for it in OpenSslSupport.h, which was then
> superseded by the #define strcmp AsciiStrCmp.
> 
> Now, OpenSSL code *can* use &strcmp without problems.

With this patch, we probably can remove "defined(OPENSSL_SYS_UEFI)" from
https://github.com/openssl/openssl/blob/master/crypto/objects/o_names.c#L32

Gary Lin
> 
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Ronald Cron <ronald.cron@arm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
>  CryptoPkg/Include/OpenSslSupport.h                | 3 +--
>  CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/CryptoPkg/Include/OpenSslSupport.h b/CryptoPkg/Include/OpenSslSupport.h
> index 91567c78f8..c3c5b5dcd7 100644
> --- a/CryptoPkg/Include/OpenSslSupport.h
> +++ b/CryptoPkg/Include/OpenSslSupport.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Root include file to support building OpenSSL Crypto Library.
>  
> -Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -275,7 +275,6 @@ extern FILE  *stdout;
>  #define memchr(buf,ch,count)              ScanMem8(buf,(UINTN)(count),(UINT8)ch)
>  #define memcmp(buf1,buf2,count)           (int)(CompareMem(buf1,buf2,(UINTN)(count)))
>  #define memmove(dest,source,count)        CopyMem(dest,source,(UINTN)(count))
> -#define strcmp                            AsciiStrCmp
>  #define strncmp(string1,string2,count)    (int)(AsciiStrnCmp(string1,string2,(UINTN)(count)))
>  #define strcpy(strDest,strSource)         AsciiStrCpyS(strDest,MAX_STRING_SIZE,strSource)
>  #define strncpy(strDest,strSource,count)  AsciiStrnCpyS(strDest,MAX_STRING_SIZE,strSource,(UINTN)count)
> diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> index 9d6867ebce..e8a76d07ff 100644
> --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> @@ -2,7 +2,7 @@
>    Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
>    Cryptographic Library.
>  
> -Copyright (c) 2010 - 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #include <Base.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/BaseLib.h>
>  
>  /* OpenSSL will use floating point support, and C compiler produces the _fltused
>     symbol by default. Simply define this symbol here to satisfy the linker. */
> @@ -44,3 +45,8 @@ void * memset (void *dest, char ch, unsigned int count)
>    
>    return dest;
>  }
> +
> +int strcmp (const char *s1, const char *s2)
> +{
> +  return (int)AsciiStrCmp(s1, s2);
> +}
> -- 
> 2.11.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel