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

[Laszlo Ersek <lersek@redhat.com>]

On 07/06/17 05:32, Gao, Liming wrote:
> 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 (and obviously: Ard), pushed as commit 60e85a39fe49.

Laszlo

>> -----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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>