[v2] elf: Treat undefined version as hidden

Message ID CAMe9rOqrzfLR=HgxNEMX5d4QeoC3L1LJft+uvY7E0NHY6RZ_rQ@mail.gmail.com
State New
Headers show
Series
  • [v2] elf: Treat undefined version as hidden
Related show

Commit Message

Alan Modra via Binutils Aug. 2, 2021, 2:51 a.m.
On Sun, Aug 1, 2021 at 7:03 PM Alan Modra <amodra@gmail.com> wrote:
>

> On Sun, Aug 01, 2021 at 07:31:04AM -0700, H.J. Lu via Binutils wrote:

> > Since undefined version can't be used to resolve any references,

>

> Testcase please, specifically when the "undefined version" is that


Here is the v2 patch with a testcase.

> for a .dynbss symbol definition.  I did a little experiment myself and

> I don't think you are correct.  My testcase might be faulty, so please

> write one yourself.

>

> Mine had

> a) One shared lib with a definition of a variable foo, a function to

> print foo seen by that lib, and another function to set a hidden alias

> of foo.  foo was made versioned using a version script.

> b) A second lib with a reference to a foo, a function to print foo

> seen by the lib, and another function to set foo.  No version script

> was used.

> c) A main program with a text reference to foo in order to force a

> dynbss copy of foo.

>

> Output is

>

> b foo = 1

> c foo = 1

> c setting foo to 2

> b foo = 2

> c foo = 2

> b setting foo alias to 3

> b foo = 2

> c foo = 2

>

> readelf shows there is just one dynamic foo in the main program, the

> dynbss copy: GLOBAL DEFAULT 25 foo@VER (2).

>

> LD_DEBUG=all | grep foo shows

>    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

>    3541018:     binding file ./prxxxxxc.so [0] to ./prxxxxx [0]: normal symbol `foo'

>    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

>    3541018:     binding file ./prxxxxxb.so [0] to ./prxxxxx [0]: normal symbol `foo' [VER]

>    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

>    3541018:     binding file ./prxxxxx [0] to ./prxxxxx [0]: normal symbol `foo' [VER]

>    3541018:     symbol=foo;  lookup in file=./prxxxxxb.so [0]

>    3541018:     binding file ./prxxxxx [0] to ./prxxxxxb.so [0]: normal symbol `foo' [VER]

>


It is not the same as hidden since it is used to resolve reference to
unversioned symbol.  But it is not the real definition.

-- 
H.J.

Comments

Alan Modra via Binutils Aug. 2, 2021, 11:38 p.m. | #1
On Sun, Aug 1, 2021 at 7:51 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Sun, Aug 1, 2021 at 7:03 PM Alan Modra <amodra@gmail.com> wrote:

> >

> > On Sun, Aug 01, 2021 at 07:31:04AM -0700, H.J. Lu via Binutils wrote:

> > > Since undefined version can't be used to resolve any references,

> >

> > Testcase please, specifically when the "undefined version" is that

>

> Here is the v2 patch with a testcase.

>

> > for a .dynbss symbol definition.  I did a little experiment myself and

> > I don't think you are correct.  My testcase might be faulty, so please

> > write one yourself.

> >

> > Mine had

> > a) One shared lib with a definition of a variable foo, a function to

> > print foo seen by that lib, and another function to set a hidden alias

> > of foo.  foo was made versioned using a version script.

> > b) A second lib with a reference to a foo, a function to print foo

> > seen by the lib, and another function to set foo.  No version script

> > was used.

> > c) A main program with a text reference to foo in order to force a

> > dynbss copy of foo.

> >

> > Output is

> >

> > b foo = 1

> > c foo = 1

> > c setting foo to 2

> > b foo = 2

> > c foo = 2

> > b setting foo alias to 3

> > b foo = 2

> > c foo = 2

> >

> > readelf shows there is just one dynamic foo in the main program, the

> > dynbss copy: GLOBAL DEFAULT 25 foo@VER (2).

> >

> > LD_DEBUG=all | grep foo shows

> >    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

> >    3541018:     binding file ./prxxxxxc.so [0] to ./prxxxxx [0]: normal symbol `foo'

> >    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

> >    3541018:     binding file ./prxxxxxb.so [0] to ./prxxxxx [0]: normal symbol `foo' [VER]

> >    3541018:     symbol=foo;  lookup in file=./prxxxxx [0]

> >    3541018:     binding file ./prxxxxx [0] to ./prxxxxx [0]: normal symbol `foo' [VER]

> >    3541018:     symbol=foo;  lookup in file=./prxxxxxb.so [0]

> >    3541018:     binding file ./prxxxxx [0] to ./prxxxxxb.so [0]: normal symbol `foo' [VER]

> >

>

> It is not the same as hidden since it is used to resolve reference to

> unversioned symbol.  But it is not the real definition.


The versioned symbol from copy relocation in the executable can change
over time.  Today, foo@VER is used to resolve the unversioned reference
in another shared library.  When foo@VER becomes hidden in the shared
library tomorrow,  the executable runs normally.  But foo@VER in the
executable will no longer be used to resolve the unversioned reference.
It is all because foo@VER in the executable is actually undefined.  I
prefer readelf's behavior.

-- 
H.J.
Alan Modra via Binutils Aug. 4, 2021, 2:57 a.m. | #2
On Mon, Aug 02, 2021 at 04:38:48PM -0700, H.J. Lu wrote:
> The versioned symbol from copy relocation in the executable can change

> over time.  Today, foo@VER is used to resolve the unversioned reference

> in another shared library.  When foo@VER becomes hidden in the shared

> library tomorrow,  the executable runs normally.  But foo@VER in the

> executable will no longer be used to resolve the unversioned reference.


OK, that's true, but there are some weird cases handled by glibc's
ld.so.  For example, a reference to foo@VER can be satisfied by an
unversioned foo definition in some cases.  You can see this by
building a shared library with script "VER { global: foo; };",
linking an executable against that library, then building a new
version of that library with script "VER { };".  The new library foo
satisfies foo@VER in the executable.

(The reverse case is true too.  An unversioned reference to foo can be
satisfied by a foo@VER hidden definition, if there is only one
versioned definition of foo!)

> It is all because foo@VER in the executable is actually undefined.  I

> prefer readelf's behavior.


"actually undefined" is complicated.  Yes, if a definition for foo@VER
(or the weird unversioned foo case I mention) is not found, then the
dynamic linker will complain (I think, when resolving the copy reloc).
But once that copy reloc has done its dirty work, then as far as the
program is concerned we really do have a definition in the executable
that overrides other definitions according to normal ELF rules.  I
know you know this.  I'm just trying to make it clear to anyone else
who might be reading this thread.

As far as readelf/nm are concerned you have convinced me that readelf
is more correct, so the patch to make nm agree with readelf is OK.

What are we to do about the gdb failure though?  I'd say it is clear
that what gdb was doing was incorrect, but on the other hand it likely
worked in 99% of cases.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra via Binutils Aug. 4, 2021, 3:30 a.m. | #3
On Tue, Aug 3, 2021 at 7:57 PM Alan Modra <amodra@gmail.com> wrote:
>

> On Mon, Aug 02, 2021 at 04:38:48PM -0700, H.J. Lu wrote:

> > The versioned symbol from copy relocation in the executable can change

> > over time.  Today, foo@VER is used to resolve the unversioned reference

> > in another shared library.  When foo@VER becomes hidden in the shared

> > library tomorrow,  the executable runs normally.  But foo@VER in the

> > executable will no longer be used to resolve the unversioned reference.

>

> OK, that's true, but there are some weird cases handled by glibc's

> ld.so.  For example, a reference to foo@VER can be satisfied by an

> unversioned foo definition in some cases.  You can see this by

> building a shared library with script "VER { global: foo; };",

> linking an executable against that library, then building a new

> version of that library with script "VER { };".  The new library foo

> satisfies foo@VER in the executable.


I think it may be done on purpose.

> (The reverse case is true too.  An unversioned reference to foo can be

> satisfied by a foo@VER hidden definition, if there is only one

> versioned definition of foo!)


This is done on purpose.

> > It is all because foo@VER in the executable is actually undefined.  I

> > prefer readelf's behavior.

>

> "actually undefined" is complicated.  Yes, if a definition for foo@VER

> (or the weird unversioned foo case I mention) is not found, then the

> dynamic linker will complain (I think, when resolving the copy reloc).

> But once that copy reloc has done its dirty work, then as far as the

> program is concerned we really do have a definition in the executable

> that overrides other definitions according to normal ELF rules.  I

> know you know this.  I'm just trying to make it clear to anyone else

