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

[Andrew Fish <afish@apple.com>]

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 <liming.gao@intel.com> 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 <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
>