Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
>
> This change is to support the pre-built EFI image on the old Edk2 code.
> Old Edk2 GenFw tool generates the wrong debug entry in EFI image.
> Those pre-built images can be loaded before d6457b309.
>
> Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 4b71176a0c..4636842eeb 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo (
>                                       &Size,
>                                       &DebugEntry
>                                       );
> -          if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> +          if ((ImageContext->DebugDirectoryEntryRva == 0) && (RETURN_ERROR (Status) || (Size != ReadSize))) {

I'm not sure I understand how this fixes anything. Why is the
condition 'RVA == 0' significant here?

Would something like the below perhaps work as well?

--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo (
               ImageContext->ImageSize += DebugEntry.SizeOfData;
             }

+            //
+            // Implementations of GenFw before commit 60e85a39fe49071 will
+            // concatenate the debug directory entry and the codeview entry,
+            // and erroneously put the combined size into the debug directory
+            // entry's size field. If this is the case, no other relevant
+            // directory entries can exist, and we can terminate here.
+            //
+            if (DebugEntry.FileOffset ==
(DebugDirectoryEntryFileOffset + Index)) {
+              break;
+            }
+
             continue;
           }