> who might be reading this thread.

>

> As far as readelf/nm are concerned you have convinced me that readelf

> is more correct, so the patch to make nm agree with readelf is OK.


I will check it in.   Thanks.

> What are we to do about the gdb failure though?  I'd say it is clear

> that what gdb was doing was incorrect, but on the other hand it likely

> worked in 99% of cases.


Agreed.  Gdb should be fixed.

-- 
H.J.

Patch

From 6f4b28ebf29048c4f0cc9635d213a043ea04f483 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 1 Aug 2021 07:26:20 -0700
Subject: [PATCH v2] elf: Treat undefined version as hidden

Since undefined version can't be used to resolve any references without
the original definition, treat it as hidden.

bfd/

	PR binutils/28158
	* elf.c (_bfd_elf_get_symbol_version_string): Treat undefined
	version as hidden.

ld/

	PR binutils/28158
	* testsuite/ld-elf/linux-x86.exp: Run PR binutils/28158 tests.
	* testsuite/ld-elf/pr28158-1.c: New file.
	* testsuite/ld-elf/pr28158-2.S: Likewise.
	* testsuite/ld-elf/pr28158.nd: Likewise.
	* testsuite/ld-elf/pr28158.rd: Likewise.
	* testsuite/ld-elf/pr28158.t: Likewise.
	* testsuite/ld-elfvers/vers2.dsym: Updated.
	* testsuite/ld-elfvers/vers3.dsym: Likewise.
	* testsuite/ld-elfvers/vers6.dsym: Likewise.
	* testsuite/ld-elfvers/vers19.dsym: Likewise.
	* testsuite/ld-elfvers/vers22.dsym: Likewise.
	* testsuite/ld-elfvers/vers23.dsym: Likewise.
	* testsuite/ld-elfvers/vers23d.dsym: Likewise.
	* testsuite/ld-elfvers/vers27d4.dsym: Likewise.
	* testsuite/ld-elfvers/vers28c.dsym: Likewise.
---
 bfd/elf.c                             |  1 +
 ld/testsuite/ld-elf/linux-x86.exp     | 17 +++++++++++++++++
 ld/testsuite/ld-elf/pr28158-1.c       |  1 +
 ld/testsuite/ld-elf/pr28158-2.S       | 16 ++++++++++++++++
 ld/testsuite/ld-elf/pr28158.nd        |  3 +++
 ld/testsuite/ld-elf/pr28158.rd        |  7 +++++++
 ld/testsuite/ld-elf/pr28158.t         |  6 ++++++
 ld/testsuite/ld-elfvers/vers19.dsym   |  2 +-
 ld/testsuite/ld-elfvers/vers2.dsym    |  2 +-
 ld/testsuite/ld-elfvers/vers22.dsym   |  2 +-
 ld/testsuite/ld-elfvers/vers23.dsym   |  2 +-
 ld/testsuite/ld-elfvers/vers23d.dsym  |  4 ++--
 ld/testsuite/ld-elfvers/vers27d4.dsym |  2 +-
 ld/testsuite/ld-elfvers/vers28c.dsym  |  2 +-
 ld/testsuite/ld-elfvers/vers3.dsym    |  2 +-
 ld/testsuite/ld-elfvers/vers6.dsym    |  6 +++---
 16 files changed, 63 insertions(+), 12 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr28158-1.c
 create mode 100644 ld/testsuite/ld-elf/pr28158-2.S
 create mode 100644 ld/testsuite/ld-elf/pr28158.nd
 create mode 100644 ld/testsuite/ld-elf/pr28158.rd
 create mode 100644 ld/testsuite/ld-elf/pr28158.t

diff --git a/bfd/elf.c b/bfd/elf.c
index d0898855de8..5941eeb010b 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -1944,6 +1944,7 @@  _bfd_elf_get_symbol_version_string (bfd *abfd, asymbol *symbol,
 		{
 		  if (a->vna_other == vernum)
 		    {
+		      *hidden = true;
 		      version_string = a->vna_nodename;
 		      break;
 		    }
diff --git a/ld/testsuite/ld-elf/linux-x86.exp b/ld/testsuite/ld-elf/linux-x86.exp
index 819237415e6..ee03b565faf 100644
--- a/ld/testsuite/ld-elf/linux-x86.exp
+++ b/ld/testsuite/ld-elf/linux-x86.exp
@@ -117,6 +117,23 @@  run_cc_link_tests [list \
 	{{readelf -rn indirect-extern-access-3.rd}} \
 	"indirect-extern-access-2b" \
     ] \
+    [list \
+	"Build pr28158.so" \
+	"-shared -Wl,-version-script,pr27128.t" \
+	"-fPIC" \
+	{ pr28158-1.c } \
+	{} \
+	"pr28158.so" \
+    ] \
+    [list \
+	"Build pr28158" \
+	"$NOPIE_LDFLAGS -Wl,--no-as-needed  \
+	 tmpdir/pr28158.so" \
+	"" \
+	{ pr28158-2.S } \
+	{{readelf {--dyn-syms -W} pr28158.rd} {nm -D pr28158.nd}} \
+	"pr28158" \
+    ] \
 ]
 
 run_ld_link_exec_tests [list \
diff --git a/ld/testsuite/ld-elf/pr28158-1.c b/ld/testsuite/ld-elf/pr28158-1.c
new file mode 100644
index 00000000000..2bc87d0c3c0
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr28158-1.c
@@ -0,0 +1 @@ 
+int foo = 0;
diff --git a/ld/testsuite/ld-elf/pr28158-2.S b/ld/testsuite/ld-elf/pr28158-2.S
new file mode 100644
index 00000000000..4e703b28ed1
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr28158-2.S
@@ -0,0 +1,16 @@ 
+	.text
+	.section	.text.startup,"ax",@progbits
+	.p2align 4
+	.globl	main
+	.type	main, @function
+main:
+	.cfi_startproc
+#ifdef __x86_64__
+	movl	foo(%rip), %eax
+#else
+	movl	foo, %eax
+#endif
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits
diff --git a/ld/testsuite/ld-elf/pr28158.nd b/ld/testsuite/ld-elf/pr28158.nd
new file mode 100644
index 00000000000..3b1a9bcef22
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr28158.nd
@@ -0,0 +1,3 @@ 
+#...
+[0-9a-z]+ B foo@VERS_2.0
+#pass
diff --git a/ld/testsuite/ld-elf/pr28158.rd b/ld/testsuite/ld-elf/pr28158.rd
new file mode 100644
index 00000000000..74884107e8b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr28158.rd
@@ -0,0 +1,7 @@ 
+#ld: -shared -version-script pr27128.t
+#readelf: --dyn-syms --wide
+#target: x86_64-*-linux* i?86-*-linux-gnu
+
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9]+ +OBJECT +GLOBAL +DEFAULT +[1-9]+ foo@VERS_2.0 \(2\)
+#pass
diff --git a/ld/testsuite/ld-elf/pr28158.t b/ld/testsuite/ld-elf/pr28158.t
new file mode 100644
index 00000000000..2b828428d91
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr28158.t
@@ -0,0 +1,6 @@ 
+VERS_2.0 {
+global:
+  foo;
+local:
+  *;
+};
diff --git a/ld/testsuite/ld-elfvers/vers19.dsym b/ld/testsuite/ld-elfvers/vers19.dsym
index a77f9490127..798466fb8bd 100644
--- a/ld/testsuite/ld-elfvers/vers19.dsym
+++ b/ld/testsuite/ld-elfvers/vers19.dsym
@@ -1 +1 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_2\.0 +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_2\.0\) +(0x[0-9a-f]+ )?_?show_foo
diff --git a/ld/testsuite/ld-elfvers/vers2.dsym b/ld/testsuite/ld-elfvers/vers2.dsym
index 30ba91b82b4..f820fdc9717 100644
--- a/ld/testsuite/ld-elfvers/vers2.dsym
+++ b/ld/testsuite/ld-elfvers/vers2.dsym
@@ -1,3 +1,3 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_2\.0 +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_2\.0\) +(0x[0-9a-f]+ )?_?show_foo
 0+ g +DO \*ABS\*	0+ +VERS_XXX_1\.1 VERS_XXX_1\.1
 [0-9a-f]+ g +DF (\.text|\.opd|\*ABS\*)	[0-9a-f]+ +VERS_XXX_1\.1 (0x[0-9a-f]+ )?_?show_xyzzy
