[v2] elf: Avoid possible duplicated local symbol names

Message ID CAMe9rOqPdp7jiLoVvE94ZVWHkkpWwEjQMjmU=Jk-eeGLXYt5DQ@mail.gmail.com
State New
Headers show
Series
  • [v2] elf: Avoid possible duplicated local symbol names
Related show

Commit Message

H.J. Lu via Binutils May 5, 2021, 8:50 p.m.
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.

Comments

Fangrui Song May 5, 2021, 9:36 p.m. | #1
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) ?
H.J. Lu via Binutils May 5, 2021, 10:11 p.m. | #2
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?

[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]$


-- 
H.J.

Patch

From 861b8fe1d397319c829d0d0452deb3232e1ce54b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 5 May 2021 08:37:33 -0700
Subject: [PATCH v2] elf: Avoid possible duplicated local symbol names

Always append ".COUNT" to "XXX" if there are existing local symbols of
"XXX" to avoid possible duplicated local symbol names.

bfd/

	PR ld/27825
	* elflink.c (elf_link_output_symstrtab): Always append ".COUNT"
	to "XXX" if there are existing local symbols of "XXX.COUNT".

ld/

	PR ld/27825
	* testsuite/ld-elf/pr27825-1.d: New file.
	* testsuite/ld-elf/pr27825-1a.s: Likewise.
	* testsuite/ld-elf/pr27825-1b.s: Likewise.
	* testsuite/ld-elf/pr27825-2.d: Likewise.
	* testsuite/ld-elf/pr27825-2a.s: Likewise.
	* testsuite/ld-elf/pr27825-2b.s: Likewise.
---
 bfd/elflink.c                    | 40 +++++++++++++++++++++++++++-----
 ld/testsuite/ld-elf/pr27825-1.d  | 18 ++++++++++++++
 ld/testsuite/ld-elf/pr27825-1a.s |  7 ++++++
 ld/testsuite/ld-elf/pr27825-1b.s |  5 ++++
 ld/testsuite/ld-elf/pr27825-2.d  | 15 ++++++++++++
 ld/testsuite/ld-elf/pr27825-2a.s |  5 ++++
 ld/testsuite/ld-elf/pr27825-2b.s |  3 +++
 ld/testsuite/ld-elf/pr27825-2c.s |  4 ++++
 8 files changed, 91 insertions(+), 6 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr27825-1.d
 create mode 100644 ld/testsuite/ld-elf/pr27825-1a.s
 create mode 100644 ld/testsuite/ld-elf/pr27825-1b.s
 create mode 100644 ld/testsuite/ld-elf/pr27825-2.d
 create mode 100644 ld/testsuite/ld-elf/pr27825-2a.s
 create mode 100644 ld/testsuite/ld-elf/pr27825-2b.s
 create mode 100644 ld/testsuite/ld-elf/pr27825-2c.s

