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 48E9821D0A272 for ; Sun, 13 Aug 2017 19:10:57 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Aug 2017 19:13:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,371,1498546800"; d="scan'208,217";a="299703838" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 13 Aug 2017 19:13:20 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 13 Aug 2017 19:13:19 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 13 Aug 2017 19:13:19 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.183]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.236]) with mapi id 14.03.0319.002; Mon, 14 Aug 2017 10:13:17 +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//+A24CAAI9MwIAANQmAgARForA= Date: Mon, 14 Aug 2017 02:13:16 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D76FF91@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> <4A89E2EF3DFEDB4C8BFDE51014F606A14D76E739@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Mon, 14 Aug 2017 02:10:57 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew: I have no more detail than the comment. This logic is here since the firs= t version edk2 project. Per the comment, it should handle the one special P= E image that was generated in the old compiler. For TE image, its image siz= e is calculated by the RVA plus the VirtualSize of the last section mapped = into memory. So, its image size should include the debug info. This is just= my analysis. If there is such PE image, I can further verify this logic. Thanks Liming From: afish@apple.com [mailto:afish@apple.com] Sent: Saturday, August 12, 2017 12:51 AM To: Gao, Liming Cc: Kinney, Michael D ; edk2-devel@lists.01.org Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when buil= d AppPkg with XCODE5 Liming, Your right I'm off by one. The signature is in the thing EFI_IMAGE_DEBUG_DI= RECTORY_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 =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; } https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/= BasePeCoff.c#L862 if (DebugEntry.Type =3D=3D EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { ImageContext->DebugDirectoryEntryRva =3D (UINT32) (DebugDirectory= EntryRva + 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 r= eview 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_T= YPE_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, it= s DEBUG_CODEVIEW_ ENTRY Signature is CODEVIEW_SIGNATURE_MTOC. So, it will b= e handled in this logic. for (Index =3D 0; Index < DebugDirectoryEntry->Size; Index +=3D size= of (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 debug= info will not load into memory. // Here we will always load EFI_IMAGE_DEBUG_TYPE_CODEVIEW type deb= ug 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) (DebugDirector= yEntryRva + 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 buil= d 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 upd= ated 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 oth= er 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. Op= tion 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 buil= d 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 th= e generated EFI image. If so, the EFI image is always correct. There is no ch= ange 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 suctio= n 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 >>> 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 >>> Cc: Michael D Kinney >>> 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 secti= on, 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