Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-03-12 Thread Andrew Fish 

> On Mar 12, 2016, at 3:39 PM, Daryl McDaniel <edk2-li...@mc2research.org> 
> wrote:
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, February 25, 2016 2:01 AM
>> To: David Woodhouse <dw...@infradead.org>; edk2-devel-01 
>> <edk2-de...@ml01.01.org>
>> Cc: Eric Dong <eric.d...@intel.com>; Cecil Sheng <cecil.sh...@hpe.com>; Ting 
>> Ye
>> <ting...@intel.com>; Qiu Shumin <shumin@intel.com>; Qin Long
>> <qin.l...@intel.com>; Liming Gao <liming@intel.com>; Yao Jiewen
>> <jiewen@intel.com>; Daryl McDaniel <edk2-li...@mc2research.org>; Jaben 
>> Carsey
>> <jaben.car...@intel.com>; Samer El-Haj-Mahmoud <el...@hpe.com>
>> Subject: Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) 
>> conformance
>> improvements
>> 
>> On 02/25/16 03:04, David Woodhouse wrote:
>>> On Wed, 2016-02-24 at 22:13 +0100, Laszlo Ersek wrote:
>>>> The free() wrapper in BaseCryptLib has a bug that has been triggered
>>>> by David's recent OpenSSL work. The series fixes the bug, plus
>>>> more instances of the same.
>>> 
>>> Should we not just fix the underlying FreePool() function to do the
>>> sane thing.
>> 
>> It crossed my mind, but FreePool() is extremely widely used, it has a
>> detailed interface contract, and its current behavior matches its
>> interface contract. I'm not up to auditing all uses of FreePool(), to
>> find the one that perversely enforces a failure return form
>> FreePool(NULL). :)
>> 
>>> Anyway, I've rebased my tree on top of yours,
>> 
>> Thanks -- I'll push the first three patches to edk2 master in a minute,
>> and I'll post a new version of the fourth.
>> 
>>> split up the patch
>>> changes into separate bisectable commits, and pushed my tree out again
>>> to http://git.infradead.org/users/dwmw2/edk2.git
>> 
>> I skimmed your fresh master -- the way the patch evolves looks
>> excellent. I guess it took a lot of effort.
>> 
>>> Both Cryptest.efi and the test boot of Fedora 22 are working correctly
>>> at all stages.
>> 
>> Perfect.
>> 
>>> Again, the final two commits aren't ready yet. But the rest probably
>>> are if they build OK on Windows.
>>> 
>>> I do still want to kill that -w. And why in $DEITY's name do we not
>>> already have -nostdinc in our CFLAGS for the whole EDK2 build?
> 
> The -nostdinc equivalent for Visual Studio breaks the NT32 build and there is 
> no way to negate the flag once it has been specified.
> 
> There would have to be a new target added just for NT32 builds.
> 

Would it be possible to fix the NT32 issue in a n INF file? It looks like the 
NT32 App is using == to override compiler flags?

https://github.com/tianocore/edk2/blob/master/Nt32Pkg/Sec/SecMain.inf

[BuildOptions]

  MSFT:*_*_IA32_DLINK_FLAGS == /out:"$(BIN_DIR)\SecMain.exe" /base:0x1000 
/pdb:"$(BIN_DIR)\SecMain.pdb" /LIBPATH:"$(VCINSTALLDIR)\Lib" 
/LIBPATH:"$(VCINSTALLDIR)\PlatformSdk\Lib" /NOLOGO /SUBSYSTEM:CONSOLE 
/NODEFAULTLIB /IGNORE:4086 /MAP /OPT:REF /DEBUG /MACHINE:I386 /LTCG 
Kernel32.lib MSVCRTD.lib Gdi32.lib User32.lib Winmm.lib Advapi32.lib

  MSFT:*_VS2015_IA32_DLINK_FLAGS == /out:"$(BIN_DIR)\SecMain.exe" 
/base:0x1000 /pdb:"$(BIN_DIR)\SecMain.pdb" /LIBPATH:"$(VCINSTALLDIR)\Lib" 
/LIBPATH:"$(VCINSTALLDIR)\PlatformSdk\Lib" /NOLOGO /SUBSYSTEM:CONSOLE 
/NODEFAULTLIB /IGNORE:4086 /MAP /OPT:REF /DEBUG /MACHINE:I386 /LTCG 
Kernel32.lib MSVCRTD.lib vcruntimed.lib ucrtd.lib Gdi32.lib User32.lib 
Winmm.lib Advapi32.lib

  MSFT:*_VS2015x86_IA32_DLINK_FLAGS == /out:"$(BIN_DIR)\SecMain.exe" 
/base:0x1000 /pdb:"$(BIN_DIR)\SecMain.pdb" /LIBPATH:"$(VCINSTALLDIR)\Lib" 
/LIBPATH:"$(VCINSTALLDIR)\PlatformSdk\Lib" /NOLOGO /SUBSYSTEM:CONSOLE 
/NODEFAULTLIB /IGNORE:4086 /MAP /OPT:REF /DEBUG /MACHINE:I386 /LTCG 
Kernel32.lib MSVCRTD.lib vcruntimed.lib ucrtd.lib Gdi32.lib User32.lib 
Winmm.lib Advapi32.lib

  MSFT:*_*_IA32_CC_FLAGS == /nologo /W4 /WX /Gy /c /D UNICODE /Od /FIAutoGen.h 
/EHs-c- /GF /Gs8192 /Zi /Gm /D _CRT_SECURE_NO_WARNINGS /D 
_CRT_SECURE_NO_DEPRECATE

  MSFT:*_*_IA32_PP_FLAGS == /nologo /E /TC /FIAutoGen.h

  MSFT:*_*_IA32_ASM_FLAGS == /nologo /W3 /WX /c /coff /Cx /Zd /W0 /Zi

  MSFT:*_*_IA32_ASMLINK_FLAGS   == /link /nologo /tiny


  MSFT:*_*_X64_DLINK_FLAGS == /out:"$(BIN_DIR)\SecMain.exe" /base:0x1000 
/pdb:"$(BIN_DIR)\SecMain.pdb" /LIBPATH:"$(VCINSTALLDIR)\Lib\AMD64" 
/LIBPATH:"$(V

Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-03-12 Thread Daryl McDaniel 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, February 25, 2016 2:01 AM
> To: David Woodhouse <dw...@infradead.org>; edk2-devel-01 
> <edk2-de...@ml01.01.org>
> Cc: Eric Dong <eric.d...@intel.com>; Cecil Sheng <cecil.sh...@hpe.com>; Ting 
> Ye
> <ting...@intel.com>; Qiu Shumin <shumin@intel.com>; Qin Long
> <qin.l...@intel.com>; Liming Gao <liming@intel.com>; Yao Jiewen
> <jiewen@intel.com>; Daryl McDaniel <edk2-li...@mc2research.org>; Jaben 
> Carsey
> <jaben.car...@intel.com>; Samer El-Haj-Mahmoud <el...@hpe.com>
> Subject: Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance
> improvements
> 
> On 02/25/16 03:04, David Woodhouse wrote:
> > On Wed, 2016-02-24 at 22:13 +0100, Laszlo Ersek wrote:
> >> The free() wrapper in BaseCryptLib has a bug that has been triggered
> >> by David's recent OpenSSL work. The series fixes the bug, plus
> >> more instances of the same.
> >
> > Should we not just fix the underlying FreePool() function to do the
> > sane thing.
> 
> It crossed my mind, but FreePool() is extremely widely used, it has a
> detailed interface contract, and its current behavior matches its
> interface contract. I'm not up to auditing all uses of FreePool(), to
> find the one that perversely enforces a failure return form
> FreePool(NULL). :)
> 
> > Anyway, I've rebased my tree on top of yours,
> 
> Thanks -- I'll push the first three patches to edk2 master in a minute,
> and I'll post a new version of the fourth.
> 
> > split up the patch
> > changes into separate bisectable commits, and pushed my tree out again
> > to http://git.infradead.org/users/dwmw2/edk2.git
> 
> I skimmed your fresh master -- the way the patch evolves looks
> excellent. I guess it took a lot of effort.
> 
> > Both Cryptest.efi and the test boot of Fedora 22 are working correctly
> > at all stages.
> 
> Perfect.
> 
> > Again, the final two commits aren't ready yet. But the rest probably
> > are if they build OK on Windows.
> >
> > I do still want to kill that -w. And why in $DEITY's name do we not
> > already have -nostdinc in our CFLAGS for the whole EDK2 build?