diff --git a/bfd/elflink.c b/bfd/elflink.c
index cb38a025349..24d6e5d92f8 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -9830,6 +9830,7 @@  elf_link_output_symstrtab (void *finf,
 	       && ELF_ST_BIND (elfsym->st_info) == STB_LOCAL)
 	{
 	  struct local_hash_entry *lh;
+	  size_t base_len;
 	  switch (ELF_ST_TYPE (elfsym->st_info))
 	    {
 	    case STT_FILE:
@@ -9840,18 +9841,45 @@  elf_link_output_symstrtab (void *finf,
 		     (&flinfo->local_hash_table, name, true, false);
 	      if (lh == NULL)
 		return 0;
+	      if (lh->size)
+		base_len = lh->size;
+	      else
+		{
+		  base_len = strlen (name);
+		  lh->size = base_len;
+		}
+	      if (lh->count == 0 && base_len > 2)
+		{
+		  /* Always append ".COUNT" if there are local symbols
+		     of "XXX".  */
+		  const char *p = name + base_len - 1;
+		  /* Search for local symbols of "XXX.COUNT".  */
+		  for (; p != name && ISXDIGIT (*p); p--)
+		    ;
+		  if (*p == '.')
+		    {
+		      /* Found "XXX.COUNT".  */
+		      size_t len = p - name;
+		      struct local_hash_entry *lbh;
+		      char *base_name = bfd_alloc (flinfo->output_bfd,
+						   len + 1);
+		      if (base_name == NULL)
+			return 0;
+		      memcpy (base_name, name, len);
+		      base_name[len] = '\0';
+		      lbh = (struct local_hash_entry *) bfd_hash_lookup
+			(&flinfo->local_hash_table, base_name,
+			 false, false);
+		      if (lbh != NULL)
+			lh->count = 1;
+		    }
+		}
 	      if (lh->count)
 		{
 		  /* Append ".COUNT" to duplicated local symbols.  */
 		  size_t count_len;
-		  size_t base_len = lh->size;
 		  char buf[30];
 		  sprintf (buf, "%lx", lh->count);
-		  if (!base_len)
-		    {
-		      base_len = strlen (name);
-		      lh->size = base_len;
-		    }
 		  count_len = strlen (buf);
 		  versioned_name = bfd_alloc (flinfo->output_bfd,
 					      base_len + count_len + 2);
diff --git a/ld/testsuite/ld-elf/pr27825-1.d b/ld/testsuite/ld-elf/pr27825-1.d
new file mode 100644
index 00000000000..d3f348c732f
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-1.d
@@ -0,0 +1,18 @@ 
+#source: pr27825-1a.s
+#source: pr27825-1b.s
+#ld: -e _start --emit-relocs -z unique-symbol
+#nm: --defined-only
+
+#...
+[0-9a-f]+ t bar
+#...
+[0-9a-f]+ t bar.1
+#...
+[0-9a-f]+ t bar.1.1
+#...
+[0-9a-f]+ t bar.1.2
+#...
+[0-9a-f]+ t bar.2.1
+#...
+[0-9a-f]+ t bar.2.2
+#pass
diff --git a/ld/testsuite/ld-elf/pr27825-1a.s b/ld/testsuite/ld-elf/pr27825-1a.s
new file mode 100644
index 00000000000..e6940e17430
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-1a.s
@@ -0,0 +1,7 @@ 
+	.text
+	.globl _start
+_start:
+bar:
+bar.1:
+bar.2:
+	.nop
diff --git a/ld/testsuite/ld-elf/pr27825-1b.s b/ld/testsuite/ld-elf/pr27825-1b.s
new file mode 100644
index 00000000000..2128e802d4b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-1b.s
@@ -0,0 +1,5 @@ 
+	.text
+bar:
+bar.1:
+bar.2:
+	.nop
diff --git a/ld/testsuite/ld-elf/pr27825-2.d b/ld/testsuite/ld-elf/pr27825-2.d
new file mode 100644
index 00000000000..6c5d725df3a
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-2.d
@@ -0,0 +1,15 @@ 
+#source: pr27825-2a.s
+#source: pr27825-2b.s
+#source: pr27825-2c.s
+#ld: -e _start --emit-relocs -z unique-symbol
+#nm: --defined-only
+
+#...
+[0-9a-f]+ t bar
+#...
+[0-9a-f]+ t bar.1
+#...
+[0-9a-f]+ t bar.1.1
+#...
+[0-9a-f]+ t bar.2.1
+#pass
diff --git a/ld/testsuite/ld-elf/pr27825-2a.s b/ld/testsuite/ld-elf/pr27825-2a.s
new file mode 100644
index 00000000000..40fc05bc537
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-2a.s
@@ -0,0 +1,5 @@ 
+	.text
+	.globl _start
+_start:
+bar:
+	.nop
diff --git a/ld/testsuite/ld-elf/pr27825-2b.s b/ld/testsuite/ld-elf/pr27825-2b.s
new file mode 100644
index 00000000000..9e6f96a8bc7
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-2b.s
@@ -0,0 +1,3 @@ 
+	.text
+bar:
+	.nop
diff --git a/ld/testsuite/ld-elf/pr27825-2c.s b/ld/testsuite/ld-elf/pr27825-2c.s
new file mode 100644
index 00000000000..762f8864e5d
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr27825-2c.s
@@ -0,0 +1,4 @@ 
+	.text
+bar.1:
+bar.2:
+	.nop
-- 
2.31.1