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

["Long, Qin" <qin.long@intel.com>]

Gary, I missed your comment.
Yes, I didn't notice this fix was already merged. 
Since 1.1.0xx already merge this OPENSSL_SYS_UEFI handling , I think we can simplify our side with direct AsciiStrCmp mapping in CRT wrapper. 

Thank you so much for catching this one.


Best Regards & Thanks,
LONG, Qin

> -----Original Message-----
> From: Gary Lin [mailto:glin@suse.com]
> Sent: Wednesday, March 22, 2017 6:11 PM
> To: Long, Qin <qin.long@intel.com>
> Cc: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org; Ye, Ting
> <ting.ye@intel.com>; David Woodhouse <dwmw2@infradead.org>;
> ronald.cron@arm.com; Wu, Jiaxin <jiaxin.wu@intel.com>;
> lersek@redhat.com
> Subject: Re: [edk2] [PATCH v1 3/9] CryptoPkg: Fix handling of &strcmp
> function pointers
> 
> 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