The -nostdinc equivalent for Visual Studio breaks the NT32 build and there is 
no way to negate the flag once it has been specified.

There would have to be a new target added just for NT32 builds.

> I propose to involve our BaseTools overlords here...
> 
> Thanks
> Laszlo

Daryl McDaniel


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-02-25 Thread David Woodhouse 
On Thu, 2016-02-25 at 11:01 +0100, Laszlo Ersek wrote:
> 
> > Anyway, I've rebased my tree on top of yours,
> 
> Thanks -- I'll push the first three patches to edk2 master in a minute,
> and I'll post a new version of the fourth.

You've committed the one I need. Thanks.

> > split up the patch
> > changes into separate bisectable commits, and pushed my tree out again
> > to http://git.infradead.org/users/dwmw2/edk2.git
> 
> I skimmed your fresh master -- the way the patch evolves looks
> excellent. I guess it took a lot of effort.

Actually it wasn't that much effort in the end — I'd already *done* the
fun part, which was identifying the various changes which were all
mashed together into our EDKII_openssl patch and getting them into
suitable shape for upstream.

And the "new" patch, which we end up with, is just generated from a
series of clean commits in a git tree. I'm not *completely* insane :)

So all I really needed to do was 

  git checkout 1.0.2f
  patch -p0 < EDKII_openssl patch
  git commit -m rt3992 crypto/x509v3/ext_dat.h
  git commit -m rt3951 crypto/x509/x509_vfy.[ch]
  ...etc.

In all but one case, it really was that simple because the separate
logical changes all touched *different* files. One time I had to use
'git commit --interactive' because two logical changes touched the same
file.

Then for each logical change, it was just a case of reverting the
original version I'd just committed from the EDKII_openssl patch, and
applying the backported version of the same change from upstream.
And 'git diff 1.0.2f.. > unix2dos > EDKII_openssl.patch at each stage.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-02-25 Thread Laszlo Ersek 
On 02/25/16 03:04, David Woodhouse wrote:
> On Wed, 2016-02-24 at 22:13 +0100, Laszlo Ersek wrote:
>> The free() wrapper in BaseCryptLib has a bug that has been triggered
>> by David's recent OpenSSL work. The series fixes the bug, plus
>> more instances of the same.
> 
> Should we not just fix the underlying FreePool() function to do the
> sane thing.

It crossed my mind, but FreePool() is extremely widely used, it has a
detailed interface contract, and its current behavior matches its
interface contract. I'm not up to auditing all uses of FreePool(), to
find the one that perversely enforces a failure return form
FreePool(NULL). :)

> Anyway, I've rebased my tree on top of yours,

Thanks -- I'll push the first three patches to edk2 master in a minute,
and I'll post a new version of the fourth.

> split up the patch
> changes into separate bisectable commits, and pushed my tree out again
> to http://git.infradead.org/users/dwmw2/edk2.git

I skimmed your fresh master -- the way the patch evolves looks
excellent. I guess it took a lot of effort.

> Both Cryptest.efi and the test boot of Fedora 22 are working correctly
> at all stages.

Perfect.

> Again, the final two commits aren't ready yet. But the rest probably
> are if they build OK on Windows.
> 
> I do still want to kill that -w. And why in $DEITY's name do we not
> already have -nostdinc in our CFLAGS for the whole EDK2 build?

I propose to involve our BaseTools overlords here...

