From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 424802095A6C9 for ; Wed, 5 Jul 2017 06:27:23 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2ECD07F3F1; Wed, 5 Jul 2017 13:29:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2ECD07F3F1 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2ECD07F3F1 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-25.phx2.redhat.com [10.3.116.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 30DA060A99; Wed, 5 Jul 2017 13:29:00 +0000 (UTC) From: Laszlo Ersek To: Liming Gao , edk2-devel@lists.01.org Cc: Ard Biesheuvel References: <1499059281-11744-1-git-send-email-liming.gao@intel.com> Message-ID: <751fb308-e7b2-c15c-42c0-cade9ffe98bd@redhat.com> Date: Wed, 5 Jul 2017 15:29:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 05 Jul 2017 13:29:02 +0000 (UTC) Subject: Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Jul 2017 13:27:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >> --- >> 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 , > 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