public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
@ 2023-04-27  6:17 gaoliming
  2023-04-27 14:47 ` [edk2-devel] " Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: gaoliming @ 2023-04-27  6:17 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Michael Kubacki

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))) {
             ImageContext->ImageError = IMAGE_ERROR_IMAGE_READ;
             if (Size != ReadSize) {
               Status = RETURN_UNSUPPORTED;
-- 
2.37.3.windows.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
  2023-04-27  6:17 [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found gaoliming
@ 2023-04-27 14:47 ` Ard Biesheuvel
  2023-04-28  6:02   ` 回复: " gaoliming
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-04-27 14:47 UTC (permalink / raw)
  To: devel, gaoliming; +Cc: Michael Kubacki

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;
           }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 回复: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
  2023-04-27 14:47 ` [edk2-devel] " Ard Biesheuvel
@ 2023-04-28  6:02   ` gaoliming
  2023-04-28  9:20     ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: gaoliming @ 2023-04-28  6:02 UTC (permalink / raw)
  To: devel, ardb; +Cc: 'Michael Kubacki'

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?

Thanks
Liming
>              continue;
>            }
> 
> 
> 
> 




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
  2023-04-28  6:02   ` 回复: " gaoliming
@ 2023-04-28  9:20     ` Ard Biesheuvel
  2023-05-04  7:49       ` 回复: " gaoliming
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-04-28  9:20 UTC (permalink / raw)
  To: devel, gaoliming; +Cc: Michael Kubacki

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 回复: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
  2023-04-28  9:20     ` Ard Biesheuvel
@ 2023-05-04  7:49       ` gaoliming
  2023-05-04 14:15         ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: gaoliming @ 2023-05-04  7:49 UTC (permalink / raw)
  To: devel, ardb; +Cc: 'Michael Kubacki'

Ard:

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> Biesheuvel
> 发送时间: 2023年4月28日 17:21
> 收件人: 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 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.
> 
> 
I verified this change. It doesn't work, because 
DebugEntry.FileOffset value is DebugDirectoryEntryFileOffset + Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY)

I review the change 60e85a39fe49071 in GenFw. It places DebugEntry as the last debug directory. 
So, I think the simple fix is to directly break once DebugEntry is found. 

Thanks
Liming
> 
> 




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found
  2023-05-04  7:49       ` 回复: " gaoliming
@ 2023-05-04 14:15         ` Ard Biesheuvel
  0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2023-05-04 14:15 UTC (permalink / raw)
  To: devel, gaoliming; +Cc: Michael Kubacki

On Thu, 4 May 2023 at 09:49, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> Ard:
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> > Biesheuvel
> > 发送时间: 2023年4月28日 17:21
> > 收件人: 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 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.
> >
> >
> I verified this change. It doesn't work, because
> DebugEntry.FileOffset value is DebugDirectoryEntryFileOffset + Index + sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY)
>
> I review the change 60e85a39fe49071 in GenFw. It places DebugEntry as the last debug directory.
> So, I think the simple fix is to directly break once DebugEntry is found.
>

I'm confused. Doesn't that mean we will ignore the
DEBUG_TYPE_EX_DLLCHARACTERISTICS entry?

Given that the old, broken GenFw puts the NB10 directory entry and the
NB10 data structure contiguous in memory, we should be able to detect
that, right? And in this case, because the NB10 directory entry is
followed by something we /know/ is not another directory entry, we can
exit the loop early.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-05-04 14:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-27  6:17 [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found gaoliming
2023-04-27 14:47 ` [edk2-devel] " Ard Biesheuvel
2023-04-28  6:02   ` 回复: " gaoliming
2023-04-28  9:20     ` Ard Biesheuvel
2023-05-04  7:49       ` 回复: " gaoliming
2023-05-04 14:15         ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox