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

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

Looks there are other places (e.g. lhash.c) also have this function-mapping issue.
OK, let's keep this patch.

And o_names.c can be simplified then. (Will submit the patch to openssl part).


Best Regards & Thanks,
LONG, Qin

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Long, Qin
> Sent: Thursday, March 23, 2017 10:16 AM
> To: Gary Lin
> Cc: ard.biesheuvel@linaro.org; Ye, Ting; lersek@redhat.com; edk2-
> devel@lists.01.org; ronald.cron@arm.com; Wu, Jiaxin; David Woodhouse
> Subject: Re: [edk2] [PATCH v1 3/9] CryptoPkg: Fix handling of &strcmp
> function pointers
> 
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel