From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in2.apple.com (mail-out2.apple.com [17.151.62.25]) (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 7D10E2095DFED for ; Fri, 11 Aug 2017 09:48:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1502470238; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=aE8nX01iKxwobzcqf+tH90pmymBzd/P/CCfR4QIUaPw=; b=GbXbo8q+U77uH4bCHfGoo8jNu7gVu4ur5vGcsgJIHp0UTog0Z76rF0qBIvlv7mpB pcQjesoSimnwRJOh6K9bTfIsgjNSwYA5eeLlRZiK37rHsSYUrqUM9WzWxvF3uMbf y/cff6M0vVuQK9YtLVOsuLfHrtE8scC4iKaaSkdeQWkcGG4U+ZguBp2U+cfwS7vO Mbehvk+bYBtgz9d7h6p2TaHlJahRsC+NzYLJZNf9Z5foUpUlT8Oa6hIG9+yXLfsN JRD8dikFaB9+Icl8gAMakYh5RdhmS/kQD7V1CfbgDO9HZk4PAmvYESjU4yyq4qOQ +y2NbDhYJRAxLvBtwy06Dg==; Received: from relay25.apple.com (relay25.apple.com [17.171.128.106]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in2.apple.com (Apple Secure Mail Relay) with SMTP id 52.65.07214.E50ED895; Fri, 11 Aug 2017 09:50:38 -0700 (PDT) X-AuditID: 11973e11-30dff70000001c2e-00-598de05d0cae Received: from ma1-mmpp-sz09.apple.com (ma1-mmpp-sz09.apple.com [17.171.128.183]) by relay25.apple.com (Apple SCV relay) with SMTP id CE.53.08442.D50ED895; Fri, 11 Aug 2017 09:50:37 -0700 (PDT) MIME-version: 1.0 Received: from [17.234.211.28] by ma1-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.1.2.20170621 64bit (built Jun 21 2017)) with ESMTPSA id <0OUJ0091J5G9TG70@ma1-mmpp-sz09.apple.com>; Fri, 11 Aug 2017 09:50:37 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish Message-id: Date: Fri, 11 Aug 2017 09:50:33 -0700 In-reply-to: <4A89E2EF3DFEDB4C8BFDE51014F606A14D76E739@shsmsx102.ccr.corp.intel.com> Cc: Mike Kinney , "edk2-devel@lists.01.org" To: "Gao, Liming" References: <1502078429-13340-1-git-send-email-yonghong.zhu@intel.com> <5BC1C303-CE42-4DAD-91EB-F4BB327DE88A@apple.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D76DAD8@shsmsx102.ccr.corp.intel.com> <2AC68977-AB40-45DC-B97D-27FFCE19C155@apple.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D76E5FF@shsmsx102.ccr.corp.intel.com> <6B474862-80FC-4D7B-B732-D10F791182F5@apple.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D76E739@shsmsx102.ccr.corp.intel.com> X-Mailer: Apple Mail (2.3273) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrNLMWRmVeSWpSXmKPExsUiuLohSzfuQW+kwfRF5hZ7Dh1ltlhxbwO7 RUfHPyYHZo/Fe14yeXTP/scSwBTFZZOSmpNZllqkb5fAlXHsxyu2gmuTmSuON0xhbWA8c5+p i5GTQ0LARGLlyjdANheHkMA6JolnN26xwSTWdU9hhUgcZpT4euo2I0iCV0BQ4sfkeywgNrNA mMSLS6dZIIq+Mkqs29sDlhAWEJd4d2YTM4jNJqAssWL+B/YuRg6gZhuJk7s0IEoSJT7P/Ax2 BYuAqsSBP/PB5nMCzZy2eTYjxPxkiWmLTrCD2CICGhIP7/1mhti1l1niwvY37BCXykrcmn0J LCEhsIJN4sSRl0wTGIVmITl2FpJjIWwtie+PWoFsDiBbXuLgeVmIsKbEs3uf2CFsbYkn7y6w LmBkW8UolJuYmaObmWekl1hQkJOql5yfu4kRFBvT7QR3MB5fZXWIUYCDUYmHt+Jsb6QQa2JZ cWXuIUZpDhYlcd6XD3sihQTSE0tSs1NTC1KL4otKc1KLDzEycXBKNTAmdyU8mDEjM5X9VYj9 0X691N0v9h+4nfj1jPFx06KbV68ICChmTa1os7mkPVte/83NJRIPXXKyFnYYHuM+yXGbj2sr 98UV6d+2XZnOdNn/z6Q030WxNzv+vnkQ8FvFeXoq25F0JaayuQsWROk9C7GacvyRQrjzApWt MmtKo3s9r68QMz8mP01PiaU4I9FQi7moOBEAkf4xb24CAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrPLMWRmVeSWpSXmKPExsUiuLphu27sg95Ig+WbdS32HDrKbLHi3gZ2 i46Of0wOzB6L97xk8uie/Y8lgCmKyyYlNSezLLVI3y6BK+PYj1dsBdcmM1ccb5jC2sB45j5T FyMnh4SAicS67imsXYxcHEIChxklvp66zQiS4BUQlPgx+R4LiM0sECbx4tJpFoiir4wS6/b2 gCWEBcQl3p3ZxAxiswkoS6yY/4G9i5EDqNlG4uQuDYiSRInPMz+DLWMRUJU48Gc+2HxOoJnT Ns9mhJifLDFt0Ql2EFtEQEPi4b3fzBC79jJLXNj+hh3iUlmJW7MvMU9g5J+F5L5ZSO6DsLUk vj9qBbI5gGx5iYPnZSHCmhLP7n1ih7C1JZ68u8C6gJFtFaNgUWpOYqWRqV5iQUFOql5yfu4m RkgwZ+1gvH3T7BCjAAejEg+vwuneSCHWxLLiytxDjBIczEoivM/vAYV4UxIrq1KL8uOLSnNS iw8xSnOwKInzfu/ojhQSSE8sSc1OTS1ILYLJMnFwSjUwOgTEmHKlaTyewKnzLevXZ472A7zF zwUUudR3388I7+t71+kUe4uH7cLGW/NE+Wd/5pizZ54ye+r27++eF6jdkr6+1NOiIXzBF0/T 945f9aJY/uaKGKz0qv33uohzwwuZwKvf8xJVnvMVnV3Xyvfpku6hy0vZZrCHcG9W9b4ZWvac QeRR6SkdJZbijERDLeai4kQAn2jK9WICAAA= X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [Patch] BaseTools: Fix Segmentation fault: 11 when build AppPkg with XCODE5 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: Fri, 11 Aug 2017 16:48:18 -0000 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT Liming, Your right I'm off by one. The signature is in the thing EFI_IMAGE_DEBUG_DIRECTORY_ENTRY points to. I do have a question. Why does the PE/COFF code have an extra step. The TE images are converted PE/COFF images so would not you need this code in both places? Is there some other assumption in play here? https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L769 // // From PeCoff spec, when DebugEntry.RVA == 0 means this debug info will not load into memory. // Here we will always load EFI_IMAGE_DEBUG_TYPE_CODEVIEW type debug info. so need adjust the // ImageContext->ImageSize when DebugEntry.RVA == 0. // if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { ImageContext->DebugDirectoryEntryRva = (UINT32) (DebugDirectoryEntryRva + Index); if (DebugEntry.RVA == 0 && DebugEntry.FileOffset != 0) { ImageContext->ImageSize += DebugEntry.SizeOfData; } return RETURN_SUCCESS; } https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L862 if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { ImageContext->DebugDirectoryEntryRva = (UINT32) (DebugDirectoryEntryRva + Index); return RETURN_SUCCESS; } Thanks, Andrew Fish > On Aug 10, 2017, at 10:49 PM, Gao, Liming wrote: > > Andrew: > I get your point. I agree edk2 firmware to support old EFI image. And, I review the change in BasePeCoffLib. But, I think it is unnecessary. Below is the current logic. On the first loop, DebugEntry.Type is EFI_IMAGE_DEBUG_TYPE_CODEVIEW, then will return. It doesn't enter into second loop. For mtoc debug entry, its DebugEntry.Type is also EFI_IMAGE_DEBUG_TYPE_CODEVIEW, its DEBUG_CODEVIEW_ ENTRY Signature is CODEVIEW_SIGNATURE_MTOC. So, it will be handled in this logic. > > for (Index = 0; Index < DebugDirectoryEntry->Size; Index += sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY)) { > // > // Read next debug directory entry > // > Size = sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); > ... > > // > // From PeCoff spec, when DebugEntry.RVA == 0 means this debug info will not load into memory. > // Here we will always load EFI_IMAGE_DEBUG_TYPE_CODEVIEW type debug info. so need adjust the > // ImageContext->ImageSize when DebugEntry.RVA == 0. > // > if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { > ImageContext->DebugDirectoryEntryRva = (UINT32) (DebugDirectoryEntryRva + Index); > if (DebugEntry.RVA == 0 && DebugEntry.FileOffset != 0) { > ImageContext->ImageSize += DebugEntry.SizeOfData; > } > > return RETURN_SUCCESS; > } > } > > Thanks > Liming >> -----Original Message----- >> From: afish@apple.com [mailto:afish@apple.com] >> Sent: Friday, August 11, 2017 1:08 PM >> To: Gao, Liming >> Cc: Kinney, Michael D ; edk2- >> devel@lists.01.org >> Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build >> AppPkg with XCODE5 >> >> >>> On Aug 10, 2017, at 9:48 PM, Gao, Liming wrote: >>> >>> Andrew: >>> Edk2 Build system calls GenFw to generate EFI image in build phase. Even if >> this image is not built into BIOS image, its EFI image will be generated by >> GenFw. So, only if this EFI image is built from EDK2 project, it can be updated >> by GenFw tool. You can see this step in build_rule.txt to convert .dll to .efi >> image. >>> >> >> Gao, >> >> We can fix the future for edk2, but we can't change the past, or change other >> build systems. It is not safe to assume that every EFI executable that the DXE >> Core will see is as new as the firmware, or was even built via the edk2. Option >> ROMs and OS loaders are good examples of this issue. >> >> Thanks, >> >> Andrew Fish >> >>> Thanks >>> Liming >>> From: afish@apple.com [mailto:afish@apple.com] >>> Sent: Friday, August 11, 2017 12:59 AM >>> To: Gao, Liming >>> Cc: Zhu, Yonghong ; Kinney, Michael D >> ; edk2-devel@lists.01.org >>> Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build >> AppPkg with XCODE5 >>> >>> >>> On Aug 10, 2017, at 3:38 AM, Gao, Liming >> > wrote: >>> >>> Andrew: >>> If this is a mtoc bug, I suggest to update GenFw to always correct it in the >> generated EFI image. If so, the EFI image is always correct. There is no change >> requirement in PeCoff library in MdePkg. >>> >>> >>> Liming, >>> >>> EFI supports loading PE/COFF images that are not built at the same time as >> the platform firmware (UEFI Shell, OS loader), and that is why I added the fix >> to the PE/COFF library code. >>> >>> Thanks, >>> >>> Andrew Fish >>> >>> >>> Thanks >>> Liming >>> From: afish@apple.com [mailto:afish@apple.com] >>> Sent: Tuesday, August 8, 2017 12:26 AM >>> To: Zhu, Yonghong >> > >>> Cc: edk2-devel@lists.01.org; Gao, Liming >> >; Kinney, Michael D >> > >>> Subject: Re: [Patch] BaseTools: Fix Segmentation fault: 11 when build >> AppPkg with XCODE5 >>> >>> Should that be: >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> >>> I also noticed the PeCoff lib is going to loop and reload the .debug suction >> due to this mtoc bug, so it would be good to harden that code too. >>> >>> git diff MdePkg/Library/BasePeCoffLib/BasePeCoff.c >>> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >>> index 8d1daba..1e4c67e 100644 >>> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >>> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >>> @@ -771,6 +771,8 @@ PeCoffLoaderGetImageInfo ( >>> } >>> >>> return RETURN_SUCCESS; >>> + } else if (DebugEntry.Type == CODEVIEW_SIGNATURE_MTOC) { >>> + return RETURN_SUCCESS; >>> } >>> } >>> } >>> @@ -862,6 +864,8 @@ PeCoffLoaderGetImageInfo ( >>> if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { >>> ImageContext->DebugDirectoryEntryRva = (UINT32) >> (DebugDirectoryEntryRva + Index); >>> return RETURN_SUCCESS; >>> + } else if (DebugEntry.Type == CODEVIEW_SIGNATURE_MTOC) { >>> + return RETURN_SUCCESS; >>> } >>> } >>> } >>> >>> >>> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=663 >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> >>> Thanks, >>> >>> Andrew Fish >>> >>> >>> On Aug 6, 2017, at 9:00 PM, Yonghong Zhu >> > ng.zhu@intel.com>> wrote: >>> >>> it is a bug in mtoc setting the size of the debug directory entry to >>> the size of the .debug section, not the size of the >>> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY. It was causing a loop to iterate and >>> get bogus EFI_IMAGE_DEBUG_DIRECTORY_ENTRY data and pass that to >>> memset() and boom. >>> >>> Cc: Liming Gao >> > el.com>> >>> Cc: Michael D Kinney >> > ichael.d.kinney@intel.com>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Andrew Fish >> > >>> --- >>> BaseTools/Source/C/GenFw/GenFw.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/BaseTools/Source/C/GenFw/GenFw.c >> b/BaseTools/Source/C/GenFw/GenFw.c >>> index 246deb0..af60c92 100644 >>> --- a/BaseTools/Source/C/GenFw/GenFw.c >>> +++ b/BaseTools/Source/C/GenFw/GenFw.c >>> @@ -2813,10 +2813,11 @@ Returns: >>> // >>> // Get Debug, Export and Resource EntryTable RVA address. >>> // Resource Directory entry need to review. >>> // >>> Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + >> sizeof (EFI_IMAGE_FILE_HEADER)); >>> + Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr >> + sizeof (EFI_IMAGE_FILE_HEADER)); >>> if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { >>> SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) >> Optional32Hdr + FileHdr->SizeOfOptionalHeader); >>> if (Optional32Hdr->NumberOfRvaAndSizes > >> EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \ >>> Optional32Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size != 0) { >>> ExportDirectoryEntryRva = Optional32Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress; >>> @@ -2833,11 +2834,10 @@ Returns: >>> Optional32Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0; >>> Optional32Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0; >>> } >>> } >>> } else { >>> - Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr >> + sizeof (EFI_IMAGE_FILE_HEADER)); >>> SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) >> Optional64Hdr + FileHdr->SizeOfOptionalHeader); >>> if (Optional64Hdr->NumberOfRvaAndSizes > >> EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \ >>> Optional64Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size != 0) { >>> ExportDirectoryEntryRva = Optional64Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress; >>> } >>> @@ -2907,10 +2907,20 @@ Returns: >>> RsdsEntry->Unknown = 0; >>> RsdsEntry->Unknown2 = 0; >>> RsdsEntry->Unknown3 = 0; >>> RsdsEntry->Unknown4 = 0; >>> RsdsEntry->Unknown5 = 0; >>> + } else if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_MTOC) { >>> + // MTOC sets DebugDirectoryEntrySize to size of the .debug section, >> so fix it. >>> + if (!ZeroDebugFlag) { >>> + if (Optional32Hdr->Magic == >> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { >>> + Optional32Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = sizeof >> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); >>> + } else { >>> + Optional64Hdr- >>> DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = sizeof >> (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); >>> + } >>> + } >>> + break; >>> } >>> } >>> } >>> } >>> >>> -- >>> 2.6.1.windows.1 >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >