diff mbox series

bfd: fill in PE load config directory entry.

Message ID 3ee165ae-9a6c-7c39-c050-421823572692@jdrake.com
State New
Headers
Series bfd: fill in PE load config directory entry. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jeremy Drake March 9, 2025, 10:32 p.m. UTC 
  This is filled in with the rva of _load_config_used if defined (much
like _tls_used), and the size is the first 32-bit value at that symbol.

Signed-off-by: Jeremy Drake <sourceware-bugzilla@jdrake.com>
---
This is a pretty straightforward patch, so I thought it'd be a good place
to start figuring out the proper process to contribute patches to
binutils.  After this, I have a more complicated patch series to address
PR 32675 and PR 14339 as a side-effect.  That is probably the more
pressing thing...


 bfd/peXXigen.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

Comments

LIU Hao March 10, 2025, 2:41 a.m. UTC | #1 
在 2025-3-10 06:32, Jeremy Drake 写道:
> +      /* the size is stored as the first 4 bytes at _load_config_used */
> +      if (bfd_get_section_contents (abfd, h1->root.u.def.section->output_section,
> +			      sz, h1->root.u.def.section->output_offset, 4))
> +	pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].Size =
> +	  bfd_get_32 (abfd, sz);

There seems to be an issue in this code. You can try compiling this library:

  
https://github.com/lhmouse/mcfgthread/blob/a8d07be576f4089e383cc725b5cd1e83f9e6ab0e/mcfgthread/xglobals.c#L355

The load config directory was added for Clang-CL. It's always there even GNU LD doesn't use it.


With this patch I got:

    UCRT64 ~/GitHub/mcfgthread/build_release
    $ objdump -pw libmcfgthread-minimal-2.dll | grep -F 'Load Configuration Directory'
    Entry a 0000000000005120 0045004b Load Configuration Directory


The size field doesn't look quite right. It should be 0x140:

    UCRT64 ~/GitHub/mcfgthread/build_clang-cl
    $ objdump -pw libmcfgthread-minimal-2.dll | grep -F 'Load Configuration Directory'
    Entry a 0000000000008108 00000140 Load Configuration Directory




-- 
Best regards,
LIU Hao
Jeremy Drake March 10, 2025, 5:51 a.m. UTC | #2 
On Mon, 10 Mar 2025, LIU Hao wrote:

> 在 2025-3-10 06:32, Jeremy Drake 写道:
> > +      /* the size is stored as the first 4 bytes at _load_config_used */
> > +      if (bfd_get_section_contents (abfd,
> > h1->root.u.def.section->output_section,
> > +			      sz, h1->root.u.def.section->output_offset, 4))
> > +	pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].Size =
> > +	  bfd_get_32 (abfd, sz);
>
> There seems to be an issue in this code. You can try compiling this library:
>
>  https://github.com/lhmouse/mcfgthread/blob/a8d07be576f4089e383cc725b5cd1e83f9e6ab0e/mcfgthread/xglobals.c#L355
>
> The load config directory was added for Clang-CL. It's always there even GNU
> LD doesn't use it.
>
>
> With this patch I got:
>
>    UCRT64 ~/GitHub/mcfgthread/build_release
>    $ objdump -pw libmcfgthread-minimal-2.dll | grep -F 'Load Configuration
> Directory'
>    Entry a 0000000000005120 0045004b Load Configuration Directory

Hmm, I noticed when I sent the patch that the address included
h1->root.u.def.value but I didn't include that when figuring out where to
read the size from.  Could you try
if (bfd_get_section_contents (abfd, h1->root.u.def.section->output_section,
      sz, hl->root.u.def.value + h1->root.u.def.section->output_offset, 4))

?  Or maybe someone with knowledge of these APIs can shed some light on
what should be passed here.
LIU Hao March 10, 2025, 6:33 a.m. UTC | #3 
在 2025-3-10 13:51, Jeremy Drake 写道:
> Hmm, I noticed when I sent the patch that the address included
> h1->root.u.def.value but I didn't include that when figuring out where to
> read the size from.  Could you try
> if (bfd_get_section_contents (abfd, h1->root.u.def.section->output_section,
>        sz, hl->root.u.def.value + h1->root.u.def.section->output_offset, 4))
              ^^
