Re: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

["Gao, Liming" <liming.gao@intel.com>]

Ard and Laszlo:
  Thanks for your quick fix. For my patch, I think it only impacts VS tool chain. So, I don't verify GCC tool chain. Sorry for it.  

Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Thursday, July 06, 2017 2:34 AM
>To: edk2-devel@lists.01.org; lersek@redhat.com
>Cc: leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zhu,
>Yonghong <yonghong.zhu@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory
>entry size
>
>Currently, the PE/COFF conversion routines in GenFw add a so-called
>NB10 CodeView debug record to the image, and update the associated
>directory entry in the PE/COFF optional header to contain its relative
>virtual address (RVA) and size.
>
>However, there are two levels of indirection at work here: the actual
>NB10 CodeView record (which is simply a magic number and some unused
>data fields followed by the NUL terminated filename) is emitted
>separately, and a separate descriptor is emitted that identifies the
>NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and
>records
>its size. The directory entry in the PE/COFF optional header should
>refer to this intermediate descriptor's address and size only, but
>the WriteDebug## () routines in GenFw erroneously record the size of
>both the descriptor and the NB10 CodeView record.
>
>This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
>GenFw to clear unused debug entry generated by VS tool chain",
>2017-06-19), and GenFw now crashes when it attempts to iterate over
>what it thinks are multiple intermediate descriptors for different
>kinds of debug data embedded in the image.
>
>The error is understandable, given that both are carved out of the
>same file space allocation, but this is really an implementation detail
>of GenFw, and is not required. (Note that the intermediate descriptor
>does not require a RVA and so it does not even need to be inside a
>section)
>
>So omit the size of the NB10 CodeView record from the size recorded
>in the optional header.
>
>Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Co-debugged-or-whatever-by: Laszlo Ersek <lersek@redhat.com>
>---
> BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
> BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c
>b/BaseTools/Source/C/GenFw/Elf32Convert.c
>index f7b084dc9b84..14fe4a285857 100644
>--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
>@@ -1142,7 +1142,7 @@ WriteDebug32 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = &NtHdr-
>>Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>b/BaseTools/Source/C/GenFw/Elf64Convert.c
>index 7eed7b92d30f..c39bdff063ab 100644
>--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>@@ -1095,7 +1095,7 @@ WriteDebug64 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = &NtHdr-
>>Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DE
>BUG];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>--
>2.9.3