From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 B59D221D490EC for ; Thu, 10 Aug 2017 22:47:32 -0700 (PDT) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Aug 2017 22:49:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,356,1498546800"; d="scan'208";a="117895171" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 10 Aug 2017 22:49:52 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 10 Aug 2017 22:49:51 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 10 Aug 2017 22:49:51 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.183]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.25]) with mapi id 14.03.0319.002; Fri, 11 Aug 2017 13:49:49 +0800 From: "Gao, Liming" To: "afish@apple.com" CC: "Kinney, Michael D" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build AppPkg with XCODE5 Thread-Index: AQHTDzG9nrjWB46WkkigpNBVS7AKAqJ4j1OAgATSxLD//+1hAIABSr0Q//+A24CAAI9MwA== Date: Fri, 11 Aug 2017 05:49:48 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D76E739@shsmsx102.ccr.corp.intel.com> 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> In-Reply-To: <6B474862-80FC-4D7B-B732-D10F791182F5@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 05:47:33 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 i= s 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 mto= c debug entry, its DebugEntry.Type is also EFI_IMAGE_DEBUG_TYPE_CODEVIEW, i= ts DEBUG_CODEVIEW_ ENTRY Signature is CODEVIEW_SIGNATURE_MTOC. So, it will = be handled in this logic.=20 for (Index =3D 0; Index < DebugDirectoryEntry->Size; Index +=3D siz= eof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY)) { // // Read next debug directory entry // Size =3D sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); ... // // From PeCoff spec, when DebugEntry.RVA =3D=3D 0 means this debu= g info will not load into memory. // Here we will always load EFI_IMAGE_DEBUG_TYPE_CODEVIEW type de= bug info. so need adjust the // ImageContext->ImageSize when DebugEntry.RVA =3D=3D 0. // if (DebugEntry.Type =3D=3D EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { ImageContext->DebugDirectoryEntryRva =3D (UINT32) (DebugDirecto= ryEntryRva + Index); if (DebugEntry.RVA =3D=3D 0 && DebugEntry.FileOffset !=3D 0) { ImageContext->ImageSize +=3D 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 bui= ld >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. Eve= n if >this image is not built into BIOS image, its EFI image will be generated b= y >GenFw. So, only if this EFI image is built from EDK2 project, it can be up= dated >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 ot= her >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. O= ption >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 b= uild >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 c= hange >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 suc= tion >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 =3D=3D CODEVIEW_SIGNATURE_MTOC) { >> + return RETURN_SUCCESS; >> } >> } >> } >> @@ -862,6 +864,8 @@ PeCoffLoaderGetImageInfo ( >> if (DebugEntry.Type =3D=3D EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { >> ImageContext->DebugDirectoryEntryRva =3D (UINT32) >(DebugDirectoryEntryRva + Index); >> return RETURN_SUCCESS; >> + } else if (DebugEntry.Type =3D=3D CODEVIEW_SIGNATURE_MTOC) { >> + return RETURN_SUCCESS; >> } >> } >> } >> >> >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D663 >> 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 =3D (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + >sizeof (EFI_IMAGE_FILE_HEADER)); >> + Optional64Hdr =3D (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr >+ sizeof (EFI_IMAGE_FILE_HEADER)); >> if (Optional32Hdr->Magic =3D=3D EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { >> SectionHeader =3D (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) >Optional32Hdr + FileHdr->SizeOfOptionalHeader); >> if (Optional32Hdr->NumberOfRvaAndSizes > >EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \ >> Optional32Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size !=3D 0) { >> ExportDirectoryEntryRva =3D Optional32Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress; >> @@ -2833,11 +2834,10 @@ Returns: >> Optional32Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size =3D 0; >> Optional32Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress =3D 0; >> } >> } >> } else { >> - Optional64Hdr =3D (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr >+ sizeof (EFI_IMAGE_FILE_HEADER)); >> SectionHeader =3D (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) >Optional64Hdr + FileHdr->SizeOfOptionalHeader); >> if (Optional64Hdr->NumberOfRvaAndSizes > >EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \ >> Optional64Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size !=3D 0) { >> ExportDirectoryEntryRva =3D Optional64Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress; >> } >> @@ -2907,10 +2907,20 @@ Returns: >> RsdsEntry->Unknown =3D 0; >> RsdsEntry->Unknown2 =3D 0; >> RsdsEntry->Unknown3 =3D 0; >> RsdsEntry->Unknown4 =3D 0; >> RsdsEntry->Unknown5 =3D 0; >> + } else if (RsdsEntry->Signature =3D=3D CODEVIEW_SIGNATURE_MTOC)= { >> + // MTOC sets DebugDirectoryEntrySize to size of the .debug se= ction, >so fix it. >> + if (!ZeroDebugFlag) { >> + if (Optional32Hdr->Magic =3D=3D >EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { >> + Optional32Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size =3D sizeof >(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); >> + } else { >> + Optional64Hdr- >>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size =3D 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