[PATCH v3] elf: Always append ".COUNT" to local symbols

[Fangrui Song]

On 2021-05-05, H.J. Lu wrote:
>On Wed, May 5, 2021 at 3:11 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Wed, May 5, 2021 at 2:36 PM Fangrui Song <i@maskray.me> wrote:
>> >
>> > On 2021-05-05, H.J. Lu via Binutils wrote:
>> > >On Wed, May 5, 2021 at 11:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> > >>
>> > >> On Wed, May 5, 2021 at 8:48 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> > >> >
>> > >> > When appending ".COUNT" to duplicated local symbol names of "XXX",
>> > >> > increment COUNT if there is an existing local symbol name of "XXX.COUNT".
>> > >> >
>> > >> > bfd/
>> > >> >
>> > >> >         PR ld/27825
>> > >> >         * elflink.c (elf_link_output_symstrtab): Avoid conflicts with
>> > >> >         local symbol names of "XXX.COUNT".
>> > >> >
>> > >> > ld/
>> > >> >
>> > >> >         PR ld/27825
>> > >> >         * testsuite/ld-elf/pr27825.d: New file.
>> > >> >         * testsuite/ld-elf/pr27825a.s: Likewise.
>> > >> >         * testsuite/ld-elf/pr27825b.s: Likewise.
>> > >> > ---
>> > >> >  bfd/elflink.c                  | 31 ++++++++++++++++++++++++++++---
>> > >> >  ld/testsuite/ld-elf/pr27825.d  | 18 ++++++++++++++++++
>> > >> >  ld/testsuite/ld-elf/pr27825a.s |  7 +++++++
>> > >> >  ld/testsuite/ld-elf/pr27825b.s |  5 +++++
>> > >> >  4 files changed, 58 insertions(+), 3 deletions(-)
>> > >> >  create mode 100644 ld/testsuite/ld-elf/pr27825.d
>> > >> >  create mode 100644 ld/testsuite/ld-elf/pr27825a.s
>> > >> >  create mode 100644 ld/testsuite/ld-elf/pr27825b.s
>> > >> >
>> > >> > diff --git a/bfd/elflink.c b/bfd/elflink.c
>> > >> > index cb38a025349..ac46585f0ab 100644
>> > >> > --- a/bfd/elflink.c
>> > >> > +++ b/bfd/elflink.c
>> > >> > @@ -9845,22 +9845,47 @@ elf_link_output_symstrtab (void *finf,
>> > >> >                   /* Append ".COUNT" to duplicated local symbols.  */
>> > >> >                   size_t count_len;
>> > >> >                   size_t base_len = lh->size;
>> > >> > -                 char buf[30];
>> > >> > -                 sprintf (buf, "%lx", lh->count);
>> > >> > +                 char buf[20];
>> > >> > +                 if (snprintf (buf, sizeof (buf), "%lx", lh->count)
>> > >> > +                     >= (int) sizeof (buf))
>> > >> > +                   return 0;
>> > >> >                   if (!base_len)
>> > >> >                     {
>> > >> >                       base_len = strlen (name);
>> > >> >                       lh->size = base_len;
>> > >> >                     }
>> > >> >                   count_len = strlen (buf);
>> > >> > +                 /* NB: Allocate the extra suffix buffer for possible
>> > >> > +                    change.  */
>> > >> >                   versioned_name = bfd_alloc (flinfo->output_bfd,
>> > >> > -                                             base_len + count_len + 2);
>> > >> > +                                             base_len + sizeof (buf)
>> > >> > +                                             + 2);
>> > >> >                   if (versioned_name == NULL)
>> > >> >                     return 0;
>> > >> >                   memcpy (versioned_name, name, base_len);
>> > >> >                   versioned_name[base_len] = '.';
>> > >> >                   memcpy (versioned_name + base_len + 1, buf,
>> > >> >                           count_len + 1);
>> > >> > +                 do
>> > >> > +                   {
>> > >> > +                     /* Avoid conflicts with local symbol names of
>> > >> > +                        "XXX.COUNT".  */
>> > >> > +                     struct local_hash_entry *lvh
>> > >> > +                       = (struct local_hash_entry *) bfd_hash_lookup
>> > >> > +                       (&flinfo->local_hash_table, versioned_name,
>> > >> > +                        false, false);
>> > >> > +                     if (lvh == NULL)
>> > >> > +                       break;
>> > >> > +                     lh->count++;
>> > >> > +                     if (snprintf (buf, sizeof (buf), "%lx",
>> > >> > +                                   lh->count) >= (int) sizeof (buf))
>> > >> > +                       return 0;
>> > >> > +                     count_len = strlen (buf);
>> > >> > +                     /* NB: Use the existing suffix buffer.  */
>> > >> > +                     memcpy (versioned_name + base_len + 1, buf,
>> > >> > +                             count_len + 1);
>> > >> > +                   }
>> > >> > +                 while (1);
>> > >> >                 }
>> > >> >               lh->count++;
>> > >> >               break;
>> > >
>> > >> This scheme doesn't work.
>> > >>
>> > >
>> > >This one works.
>> > >
>> > >--
>> > >H.J.
>> >
>> > I don't read the implementation.
>> >
>> > If bar.2.1 already exists, can the implementation correctly rename bar.2 (to bar.2.2) ?
>>
>> Is this what you have in mind?

Yes.

If foo cannot be used, try foo.1 foo.2 foo.3 ...
foo can itself be of the form bar.COUNT .

I guess the new code has handled the case.

(Personally I'd prefer zero-based numbering but one-based numbering
shouldn't hurt here.)

For the original FGKASLR (stalled?) request, the linker approach has a
drawback that the linker cannot change DW_AT_linkage_name in debug info,
so there may be a DW_AT_linkage_name / .symtab mismatch.

>> [hjl@gnu-cfl-2 pr27825-2]$ cat pr27825a.s
>> .text
>> .globl _start
>> _start:
>> bar.2:
>> .nop
>> [hjl@gnu-cfl-2 pr27825-2]$ cat pr27825b.s
>> .text
>> bar.2:
>> .nop
>> [hjl@gnu-cfl-2 pr27825-2]$ cat pr27825c.s
>> .text
>> bar.1.1:
>> bar.2.1:
>> .nop
>> [hjl@gnu-cfl-2 pr27825-2]$ make
>> as   -o pr27825a.o pr27825a.s
>> as   -o pr27825b.o pr27825b.s
>> as   -o pr27825c.o pr27825c.s
>> ./ld --emit-relocs -z unique-symbol -e _start -o x pr27825a.o
>> pr27825b.o pr27825c.o
>> ./nm x
>> 0000000000401002 t bar.1.1
>> 0000000000401000 t bar.2
>> 0000000000401001 t bar.2.1
>> 0000000000401002 t bar.2.1.1
>> 0000000000402000 B __bss_start
>> 0000000000402000 D _edata
>> 0000000000402000 B _end
>> 0000000000401000 T _start
>> [hjl@gnu-cfl-2 pr27825-2]$
>>
>
>Here is the v3 patch to always append ".COUNT" to local symbols
>to avoid potential conflicts with existing local symbol "XXX.COUNT".
>
>-- 
>H.J.