xcoff reading dynamic relocs

[Alan Modra]

On Fri, Dec 13, 2024 at 03:07:08PM +0100, Clément Chigot wrote:
> Hi Alan,
> 
> First, I'm no longer actively developing on AIX, it's been ages since
> those patches and not sure who can be considered as a maintainer of it
> now... But I still try to answer !

Thanks for replying, I understand.

> On Fri, Dec 13, 2024 at 6:39 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Fri, Dec 13, 2024 at 03:24:49PM +1030, Alan Modra wrote:
> > > That seems reasonable to me, because prior to this patch l_symndx was
> > > being set to -1 and -2 for .tdata and .tbss symbols resulting in a
> > > buffer overflow when accessing the syms array.  (objdump -R on the
> > > testcase .so segfaults.)
> >
> > Perhaps this?  Using -1 for .tdata l_symndx was clearly wrong since
> > that value is already used..  However, I'm unsure what to do here.
> > ldrel is a public interface.
> 
> The XCOFF documentation doesn't state it, but AFAIK, .tdata and .tbss
> have indeed fixed negative values.

I guess I should assume you chose the correct values when you last
changed xcoff_create_ldrel, and instead abort on what I've now
verified as dead code by inspecting all the calls to
xcoff_create_ldrel.  I hadn't done that analysis and just assumed -1
was used for "no symbol".

OK, so the followup patch which I'm about to commit is this one:

	* xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
	.tdata and .tbss section symbols.
	(xcoff_create_ldrel): Abort on h and hsec both NULL.

diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
index b75fb42eed5..6eb4529b85a 100644
--- a/bfd/xcofflink.c
+++ b/bfd/xcofflink.c
@@ -439,12 +439,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
 
       bfd_xcoff_swap_ldrel_in (abfd, elrel, &ldrel);
 
-      if (ldrel.l_symndx == -1u)
-	relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
-      else if (ldrel.l_symndx < 3)
+      if (ldrel.l_symndx + 2 < 5)
 	{
-	  static const char stdsec[3][8] = { ".text", ".data", ".bss" };
-	  const char *name = stdsec[ldrel.l_symndx];
+	  static const char stdsec[5][8]
+	    = { ".tbss", ".tdata", ".text", ".data", ".bss" };
+	  const char *name = stdsec[ldrel.l_symndx + 2];
 	  asection *sec = bfd_get_section_by_name (abfd, name);
 	  if (sec == NULL)
 	    {
@@ -5090,7 +5089,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
       ldrel.l_symndx = h->ldindx;
     }
   else
-    ldrel.l_symndx = -1;
+    abort ();
 
   ldrel.l_rtype = (irel->r_size << 8) | irel->r_type;
   ldrel.l_rsecnm = output_section->target_index;


> The idea was to introduce those sections without breaking the
> compatibility with previously made binaries.
> Out of my memory, without it TLS is not possible and thus we must preserve them.
> 
> I didn't notice that default -1 at that time. I don't think we ever
> fallback to it and it's probably dead code. A loader relocation should
> target a section (and have those magic numbers) or a symbol (and thus
> be > 3).
> 
> Apart from that I'm totally fine with this idea if it can restore the
> above regression.
> Might give it a try at some point using the GCC buildfarm VM for AIX.
> Though no promise as I don't have much time recently...
> 
> >
> >         * xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
> >         .tdata and .tbss section symbols.
> >         (xcoff_create_ldrel): Set l_symndx to -3 for .tdata.
> >
> > diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
> > index b75fb42eed5..057dfbd72e2 100644
> > --- a/bfd/xcofflink.c
> > +++ b/bfd/xcofflink.c
> > @@ -441,10 +441,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
> >
> >        if (ldrel.l_symndx == -1u)
> >         relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
> > -      else if (ldrel.l_symndx < 3)
> > +      else if (ldrel.l_symndx + 3 < 6)
> >         {
> > -         static const char stdsec[3][8] = { ".text", ".data", ".bss" };
> > -         const char *name = stdsec[ldrel.l_symndx];
> > +         static const char stdsec[6][8]
> > +           = { ".tdata", ".tbss", "", ".text", ".data", ".bss" };
> > +         const char *name = stdsec[ldrel.l_symndx + 3];
> >           asection *sec = bfd_get_section_by_name (abfd, name);
> >           if (sec == NULL)
> >             {
> > @@ -5063,7 +5064,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
> >        else if (strcmp (secname, ".bss") == 0)
> >         ldrel.l_symndx = 2;
> >        else if (strcmp (secname, ".tdata") == 0)
> > -       ldrel.l_symndx = -1;
> > +       ldrel.l_symndx = -3;
> >        else if (strcmp (secname, ".tbss") == 0)
> >         ldrel.l_symndx = -2;
> >        else
> >
> > --
> > Alan Modra

-- 
Alan Modra