Thanks
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-02-24 Thread Long, Qin 
Great.
I will sync-up these changes and follow the windows / VS toolchain 
validations.


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Thursday, February 25, 2016 10:05 AM
> To: Laszlo Ersek; edk2-devel-01
> Cc: Dong, Eric; Cecil Sheng; Ye, Ting; Qiu, Shumin; Long, Qin; Gao, Liming; 
> Yao,
> Jiewen; Daryl McDaniel; Carsey, Jaben; Samer El-Haj-Mahmoud
> Subject: Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size)
> conformance improvements
>
> On Wed, 2016-02-24 at 22:13 +0100, Laszlo Ersek wrote:
> > The free() wrapper in BaseCryptLib has a bug that has been triggered
> > by David's recent OpenSSL work. The series fixes the bug, plus
> > more instances of the same.
>
> Should we not just fix the underlying FreePool() function to do the
> sane thing.
>
> Anyway, I've rebased my tree on top of yours, split up the patch
> changes into separate bisectable commits, and pushed my tree out again
> to http://git.infradead.org/users/dwmw2/edk2.git
>
> Both Cryptest.efi and the test boot of Fedora 22 are working correctly
> at all stages.
>
> Laszlo Ersek (4):
>   CryptoPkg: BaseCryptLib: support free(NULL)
>
>  CryptoPkg: RuntimeCryptLib: support free(NULL)
>   CryptoPkg:
> RuntimeCryptLib: support realloc(NULL, size)
>   MdeModulePkg:
> RegularExpressionDxe: support free(NULL)
>
> David Woodhouse (14):
>   CryptoPkg: Use OpenSSL include directory directly
>   CryptoPkg/OpensslLib: Include complete copy of opensslconf.h
>   CryptoPkg/OpensslLib: Regenerate OpenSSL patch
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#4175
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3964
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3628
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3955
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3674
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3951
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3992
>   CryptoPkg/OpensslLib: Switch to upstream fix for OpenSSL RT#3969
>   CryptoPkg/OpensslLib: Automatically configure OpenSSL and generate 
> file
> list
>   CryptoPkg: Support building with OpenSSL HEAD (1.1.0-devel)
>   CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work
>
> Again, the final two commits aren't ready yet. But the rest probably
> are if they build OK on Windows.
>
> I do still want to kill that -w. And why in $DEITY's name do we not
> already have -nostdinc in our CFLAGS for the whole EDK2 build?
>
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

2016-02-24 Thread Laszlo Ersek 
The free() wrapper in BaseCryptLib has a bug that has been triggered
by David's recent OpenSSL work. The series fixes the bug, plus
more instances of the same.

Here not only those free() and realloc() functions should be mentioned
that this series patches, but also those that are *not* patched.

Let's see the untouched free() implementations:

- StdLib/LibC/StdLib/Malloc.c:

  Handles NULL correctly.

- BaseTools/Source/C/Common/MyAlloc.[hc]:

  The MyFree() function handles NULL correctly.

The untouched realloc() implementations:

- StdLib/LibC/StdLib/Malloc.c:

  Handles NULL correctly.

- MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h:

  The prototype of realloc() differs from the one seen in standard C.

- CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c:

  For a NULL pointer, the current implementation (i.e., calling
  ReallocatePool (size, size, NULL)) conforms to the ReallocatePool()
  specification, and has the intended effect.

- BaseTools/Source/C/Common/MyAlloc.[hc]:

  The MyRealloc() function handles NULL correctly.

Thanks
Laszlo

Cc: Cecil Sheng 
Cc: Cinnamon Shia 
Cc: Daryl McDaniel 
Cc: David Woodhouse 
Cc: Eric Dong 
Cc: Jaben Carsey 
Cc: Liming Gao 
Cc: Qin Long 
Cc: Qiu Shumin 
Cc: Samer El-Haj-Mahmoud 
Cc: Ting Ye 
Cc: Yao Jiewen 
Cc: Yonghong Zhu 

Laszlo Ersek (4):
  CryptoPkg: BaseCryptLib: support free(NULL)
  CryptoPkg: RuntimeCryptLib: support free(NULL)
  CryptoPkg: RuntimeCryptLib: support realloc(NULL, size)
  MdeModulePkg: RegularExpressionDxe: support free(NULL)

 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h | 12 
+++-
 CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c|  8 
+++-
 CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c | 12 
+++-
 3 files changed, 29 insertions(+), 3 deletions(-)

-- 
1.8.3.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel