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 7F2C521A07AB5 for ; Wed, 5 Jul 2017 05:40:41 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4561E4E4D1; Wed, 5 Jul 2017 12:42:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4561E4E4D1 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4561E4E4D1 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 9173D77670; Wed, 5 Jul 2017 12:42:16 +0000 (UTC) To: Liming Gao , edk2-devel@lists.01.org References: <1499059281-11744-1-git-send-email-liming.gao@intel.com> Cc: Gerd Hoffmann , Ard Biesheuvel From: Laszlo Ersek Message-ID: Date: Wed, 5 Jul 2017 14:42:15 +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: <1499059281-11744-1-git-send-email-liming.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 05 Jul 2017 12:42:20 +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 12:40:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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. > + 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