public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
@ 2017-07-03  5:21 Liming Gao
  2017-07-03  5:28 ` Zhu, Yonghong
  2017-07-05 12:42 ` Laszlo Ersek
  0 siblings, 2 replies; 6+ messages in thread
From: Liming Gao @ 2017-07-03  5:21 UTC (permalink / raw)
  To: edk2-devel

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);
+        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;
+        }
+      }
     }
   }
 
-- 
2.8.0.windows.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
  2017-07-03  5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
@ 2017-07-03  5:28 ` Zhu, Yonghong
  2017-07-05 12:42 ` Laszlo Ersek
  1 sibling, 0 replies; 6+ messages in thread
From: Zhu, Yonghong @ 2017-07-03  5:28 UTC (permalink / raw)
  To: Gao, Liming, edk2-devel@lists.01.org

Reviewed-by: Yonghong Zhu <yonghong.zhu@intel.com> 

Best Regards,
Zhu Yonghong


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Liming Gao
Sent: Monday, July 03, 2017 1:21 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain

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);
+        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;
+        }
+      }
     }
   }
 
--
2.8.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
  2017-07-03  5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
  2017-07-03  5:28 ` Zhu, Yonghong
@ 2017-07-05 12:42 ` Laszlo Ersek
  2017-07-05 13:29   ` Laszlo Ersek
  2017-07-05 13:41   ` Leif Lindholm
  1 sibling, 2 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 12:42 UTC (permalink / raw)
  To: Liming Gao, edk2-devel; +Cc: Gerd Hoffmann, Ard Biesheuvel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
  2017-07-05 12:42 ` Laszlo Ersek
@ 2017-07-05 13:29   ` Laszlo Ersek
  2017-07-05 15:29     ` Laszlo Ersek
  2017-07-05 13:41   ` Leif Lindholm
  1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 13:29 UTC (permalink / raw)
  To: Liming Gao, edk2-devel; +Cc: Ard Biesheuvel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
  2017-07-05 12:42 ` Laszlo Ersek
  2017-07-05 13:29   ` Laszlo Ersek
@ 2017-07-05 13:41   ` Leif Lindholm
  1 sibling, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2017-07-05 13:41 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: Liming Gao, edk2-devel, Ard Biesheuvel, Kinney, Michael D,
	Andrew Fish, Yonghong Zhu

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
  2017-07-05 13:29   ` Laszlo Ersek
@ 2017-07-05 15:29     ` Laszlo Ersek
  0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 15:29 UTC (permalink / raw)
  To: Liming Gao, edk2-devel
  Cc: Ard Biesheuvel, Leif Lindholm (Linaro address), Zhu, Yonghong

I understand it now. The article at
<http://www.debuginfo.com/articles/debuginfomatch.html> was of great help.

On 07/05/17 15:29, Laszlo Ersek wrote:
> On 07/05/17 14:42, Laszlo Ersek wrote:
>> On 07/03/17 07:21, Liming Gao wrote:

>>> @@ -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}

Notice this. The first entry (which is valid, with type =
EFI_IMAGE_DEBUG_TYPE_CODEVIEW), points at offset 33292, and the
pointed-to data (which is a CodeView information block) has size 209.
This means that the first byte *past* the CodeView information is at
offset 33292 + 209 = 33501.

Now, look at the data dictionary again:

>>> (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

Notice this: 33264 + 237 = 33501. The same end offset!

And then: 237 - 209 = 28, which is exactly the size of
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.

So, what happens is that the GNU linker (or some other bin-util that
produces the input file for GenFw)

(1) creates a valid EFI_IMAGE_DEBUG_DIRECTORY_ENTRY, of type
EFI_IMAGE_DEBUG_TYPE_CODEVIEW, with size = 28,

(2) this element points to a valid CV_INFO_PDB20 structure (having NB10
for signature), with size = 209,

(3) The CV_INFO_PDB20 structure *immediately* follows the
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element that points to it -- this is all
fine,

(4) *however*, the pointed-to CV_INFO_PDB20 element (from (3)) is *also*
included in the size of the debug directory, even though the size of the
debug directory should *only* describe the debug directory entry from (1)!

So, this is most certainly a GNU Binutils bug.

We can catch it though: as soon as we find an
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY whose FileOffset field points into the
debug directory itself, we know that the debug directory's size has to
be truncated at once to that offset.

Let me see if I can write a patch for this.

Thanks
Laszlo



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-07-05 15:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03  5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
2017-07-03  5:28 ` Zhu, Yonghong
2017-07-05 12:42 ` Laszlo Ersek
2017-07-05 13:29   ` Laszlo Ersek
2017-07-05 15:29     ` Laszlo Ersek
2017-07-05 13:41   ` Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox