Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/17 14:42, Laszlo Ersek wrote:
> Hi Liming,
> 
> Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
> 
> I can reproduce the segfault locally. This is the command line:
> 
>> GenFw \
>>   -e DXE_RUNTIME_DRIVER \
>>   -o .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi \
>>   Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
> 
> Backtrace from gdb:
> 
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
>> Missing separate debuginfos, use: debuginfo-install glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
>> (gdb) where
>> #0  0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
>> #1  0x0000000000407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", ZeroDebugFlag=0 '\000') at GenFw.c:2898
>> #2  0x0000000000406a4d in main (argc=0, argv=0x7fffffffd0b8) at GenFw.c:2591
> 
> See below for the SIGSEGV location:
> 
> On 07/03/17 07:21, Liming Gao wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=600
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>> ---
>>  BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
>> index 22e4e72..6569460 100644
>> --- a/BaseTools/Source/C/GenFw/GenFw.c
>> +++ b/BaseTools/Source/C/GenFw/GenFw.c
>> @@ -2770,6 +2770,7 @@ Returns:
>>  {
>>    UINT32                           Index;
>>    UINT32                           DebugDirectoryEntryRva;
>> +  UINT32                           DebugDirectoryEntrySize;
>>    UINT32                           DebugDirectoryEntryFileOffset;
>>    UINT32                           ExportDirectoryEntryRva;
>>    UINT32                           ExportDirectoryEntryFileOffset;
>> @@ -2781,12 +2782,14 @@ Returns:
>>    EFI_IMAGE_OPTIONAL_HEADER64     *Optional64Hdr;
>>    EFI_IMAGE_SECTION_HEADER        *SectionHeader;
>>    EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
>> +  EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
>>    UINT32                          *NewTimeStamp;
>>
>>    //
>>    // Init variable.
>>    //
>>    DebugDirectoryEntryRva           = 0;
>> +  DebugDirectoryEntrySize          = 0;
>>    ExportDirectoryEntryRva          = 0;
>>    ResourceDirectoryEntryRva        = 0;
>>    DebugDirectoryEntryFileOffset    = 0;
>> @@ -2822,6 +2825,7 @@ Returns:
>>      if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>>          Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
>>        DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> +      DebugDirectoryEntrySize = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>>        if (ZeroDebugFlag) {
>>          Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
>>          Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
>> @@ -2841,6 +2845,7 @@ Returns:
>>      if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>>          Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
>>        DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> +      DebugDirectoryEntrySize = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>>        if (ZeroDebugFlag) {
>>          Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
>>          Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
>> @@ -2886,11 +2891,23 @@ Returns:
>>
>>    if (DebugDirectoryEntryFileOffset != 0) {
>>      DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
>> -    DebugEntry->TimeDateStamp = 0;
>> -    mImageTimeStamp = 0;
>> -    if (ZeroDebugFlag) {
>> -      memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>> -      memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>> +    Index = 0;
>> +    for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
>> +      DebugEntry->TimeDateStamp = 0;
>> +      if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>> +        memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
> 
> This memset() is the culprit.
> 
> According to gdb (all values decimal),
> - Index = 1,
> - DebugDirectoryEntrySize = 237,
> - ZeroDebugFlag = 0.
> 
> These values look suspicious, because
> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
> DebugDirectoryEntrySize (237) is not an integral multiple of that.
> 
> This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
> 
>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[0]
>> $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 33292}
> 
> This is the second one (which triggers the crash, Index=1):
> 
>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[1]
>> $12 = {Characteristics = 808534606, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 0, SizeOfData = 1836017711, RVA = 1634479973, FileOffset = 796094307}
> 
> The second element is obviously garbage (FileOffset = 796094307, and
> then the memset() wanders off into the weeds).
> 
> This is the contents of the DataDirectory:
> 
>> (gdb) print Optional64Hdr->DataDirectory
>> $22 = {{VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
>>        {VirtualAddress = 36864, Size = 4096}, // EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
>>        {VirtualAddress = 33264, Size = 237},  // EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_COPYRIGHT
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_GLOBALPTR
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_TLS
>>        {VirtualAddress = 0,     Size = 0},    // EFI_IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG
>>        {VirtualAddress = 0,     Size = 0},
>>        {VirtualAddress = 0,     Size = 0},
>>        {VirtualAddress = 0,     Size = 0},
>>        {VirtualAddress = 0,     Size = 0},
>>        {VirtualAddress = 0,     Size = 0}}
> 
> According to <https://msdn.microsoft.com/en-us/library/ms809762.aspx>,
> the format (element type) of the debug directory is specific to the
> toolchain; the article says,
> 
>> To determine the number of entries in the Microsoft linker-generated
>> debug directory, divide the size of the debug directory (found in the
>> size field of the data directory entry) by the size of an
>> IMAGE_DEBUG_DIRECTORY structure
> 
> Here's a hexdump of the debug directory (237 bytes starting from
> (FileBuffer+33264)):
> 
> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 02 00 00 00  |................|
> 00000010  d1 00 00 00 0c 82 00 00  0c 82 00 00 4e 42 31 30  |Ñ...........NB10|
> 00000020  00 00 00 00 00 00 00 00  00 00 00 00 2f 68 6f 6d  |............/hom|
> 00000030  65 2f 6c 61 63 6f 73 2f  73 72 63 2f 75 70 73 74  |e/lacos/src/upst|
> 00000040  72 65 61 6d 2f 65 64 6b  32 2f 42 75 69 6c 64 2f  |ream/edk2/Build/|
> 00000050  4f 76 6d 66 58 36 34 2f  4e 4f 4f 50 54 5f 47 43  |OvmfX64/NOOPT_GC|
> 00000060  43 34 38 2f 58 36 34 2f  4d 64 65 4d 6f 64 75 6c  |C48/X64/MdeModul|
> 00000070  65 50 6b 67 2f 55 6e 69  76 65 72 73 61 6c 2f 52  |ePkg/Universal/R|
> 00000080  65 70 6f 72 74 53 74 61  74 75 73 43 6f 64 65 52  |eportStatusCodeR|
> 00000090  6f 75 74 65 72 2f 52 75  6e 74 69 6d 65 44 78 65  |outer/RuntimeDxe|
> 000000a0  2f 52 65 70 6f 72 74 53  74 61 74 75 73 43 6f 64  |/ReportStatusCod|
> 000000b0  65 52 6f 75 74 65 72 52  75 6e 74 69 6d 65 44 78  |eRouterRuntimeDx|
> 000000c0  65 2f 44 45 42 55 47 2f  52 65 70 6f 72 74 53 74  |e/DEBUG/ReportSt|
> 000000d0  61 74 75 73 43 6f 64 65  52 6f 75 74 65 72 52 75  |atusCodeRouterRu|
> 000000e0  6e 74 69 6d 65 44 78 65  2e 64 6c 6c 00           |ntimeDxe.dll.|
> 
> The bytes 0x4e 0x42 0x31 0x30 ("NB10"), at offset 28, can be seen above
> as "Characteristics = 808534606". Googling this value, it seems to be a
> signature / magic value: CVINFO_PDB20_CVSIGNATURE.
> 
> Then, VirtualAddress = 33264 is 0x81F0 hex, and "objdump -x" reports:
> 
>> Sections:
>> Idx Name          Size      VMA               LMA               File off  Algn  Flags
>>   [...]
>>   2 .build-id     00000024  00000000000081f0  00000000000081f0  000081f0  2**2  CONTENTS, READONLY
>>   [...]
>> SYMBOL TABLE:
>> [...]
>> 00000000000081f0 l    d  .build-id      0000000000000000 .build-id
>> [...]
> 
> Also, I found a function called pe_bfd_read_buildid() in the GNU
> Binutils source that works with the PE_DEBUG_DATA entry of the data
> directory.
> 
> In "BaseTools/Scripts/GccBase.lds", we have:
> 
>>   /*
>>    * Retain the GNU build id but in a non-allocatable section so GenFw
>>    * does not copy it into the PE/COFF image.
>>    */
>>   .build-id (INFO) : { *(.note.gnu.build-id) }
> 
> This is from commit 7fd5d619806d ("BaseTools GCC: drop GNU notes section
> from EFI image", 2016-07-27).
> 
> So... I don't really know what's happening here, but the DEBUG entry of
> the data directory (i.e., the debug directory) doesn't seem to be
> structured like GenFw expects.

After looking a bit more at the hexdump, it seems that the first entry
in the debug directory is indeed of type
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY, and its Type field is 2
(EFI_IMAGE_DEBUG_TYPE_CODEVIEW), so GenFw branches to the "RsdsEntry" logic.

The second entry however is bogus -- apparently a codeview structure
(containing the build-id) is placed into the debug directory?

typedef struct _CV_INFO_PDB20
{
  char CvHeader[4];   // NB10
  char Offset[4];     // zeroes
  char Signature[4];  // zeroes
  char Age[4];        // zeroes
  char PdbFileName[]; // filename
} CV_INFO_PDB20;

This is from GNU Binutils commit 61e2488cd849 ("Add support for
generating and inserting build IDs into COFF binaries.", 2014-04-08).

What I don't understand is why "objdump -p" does not decode the debug
directory for me. :/

Thanks
Laszlo