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

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/17 20:33, Ard Biesheuvel wrote:
> 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

Please prepend:

Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012162.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>

Haha, great :)

> ---
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
>  BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks, Ard!
Laszlo

> 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_DEBUG];
>    DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>  }
>  
>  STATIC
>