I think you meant `h1` here? In that case it works as expected.

> 
> ?  Or maybe someone with knowledge of these APIs can shed some light on
> what should be passed here.


-- 
Best regards,
LIU Hao
LIU Hao March 10, 2025, 9:31 a.m. UTC | #4 
在 2025-3-10 13:51, Jeremy Drake 写道:
> Hmm, I noticed when I sent the patch that the address included
> h1->root.u.def.value but I didn't include that when figuring out where to
> read the size from.  Could you try
> if (bfd_get_section_contents (abfd, h1->root.u.def.section->output_section,
>        sz, hl->root.u.def.value + h1->root.u.def.section->output_offset, 4))
> 
> ?  Or maybe someone with knowledge of these APIs can shed some light on
> what should be passed here.

https://github.com/jeremyd2019/binutils-gdb/pull/2#pullrequestreview-2670271199



-- 
Best regards,
LIU Hao
diff mbox series

Patch

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index 29012688adf..089c9247863 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -593,7 +593,7 @@  _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
   struct internal_extra_pe_aouthdr *extra = &pe->pe_opthdr;
   PEAOUTHDR *aouthdr_out = (PEAOUTHDR *) out;
   bfd_vma sa, fa, ib;
-  IMAGE_DATA_DIRECTORY idata2, idata5, tls;
+  IMAGE_DATA_DIRECTORY idata2, idata5, tls, loadcfg;

   sa = extra->SectionAlignment;
   fa = extra->FileAlignment;
@@ -602,6 +602,7 @@  _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
   idata2 = pe->pe_opthdr.DataDirectory[PE_IMPORT_TABLE];
   idata5 = pe->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE];
   tls = pe->pe_opthdr.DataDirectory[PE_TLS_TABLE];
+  loadcfg = pe->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE];

   if (aouthdr_in->tsize)
     {
@@ -651,6 +652,7 @@  _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
   extra->DataDirectory[PE_IMPORT_TABLE]  = idata2;
   extra->DataDirectory[PE_IMPORT_ADDRESS_TABLE] = idata5;
   extra->DataDirectory[PE_TLS_TABLE] = tls;
+  extra->DataDirectory[PE_LOAD_CONFIG_TABLE] = loadcfg;

   if (extra->DataDirectory[PE_IMPORT_TABLE].VirtualAddress == 0)
     /* Until other .idata fixes are made (pending patch), the entry for
@@ -4573,6 +4575,43 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
 #endif
     }

+  h1 = coff_link_hash_lookup (coff_hash_table (info),
+			      (bfd_get_symbol_leading_char (abfd) != 0
+			       ? "__load_config_used" : "_load_config_used"),
+			      false, false, true);
+  if (h1 != NULL)
+    {
+      char sz[4];
+      if ((h1->root.type == bfd_link_hash_defined
+	   || h1->root.type == bfd_link_hash_defweak)
+	  && h1->root.u.def.section != NULL
+	  && h1->root.u.def.section->output_section != NULL)
+	pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].VirtualAddress =
+	  (h1->root.u.def.value
+	   + h1->root.u.def.section->output_section->vma
+	   + h1->root.u.def.section->output_offset
+	   - pe_data (abfd)->pe_opthdr.ImageBase);
+      else
+	{
+	  _bfd_error_handler
+	    (_("%pB: unable to fill in DataDictionary[10] because __load_config_used is missing"),
+	     abfd);
+	  result = false;
+	}
+      /* the size is stored as the first 4 bytes at _load_config_used */
+      if (bfd_get_section_contents (abfd, h1->root.u.def.section->output_section,
+			      sz, h1->root.u.def.section->output_offset, 4))
+	pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].Size =
+	  bfd_get_32 (abfd, sz);
+      else
+	{
+	  _bfd_error_handler
+	    (_("%pB: unable to fill in DataDictionary[10] because the size can't be read"),
+	     abfd);
+	  result = false;
+	}
+    }
+
 /* If there is a .pdata section and we have linked pdata finally, we
      need to sort the entries ascending.  */
 #if !defined(COFF_WITH_pep) && (defined(COFF_WITH_pex64) || defined(COFF_WITH_peAArch64) || defined(COFF_WITH_peLoongArch64) || defined (COFF_WITH_peRiscV64))