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

[Leif Lindholm <leif.lindholm@linaro.org>]

Many thanks for tracking this down Laszlo - we'd stumbled over it
ourselves this morning.

Liming, Mike, Andrew: this currently makes the BaseTools unusable with
gcc toolchains. Can we revert this commit until this has been
resolved?

Best Regards,

Leif

On Wed, Jul 05, 2017 at 02:42:15PM +0200, 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.
> 
> > +        memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> > +      }
> > +      if (DebugEntry->Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> > +        RsdsEntry = (EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *) (FileBuffer + DebugEntry->FileOffset);
> > +        if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_RSDS) {
> > +          RsdsEntry->Unknown  = 0;
> > +          RsdsEntry->Unknown2 = 0;
> > +          RsdsEntry->Unknown3 = 0;
> > +          RsdsEntry->Unknown4 = 0;
> > +          RsdsEntry->Unknown5 = 0;
> > +        }
> > +      }
> >      }
> >    }
> >
> >
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel