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

[Laszlo Ersek <lersek@redhat.com>]

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