Re: [Patch] BaseTools: Fix Segmentation fault: 11 when build AppPkg with XCODE5

["Gao, Liming" <liming.gao@intel.com>]

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 <liming.gao@intel.com>
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 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 <liming.gao@intel.com> 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 <liming.gao@intel.com>
>> Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; 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
><liming.gao@intel.com<mailto:liming.gao@intel.com>> 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> [mailto:afish@apple.com]
>> Sent: Tuesday, August 8, 2017 12:26 AM
>> To: Zhu, Yonghong
><yonghong.zhu@intel.com<mailto:yonghong.zhu@intel.com>>
>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming
><liming.gao@intel.com<mailto:liming.gao@intel.com>>; Kinney, Michael D
><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>> 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
><yonghong.zhu@intel.com<mailto:yonghong.zhu@intel.com><mailto:yongho
>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
><liming.gao@intel.com<mailto:liming.gao@intel.com><mailto:liming.gao@int
>el.com>>
>> Cc: Michael D Kinney
><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com><mailto:m
>ichael.d.kinney@intel.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Andrew Fish
><afish@apple.com<mailto:afish@apple.com><mailto:afish@apple.com>>
>> ---
>> 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<mailto: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