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

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

On Fri, 28 Apr 2023 at 07:02, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> Ard:
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> > Biesheuvel
> > 发送时间: 2023年4月27日 22:47
> > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> > 抄送: Michael Kubacki <michael.kubacki@microsoft.com>
> > 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug
> > entry read error after it is found
> >
> > 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?
> >
>
> DebugDirectoryEntryRva will set to the real value if DEBUG_TYPE_CODEVIEW is found.
> If its value is not zero, it means DebugEntry has been found. This is not an error, and don't need to report error here.
>
> > --- 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;
> > +            }
> > +
> d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, there may be two DEBUG entries in one EFI image.
> One is DEBUG_TYPE_CODEVIEW, another is DEBUG_TYPE_EX_DLLCHARACTERISTICS.
>
> Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance to search DEBUG_TYPE_EX_DLLCHARACTERISTICS?
>

No. But if the codeview payload follows the codeview entry directly,
there cannot be any other entries, so there is no reason to proceed.