diff --git a/ld/testsuite/ld-elfvers/vers22.dsym b/ld/testsuite/ld-elfvers/vers22.dsym
index db2aeec4bc3..65a183345a2 100644
--- a/ld/testsuite/ld-elfvers/vers22.dsym
+++ b/ld/testsuite/ld-elfvers/vers22.dsym
@@ -1 +1 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS\.0 +(0x[0-9a-f]+ )?_?bar
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS\.0\) +(0x[0-9a-f]+ )?_?bar
diff --git a/ld/testsuite/ld-elfvers/vers23.dsym b/ld/testsuite/ld-elfvers/vers23.dsym
index dfd6a3321ad..d6c93b4b508 100644
--- a/ld/testsuite/ld-elfvers/vers23.dsym
+++ b/ld/testsuite/ld-elfvers/vers23.dsym
@@ -1 +1 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS\.0 +(0x[0-9a-f]+ )?_?foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS\.0\) +(0x[0-9a-f]+ )?_?foo
diff --git a/ld/testsuite/ld-elfvers/vers23d.dsym b/ld/testsuite/ld-elfvers/vers23d.dsym
index ab5fbd834e7..12f8c59fe05 100644
--- a/ld/testsuite/ld-elfvers/vers23d.dsym
+++ b/ld/testsuite/ld-elfvers/vers23d.dsym
@@ -1,2 +1,2 @@ 
-[0-9a-f]*      DF \*UND\*	[0-9a-f]*  VERS.0      (0x[0-9a-f][0-9a-f] )?_?bar
-[0-9a-f]*      DF \*UND\*	[0-9a-f]*  VERS.0      (0x[0-9a-f][0-9a-f] )?_?foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS\.0\) +(0x[0-9a-f]+ )?_?bar
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS\.0\) +(0x[0-9a-f]+ )?_?foo
diff --git a/ld/testsuite/ld-elfvers/vers27d4.dsym b/ld/testsuite/ld-elfvers/vers27d4.dsym
index dfd6a3321ad..d6c93b4b508 100644
--- a/ld/testsuite/ld-elfvers/vers27d4.dsym
+++ b/ld/testsuite/ld-elfvers/vers27d4.dsym
@@ -1 +1 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS\.0 +(0x[0-9a-f]+ )?_?foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS\.0\) +(0x[0-9a-f]+ )?_?foo
diff --git a/ld/testsuite/ld-elfvers/vers28c.dsym b/ld/testsuite/ld-elfvers/vers28c.dsym
index 7ad56789eea..a575eff0d2c 100644
--- a/ld/testsuite/ld-elfvers/vers28c.dsym
+++ b/ld/testsuite/ld-elfvers/vers28c.dsym
@@ -1 +1 @@ 
-[0-9a-f]+[ 	]+DF[ 	]+\*UND\*[	]+[0-9a-f]+[ 	]+VERS\.0[ 	]+(0x[0-9a-f]+ )?_?foo
+[0-9a-f]+[ 	]+DF[ 	]+\*UND\*[	]+[0-9a-f]+[ 	]+\(VERS\.0\)[ 	]+(0x[0-9a-f]+ )?_?foo
diff --git a/ld/testsuite/ld-elfvers/vers3.dsym b/ld/testsuite/ld-elfvers/vers3.dsym
index a77f9490127..798466fb8bd 100644
--- a/ld/testsuite/ld-elfvers/vers3.dsym
+++ b/ld/testsuite/ld-elfvers/vers3.dsym
@@ -1 +1 @@ 
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_2\.0 +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_2\.0\) +(0x[0-9a-f]+ )?_?show_foo
diff --git a/ld/testsuite/ld-elfvers/vers6.dsym b/ld/testsuite/ld-elfvers/vers6.dsym
index a5a805377ca..6d073127648 100644
--- a/ld/testsuite/ld-elfvers/vers6.dsym
+++ b/ld/testsuite/ld-elfvers/vers6.dsym
@@ -1,4 +1,4 @@ 
 [0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +Base +(0x[0-9a-f]+ )?_?show_foo
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_2.0 +(0x[0-9a-f]+ )?_?show_foo
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_1.2 +(0x[0-9a-f]+ )?_?show_foo
-[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +VERS_1.1 +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_2.0\) +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_1.2\) +(0x[0-9a-f]+ )?_?show_foo
+[0-9a-f]+ +DF \*UND\*	[0-9a-f]+ +\(VERS_1.1\) +(0x[0-9a-f]+ )?_?show_foo
-- 
2.31.1