ld.so: Support moving versioned symbols between sonames [BZ #24741]
diff mbox series

Message ID 87woh7t7f7.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • ld.so: Support moving versioned symbols between sonames [BZ #24741]
Related show

Commit Message

Florian Weimer June 27, 2019, 2:43 p.m. UTC
This change should be fully backwards-compatible because the old
code aborted the load if a soname mismatch was encountered
(instead of searching further for a matching symbol).  This means
that no different symbols are found.

The soname check was explicitly disabled for the skip_map != NULL
case.  However, this only happens with dl(v)sym and RTLD_NEXT,
and those lookups do not come with a verneed entry that could be used
for the check.

The error check was already explicitly disabled for the skip_map !=
NULL case, that is, when dl(v)sym was called with RTLD_NEXT.  But
_dl_vsym always sets filename in the struct r_found_version argument
to NULL, so the check was not active anyway.  This means that
symbol lookup results for the skip_map != NULL case do not change,
either.

2019-06-27  Florian Weimer  <fweimer@redhat.com>

	[BZ #24741]
	* elf/dl-lookup.c (do_lookup_x): Do not fail if there is a soname
	mismatch in a versioned symbol reference.
	(_dl_lookup_symbol_x): Do not report soname mismatch failures.
	* elf/Makefile [$(build-shared)] (tests): Add tst-sonamemove,
	tst-sonamemove-dlopen.
	(module-names): Add tst-sonamemove-linkmod1,
	tst-sonamemove-runmod1, tst-sonamemove-runmod2.
	(LDFLAGS-tst-sonamemove-linkmod1.so): Set.
	(LDFLAGS-tst-sonamemove-runmod1.so): Likewise.
	(LDFLAGS-tst-sonamemove-runmod2.so): Likewise.
	(tst-sonamemove-runmod1.so): Link against
	tst-sonamemove-runmod2.so.
	(tst-sonamemove): Link against tst-sonamemove-linkmod1.so.
	(tst-sonamemove.out): Depend on tst-sonamemove-runmod1.so,
	tst-sonamemove-runmod2.so.
	(tst-sonamemove-dlopen.out): Likewise.
	* elf/tst-sonamemove.c: New file.
	* elf/tst-sonamemove-dlopen.c: Likewise.
	* elf/tst-sonamemove-linkmod1.c: Likewise.
	* elf/tst-sonamemove-linkmod1.map: Likewise.
	* elf/tst-sonamemove-runmod1.c: Likewise.
	* elf/tst-sonamemove-runmod1.map: Likewise.
	* elf/tst-sonamemove-runmod2.c: Likewise.
	* elf/tst-sonamemove-runmod2.map: Likewise.
	* support/xdlfcn.h (xdlvsym): Declare function.
	* support/xdlfcn.c (xdlvsym): Define funciton.

Comments

Zack Weinberg June 27, 2019, 2:48 p.m. UTC | #1
On Thu, Jun 27, 2019 at 10:44 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> This change should be fully backwards-compatible because the old
> code aborted the load if a soname mismatch was encountered
> (instead of searching further for a matching symbol).  This means
> that no different symbols are found.

I am not familiar enough with the guts of the dynamic linker to review
your code changes, but I endorse the idea.  This should make it
possible to eliminate most of the double definitions of functions in
libc.so and libpthread.so without needing to leave forwarding stubs
behind in libpthread.

zw
Florian Weimer June 27, 2019, 3:08 p.m. UTC | #2
* Zack Weinberg:

> On Thu, Jun 27, 2019 at 10:44 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> This change should be fully backwards-compatible because the old
>> code aborted the load if a soname mismatch was encountered
>> (instead of searching further for a matching symbol).  This means
>> that no different symbols are found.
>
> I am not familiar enough with the guts of the dynamic linker to review
> your code changes, but I endorse the idea.  This should make it
> possible to eliminate most of the double definitions of functions in
> libc.so and libpthread.so without needing to leave forwarding stubs
> behind in libpthread.

Right, I want to use this to fix bug 20188, among other things.

It's also a prerequisite for weak symbol version support.  (In my
interpretation of Ulrich's design, a weak symbol version reference
should be generated by the link editor if all symbols that reference
this version are weak.)  ld.so is expected to support those already, but
any use of them currently runs into that error:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=24718#c16>

Thanks,
Florian
Yann Droneaud June 27, 2019, 4:07 p.m. UTC | #3
Hi,

Le jeudi 27 juin 2019 à 16:43 +0200, Florian Weimer a écrit :
> This change should be fully backwards-compatible because the old
> code aborted the load if a soname mismatch was encountered
> (instead of searching further for a matching symbol).  This means
> that no different symbols are found.
> 
> The soname check was explicitly disabled for the skip_map != NULL
> case.  However, this only happens with dl(v)sym and RTLD_NEXT,
> and those lookups do not come with a verneed entry that could be used
> for the check.
> 
> The error check was already explicitly disabled for the skip_map !=
> NULL case, that is, when dl(v)sym was called with RTLD_NEXT.  But
> _dl_vsym always sets filename in the struct r_found_version argument
> to NULL, so the check was not active anyway.  This means that
> symbol lookup results for the skip_map != NULL case do not change,
> either.
> 
> 2019-06-27  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24741]
> 	* elf/dl-lookup.c (do_lookup_x): Do not fail if there is a soname
> 	mismatch in a versioned symbol reference.
> 	(_dl_lookup_symbol_x): Do not report soname mismatch failures.
> 	* elf/Makefile [$(build-shared)] (tests): Add tst-sonamemove,
> 	tst-sonamemove-dlopen.
> 	(module-names): Add tst-sonamemove-linkmod1,
> 	tst-sonamemove-runmod1, tst-sonamemove-runmod2.
> 	(LDFLAGS-tst-sonamemove-linkmod1.so): Set.
> 	(LDFLAGS-tst-sonamemove-runmod1.so): Likewise.
> 	(LDFLAGS-tst-sonamemove-runmod2.so): Likewise.
> 	(tst-sonamemove-runmod1.so): Link against
> 	tst-sonamemove-runmod2.so.
> 	(tst-sonamemove): Link against tst-sonamemove-linkmod1.so.
> 	(tst-sonamemove.out): Depend on tst-sonamemove-runmod1.so,
> 	tst-sonamemove-runmod2.so.
> 	(tst-sonamemove-dlopen.out): Likewise.
> 	* elf/tst-sonamemove.c: New file.
> 	* elf/tst-sonamemove-dlopen.c: Likewise.
> 	* elf/tst-sonamemove-linkmod1.c: Likewise.
> 	* elf/tst-sonamemove-linkmod1.map: Likewise.
> 	* elf/tst-sonamemove-runmod1.c: Likewise.
> 	* elf/tst-sonamemove-runmod1.map: Likewise.
> 	* elf/tst-sonamemove-runmod2.c: Likewise.
> 	* elf/tst-sonamemove-runmod2.map: Likewise.
> 	* support/xdlfcn.h (xdlvsym): Declare function.
> 	* support/xdlfcn.c (xdlvsym): Define funciton.
> 
> diff --git a/NEWS b/NEWS
> index 8a2fecef47..8cea9f5825 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -34,6 +34,12 @@ Major new features:
>    pointer subtraction within the allocated object, where results might
>    overflow the ptrdiff_t type.
>  
> +* The dynamic linker no longer refuses to load objects which reference
> +  versioned symbols whose implementation has moved to a different soname
> +  since the object has been linked.  The old error message, symbol
> +  FUNCTION-NAME, version SYMBOL-VERSION not defined in file DSO-NAME with
> +  link time reference, is gone.
> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * The functions clock_gettime, clock_getres, clock_settime,
> diff --git a/elf/Makefile b/elf/Makefile
> index 27a2fa8c14..76b0565054 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
> -	 tst-unwind-ctor tst-unwind-main tst-audit13
> +	 tst-unwind-ctor tst-unwind-main tst-audit13 \
> +	 tst-sonamemove tst-sonamemove-dlopen

tst-sonamemove could be name tst-sonamemove-link to make obvious the
difference with -dlopen.

>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -281,7 +282,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
>  		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>  		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
> -		tst-audit13mod1
> +		tst-audit13mod1 tst-sonamemove-linkmod1 \
> +		tst-sonamemove-runmod1 tst-sonamemove-runmod2
>  # Most modules build with _ISOMAC defined, but those filtered out
>  # depend on internal headers.
>  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1410,6 +1412,28 @@ $(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
>  LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
>  tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
>  
> +# tst-sonamemove links against an older implementation of the library.
> +LDFLAGS-tst-sonamemove-linkmod1.so = \
> +  -Wl,--version-script=tst-sonamemove-linkmod1.map \
> +  -Wl,-soname,tst-sonamemove-runmod1.so
> +LDFLAGS-tst-sonamemove-runmod1.so = -Wl,--no-as-needed \
> +  -Wl,--version-script=tst-sonamemove-runmod1.map \
> +  -Wl,-soname,tst-sonamemove-runmod1.so
> +LDFLAGS-tst-sonamemove-runmod2.so = \
> +  -Wl,--version-script=tst-sonamemove-runmod2.map \
> +  -Wl,-soname,tst-sonamemove-runmod2.so
> +$(objpfx)tst-sonamemove-runmod1.so: $(objpfx)tst-sonamemove-runmod2.so
> +# Link against the link module, but depend on the run-time modules
> +# for execution.
> +$(objpfx)tst-sonamemove: $(objpfx)tst-sonamemove-linkmod1.so
> +$(objpfx)tst-sonamemove.out: \
> +  $(objpfx)tst-sonamemove-runmod1.so \
> +  $(objpfx)tst-sonamemove-runmod2.so
> +$(objpfx)tst-sonamemove-dlopen: $(libdl)
> +$(objpfx)tst-sonamemove.out: \
> +  $(objpfx)tst-sonamemove-runmod1.so \
> +  $(objpfx)tst-sonamemove-runmod2.so
> +

The last three lines duplicate those above.


>  # Override -z defs, so that we can reference an undefined symbol.
>  # Force lazy binding for the same reason.
>  LDFLAGS-tst-latepthreadmod.so = \

[...]

> diff --git a/elf/tst-sonamemove-dlopen.c b/elf/tst-sonamemove-dlopen.c
> new file mode 100644
> index 0000000000..c496705044
> --- /dev/null
> +++ b/elf/tst-sonamemove-dlopen.c
> @@ -0,0 +1,35 @@
> +/* Check that a moved versioned symbol can be found using dlsym, dlvsym.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>;.  */
> +
> +#include <stddef.h>
> +#include <support/check.h>
> +#include <support/xdlfcn.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* tst-sonamemove-runmod1.so does not define moved_function, but it
> +     depends on tst-sonamemove-runmod2.so, which does.  */
> +  void *handle = xdlopen ("tst-sonamemove-runmod1.so", RTLD_NOW);
> +  TEST_VERIFY (xdlsym (handle, "moved_function") != NULL);
> +  TEST_VERIFY (xdlvsym (handle, "moved_function", "SONAME_MOVE") != NULL);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-sonamemove-linkmod1.c b/elf/tst-sonamemove-linkmod1.c
> new file mode 100644
> index 0000000000..b8a354e5e3
> --- /dev/null
> +++ b/elf/tst-sonamemove-linkmod1.c
> @@ -0,0 +1,25 @@
> +/* Link interface for (lack of) soname matching in versioned symbol refs.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>;.  */
> +
> +/* This function moved from tst-sonamemove-runmod1.so.  This module is
> +   intended for linking only, to simulate an old application which was
> +   linked against an older version of the library.  */
> +void
> +moved_function (void)
> +{
> +}
> diff --git a/elf/tst-sonamemove-linkmod1.map b/elf/tst-sonamemove-linkmod1.map
> new file mode 100644
> index 0000000000..8fe5904018
> --- /dev/null
> +++ b/elf/tst-sonamemove-linkmod1.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: moved_function;
> +};
> diff --git a/elf/tst-sonamemove-runmod1.c b/elf/tst-sonamemove-runmod1.c
> new file mode 100644
> index 0000000000..5c409e2289
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod1.c
> @@ -0,0 +1,23 @@
> +/* Run-time module whose moved_function moved to a library dependency.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>;.  */
> +
> +/* Dummy function to add the required symbol version.  */
> +void
> +other_function (void)
> +{
> +}
> diff --git a/elf/tst-sonamemove-runmod1.map b/elf/tst-sonamemove-runmod1.map
> new file mode 100644
> index 0000000000..2ea81c6e6f
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod1.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: other_function;
> +};
> diff --git a/elf/tst-sonamemove-runmod2.c b/elf/tst-sonamemove-runmod2.c
> new file mode 100644
> index 0000000000..b5e482eff5
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod2.c
> @@ -0,0 +1,24 @@
> +/* Run-time module with the actual implementation of moved_function.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>;.  */
> +
> +/* In the test scenario, this function was originally in
> +   tst-sonamemove-runmod1.so.  */
> +void
> +moved_function (void)
> +{
> +}
> diff --git a/elf/tst-sonamemove-runmod2.map b/elf/tst-sonamemove-runmod2.map
> new file mode 100644
> index 0000000000..8fe5904018
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod2.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: moved_function;
> +};
> diff --git a/elf/tst-sonamemove.c b/elf/tst-sonamemove.c
> new file mode 100644
> index 0000000000..a80ebcb36f
> --- /dev/null
> +++ b/elf/tst-sonamemove.c
> @@ -0,0 +1,30 @@
> +/* Check that a versioned symbol can move from one library to another.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>;.  */
> +
> +/* moved_function is linked against tst-sonamemove-runmod1.so, but the
> +   actual implementation is in tst-sonamemove-runmod1.so. */

I found this comment unclear. tst-sonamemove is linked against tst-
sonamemove-linkmod1.so, which has tst-sonamemove-runmod1.so soname.
When tst-sonamemove is run, tst-sonamemove-runmod1.so is loaded, which
imply loading tst-sonamemove-runmod2.so as a dependency. tst-
sonamemove-runmod2.so is where move_function's implementation is.

> +void moved_function (void);
> +
> +static int
> +do_test (void)
> +{
> +  moved_function ();
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Regards.
Florian Weimer June 27, 2019, 5:13 p.m. UTC | #4
* Yann Droneaud:

>> diff --git a/elf/Makefile b/elf/Makefile
>> index 27a2fa8c14..76b0565054 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
>> -	 tst-unwind-ctor tst-unwind-main tst-audit13
>> +	 tst-unwind-ctor tst-unwind-main tst-audit13 \
>> +	 tst-sonamemove tst-sonamemove-dlopen
>
> tst-sonamemove could be name tst-sonamemove-link to make obvious the
> difference with -dlopen.

Good idea.

>> +$(objpfx)tst-sonamemove-dlopen: $(libdl)
>> +$(objpfx)tst-sonamemove.out: \
>> +  $(objpfx)tst-sonamemove-runmod1.so \
>> +  $(objpfx)tst-sonamemove-runmod2.so
>> +
>
> The last three lines duplicate those above.

Fixed.

>> +/* moved_function is linked against tst-sonamemove-runmod1.so, but the
>> +   actual implementation is in tst-sonamemove-runmod1.so. */
>
> I found this comment unclear. tst-sonamemove is linked against tst-
> sonamemove-linkmod1.so, which has tst-sonamemove-runmod1.so soname.
> When tst-sonamemove is run, tst-sonamemove-runmod1.so is loaded, which
> imply loading tst-sonamemove-runmod2.so as a dependency. tst-
> sonamemove-runmod2.so is where move_function's implementation is.

I tried to explain this more explictly:

/* At link time, moved_function is bound to the symbol version
   SONAME_MOVE in tst-sonamemove-runmod1.so, using the
   tst-sonamemove-linkmod1.so stub object.

   At run time, the process loads the real tst-sonamemove-runmod1.so,
   which depends on tst-sonamemove-runmod2.so.
   tst-sonamemove-runmod1.so does not define moved_function, but
   tst-sonamemove-runmod2.so does.

   The net effect is that the versioned symbol
   moved_function@SONAME_MOVE moved from the soname
   tst-sonamemove-linkmod1.so at link time to the soname
   tst-sonamemove-linkmod2.so at run time. */

Thanks,
Florian

ld.so: Support moving versioned symbols between sonames [BZ #24741]

This change should be fully backwards-compatible because the old
code aborted the load if a soname mismatch was encountered
(instead of searching further for a matching symbol).  This means
that no different symbols are found.

The soname check was explicitly disabled for the skip_map != NULL
case.  However, this only happens with dl(v)sym and RTLD_NEXT,
and those lookups do not come with a verneed entry that could be used
for the check.

The error check was already explicitly disabled for the skip_map !=
NULL case, that is, when dl(v)sym was called with RTLD_NEXT.  But
_dl_vsym always sets filename in the struct r_found_version argument
to NULL, so the check was not active anyway.  This means that
symbol lookup results for the skip_map != NULL case do not change,
either.

2019-06-27  Florian Weimer  <fweimer@redhat.com>

	[BZ #24741]
	* elf/dl-lookup.c (do_lookup_x): Do not fail if there is a soname
	mismatch in a versioned symbol reference.
	(_dl_lookup_symbol_x): Do not report soname mismatch failures.
	* elf/Makefile [$(build-shared)] (tests): Add tst-sonamemove-link,
	tst-sonamemove-dlopen.
	(module-names): Add tst-sonamemove-linkmod1,
	tst-sonamemove-runmod1, tst-sonamemove-runmod2.
	(LDFLAGS-tst-sonamemove-linkmod1.so): Set.
	(LDFLAGS-tst-sonamemove-runmod1.so): Likewise.
	(LDFLAGS-tst-sonamemove-runmod2.so): Likewise.
	(tst-sonamemove-runmod1.so): Link against
	tst-sonamemove-runmod2.so.
	(tst-sonamemove-link): Link against tst-sonamemove-linkmod1.so.
	(tst-sonamemove-link.out): Depend on tst-sonamemove-runmod1.so,
	tst-sonamemove-runmod2.so.
	(tst-sonamemove-dlopen): Link with -ldl.
	(tst-sonamemove-dlopen.out): Likewise.
	* elf/tst-sonamemove-link.c: New file.
	* elf/tst-sonamemove-dlopen.c: Likewise.
	* elf/tst-sonamemove-linkmod1.c: Likewise.
	* elf/tst-sonamemove-linkmod1.map: Likewise.
	* elf/tst-sonamemove-runmod1.c: Likewise.
	* elf/tst-sonamemove-runmod1.map: Likewise.
	* elf/tst-sonamemove-runmod2.c: Likewise.
	* elf/tst-sonamemove-runmod2.map: Likewise.
	* support/xdlfcn.h (xdlvsym): Declare function.
	* support/xdlfcn.c (xdlvsym): Define funciton.

diff --git a/NEWS b/NEWS
index 8a2fecef47..8cea9f5825 100644
--- a/NEWS
+++ b/NEWS
@@ -34,6 +34,12 @@ Major new features:
   pointer subtraction within the allocated object, where results might
   overflow the ptrdiff_t type.
 
+* The dynamic linker no longer refuses to load objects which reference
+  versioned symbols whose implementation has moved to a different soname
+  since the object has been linked.  The old error message, symbol
+  FUNCTION-NAME, version SYMBOL-VERSION not defined in file DSO-NAME with
+  link time reference, is gone.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The functions clock_gettime, clock_getres, clock_settime,
diff --git a/elf/Makefile b/elf/Makefile
index 27a2fa8c14..a3eefd1b1f 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
 	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
 	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
-	 tst-unwind-ctor tst-unwind-main tst-audit13
+	 tst-unwind-ctor tst-unwind-main tst-audit13 \
+	 tst-sonamemove-link tst-sonamemove-dlopen
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -281,7 +282,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
 		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
 		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
-		tst-audit13mod1
+		tst-audit13mod1 tst-sonamemove-linkmod1 \
+		tst-sonamemove-runmod1 tst-sonamemove-runmod2
 # Most modules build with _ISOMAC defined, but those filtered out
 # depend on internal headers.
 modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1410,6 +1412,28 @@ $(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
 LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
 tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
 
+# tst-sonamemove links against an older implementation of the library.
+LDFLAGS-tst-sonamemove-linkmod1.so = \
+  -Wl,--version-script=tst-sonamemove-linkmod1.map \
+  -Wl,-soname,tst-sonamemove-runmod1.so
+LDFLAGS-tst-sonamemove-runmod1.so = -Wl,--no-as-needed \
+  -Wl,--version-script=tst-sonamemove-runmod1.map \
+  -Wl,-soname,tst-sonamemove-runmod1.so
+LDFLAGS-tst-sonamemove-runmod2.so = \
+  -Wl,--version-script=tst-sonamemove-runmod2.map \
+  -Wl,-soname,tst-sonamemove-runmod2.so
+$(objpfx)tst-sonamemove-runmod1.so: $(objpfx)tst-sonamemove-runmod2.so
+# Link against the link module, but depend on the run-time modules
+# for execution.
+$(objpfx)tst-sonamemove-link: $(objpfx)tst-sonamemove-linkmod1.so
+$(objpfx)tst-sonamemove-link.out: \
+  $(objpfx)tst-sonamemove-runmod1.so \
+  $(objpfx)tst-sonamemove-runmod2.so
+$(objpfx)tst-sonamemove-dlopen: $(libdl)
+$(objpfx)tst-sonamemove-dlopen.out: \
+  $(objpfx)tst-sonamemove-runmod1.so \
+  $(objpfx)tst-sonamemove-runmod2.so
+
 # Override -z defs, so that we can reference an undefined symbol.
 # Force lazy binding for the same reason.
 LDFLAGS-tst-latepthreadmod.so = \
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index e3f42a1efb..eb23cca4e3 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -536,11 +536,7 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
 	}
 
 skip:
-      /* If this current map is the one mentioned in the verneed entry
-	 and we have not found a weak entry, it is a bug.  */
-      if (symidx == STN_UNDEF && version != NULL && version->filename != NULL
-	  && __glibc_unlikely (_dl_name_match_p (version->filename, map)))
-	return -1;
+      ;
     }
   while (++i < n);
 
@@ -810,34 +806,10 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
 
   /* Search the relevant loaded objects for a definition.  */
   for (size_t start = i; *scope != NULL; start = 0, ++scope)
-    {
-      int res = do_lookup_x (undef_name, new_hash, &old_hash, *ref,
-			     &current_value, *scope, start, version, flags,
-			     skip_map, type_class, undef_map);
-      if (res > 0)
-	break;
-
-      if (__glibc_unlikely (res < 0) && skip_map == NULL)
-	{
-	  /* Oh, oh.  The file named in the relocation entry does not
-	     contain the needed symbol.  This code is never reached
-	     for unversioned lookups.  */
-	  assert (version != NULL);
-	  const char *reference_name = undef_map ? undef_map->l_name : "";
-	  struct dl_exception exception;
-	  /* XXX We cannot translate the message.  */
-	  _dl_exception_create_format
-	    (&exception, DSO_FILENAME (reference_name),
-	     "symbol %s version %s not defined in file %s"
-	     " with link time reference%s",
-	     undef_name, version->name, version->filename,
-	     res == -2 ? " (no version symbols)" : "");
-	  _dl_signal_cexception (0, &exception, N_("relocation error"));
-	  _dl_exception_free (&exception);
-	  *ref = NULL;
-	  return 0;
-	}
-    }
+    if (do_lookup_x (undef_name, new_hash, &old_hash, *ref,
+		     &current_value, *scope, start, version, flags,
+		     skip_map, type_class, undef_map) != 0)
+      break;
 
   if (__glibc_unlikely (current_value.s == NULL))
     {
diff --git a/elf/tst-sonamemove-dlopen.c b/elf/tst-sonamemove-dlopen.c
new file mode 100644
index 0000000000..c496705044
--- /dev/null
+++ b/elf/tst-sonamemove-dlopen.c
@@ -0,0 +1,35 @@
+/* Check that a moved versioned symbol can be found using dlsym, dlvsym.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stddef.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static int
+do_test (void)
+{
+  /* tst-sonamemove-runmod1.so does not define moved_function, but it
+     depends on tst-sonamemove-runmod2.so, which does.  */
+  void *handle = xdlopen ("tst-sonamemove-runmod1.so", RTLD_NOW);
+  TEST_VERIFY (xdlsym (handle, "moved_function") != NULL);
+  TEST_VERIFY (xdlvsym (handle, "moved_function", "SONAME_MOVE") != NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-sonamemove-link.c b/elf/tst-sonamemove-link.c
new file mode 100644
index 0000000000..4bc3bf32f8
--- /dev/null
+++ b/elf/tst-sonamemove-link.c
@@ -0,0 +1,41 @@
+/* Check that a versioned symbol can move from one library to another.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* At link time, moved_function is bound to the symbol version
+   SONAME_MOVE in tst-sonamemove-runmod1.so, using the
+   tst-sonamemove-linkmod1.so stub object.
+
+   At run time, the process loads the real tst-sonamemove-runmod1.so,
+   which depends on tst-sonamemove-runmod2.so.
+   tst-sonamemove-runmod1.so does not define moved_function, but
+   tst-sonamemove-runmod2.so does.
+
+   The net effect is that the versioned symbol
+   moved_function@SONAME_MOVE moved from the soname
+   tst-sonamemove-linkmod1.so at link time to the soname
+   tst-sonamemove-linkmod2.so at run time. */
+void moved_function (void);
+
+static int
+do_test (void)
+{
+  moved_function ();
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-sonamemove-linkmod1.c b/elf/tst-sonamemove-linkmod1.c
new file mode 100644
index 0000000000..b8a354e5e3
--- /dev/null
+++ b/elf/tst-sonamemove-linkmod1.c
@@ -0,0 +1,25 @@
+/* Link interface for (lack of) soname matching in versioned symbol refs.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This function moved from tst-sonamemove-runmod1.so.  This module is
+   intended for linking only, to simulate an old application which was
+   linked against an older version of the library.  */
+void
+moved_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-linkmod1.map b/elf/tst-sonamemove-linkmod1.map
new file mode 100644
index 0000000000..8fe5904018
--- /dev/null
+++ b/elf/tst-sonamemove-linkmod1.map
@@ -0,0 +1,3 @@
+SONAME_MOVE {
+  global: moved_function;
+};
diff --git a/elf/tst-sonamemove-runmod1.c b/elf/tst-sonamemove-runmod1.c
new file mode 100644
index 0000000000..5c409e2289
--- /dev/null
+++ b/elf/tst-sonamemove-runmod1.c
@@ -0,0 +1,23 @@
+/* Run-time module whose moved_function moved to a library dependency.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Dummy function to add the required symbol version.  */
+void
+other_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-runmod1.map b/elf/tst-sonamemove-runmod1.map
new file mode 100644
index 0000000000..2ea81c6e6f
--- /dev/null
+++ b/elf/tst-sonamemove-runmod1.map
@@ -0,0 +1,3 @@
+SONAME_MOVE {
+  global: other_function;
+};
diff --git a/elf/tst-sonamemove-runmod2.c b/elf/tst-sonamemove-runmod2.c
new file mode 100644
index 0000000000..b5e482eff5
--- /dev/null
+++ b/elf/tst-sonamemove-runmod2.c
@@ -0,0 +1,24 @@
+/* Run-time module with the actual implementation of moved_function.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* In the test scenario, this function was originally in
+   tst-sonamemove-runmod1.so.  */
+void
+moved_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-runmod2.map b/elf/tst-sonamemove-runmod2.map
new file mode 100644
index 0000000000..8fe5904018
--- /dev/null
+++ b/elf/tst-sonamemove-runmod2.map
@@ -0,0 +1,3 @@
+SONAME_MOVE {
+  global: moved_function;
+};
diff --git a/support/xdlfcn.c b/support/xdlfcn.c
index b2e5c21134..11fe4896d1 100644
--- a/support/xdlfcn.c
+++ b/support/xdlfcn.c
@@ -51,6 +51,26 @@ xdlsym (void *handle, const char *symbol)
   return sym;
 }
 
+void *
+xdlvsym (void *handle, const char *symbol, const char *version)
+{
+  /* Clear any pending errors.  */
+  dlerror ();
+
+  void *sym = dlvsym (handle, symbol, version);
+
+  if (sym == NULL)
+    {
+      const char *error = dlerror ();
+      if (error != NULL)
+        FAIL_EXIT1 ("error: dlvsym: %s\n", error);
+      /* If there was no error, we found a NULL symbol.  Return the
+         NULL value in this case.  */
+    }
+
+  return sym;
+}
+
 void
 xdlclose (void *handle)
 {
diff --git a/support/xdlfcn.h b/support/xdlfcn.h
index c9cd810a8a..7f8d4930fc 100644
--- a/support/xdlfcn.h
+++ b/support/xdlfcn.h
@@ -27,6 +27,7 @@ __BEGIN_DECLS
 void *xdlopen (const char *filename, int flags);
 void *xdlmopen (Lmid_t lmid, const char *filename, int flags);
 void *xdlsym (void *handle, const char *symbol);
+void *xdlvsym (void *handle, const char *symbol, const char *version);
 void xdlclose (void *handle);
 
 __END_DECLS
Yann Droneaud June 27, 2019, 5:18 p.m. UTC | #5
Hi,

Le jeudi 27 juin 2019 à 19:13 +0200, Florian Weimer a écrit :
> * Yann Droneaud:
> 
> > > diff --git a/elf/Makefile b/elf/Makefile
> > > index 27a2fa8c14..76b0565054 100644
> > > --- a/elf/Makefile
> > > +++ b/elf/Makefile
> > > @@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
> > >  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
> > >  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
> > >  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
> > > -	 tst-unwind-ctor tst-unwind-main tst-audit13
> > > +	 tst-unwind-ctor tst-unwind-main tst-audit13 \
> > > +	 tst-sonamemove tst-sonamemove-dlopen
> > 
> > tst-sonamemove could be name tst-sonamemove-link to make obvious the
> > difference with -dlopen.
> 
> Good idea.
> 
> > > +$(objpfx)tst-sonamemove-dlopen: $(libdl)
> > > +$(objpfx)tst-sonamemove.out: \
> > > +  $(objpfx)tst-sonamemove-runmod1.so \
> > > +  $(objpfx)tst-sonamemove-runmod2.so
> > > +
> > 
> > The last three lines duplicate those above.
> 
> Fixed.
> 
> > > +/* moved_function is linked against tst-sonamemove-runmod1.so, but the
> > > +   actual implementation is in tst-sonamemove-runmod1.so. */
> > 
> > I found this comment unclear. tst-sonamemove is linked against tst-
> > sonamemove-linkmod1.so, which has tst-sonamemove-runmod1.so soname.
> > When tst-sonamemove is run, tst-sonamemove-runmod1.so is loaded, which
> > imply loading tst-sonamemove-runmod2.so as a dependency. tst-
> > sonamemove-runmod2.so is where move_function's implementation is.
> 
> I tried to explain this more explictly:
> 
> /* At link time, moved_function is bound to the symbol version
>    SONAME_MOVE in tst-sonamemove-runmod1.so, using the
>    tst-sonamemove-linkmod1.so stub object.
> 
>    At run time, the process loads the real tst-sonamemove-runmod1.so,
>    which depends on tst-sonamemove-runmod2.so.
>    tst-sonamemove-runmod1.so does not define moved_function, but
>    tst-sonamemove-runmod2.so does.
> 
>    The net effect is that the versioned symbol
>    moved_function@SONAME_MOVE moved from the soname
>    tst-sonamemove-linkmod1.so at link time to the soname
>    tst-sonamemove-linkmod2.so at run time. */
> 

Thanks.

LGTM.

Reviewed-by: Yann Droneaud <ydroneaud@opteya.com>

Regards.
Carlos O'Donell June 27, 2019, 7:59 p.m. UTC | #6
On 6/27/19 1:13 PM, Florian Weimer wrote:
> * Yann Droneaud:
> 
>>> diff --git a/elf/Makefile b/elf/Makefile
>>> index 27a2fa8c14..76b0565054 100644
>>> --- a/elf/Makefile
>>> +++ b/elf/Makefile
>>> @@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>>>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>>>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>>>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
>>> -	 tst-unwind-ctor tst-unwind-main tst-audit13
>>> +	 tst-unwind-ctor tst-unwind-main tst-audit13 \
>>> +	 tst-sonamemove tst-sonamemove-dlopen
>>
>> tst-sonamemove could be name tst-sonamemove-link to make obvious the
>> difference with -dlopen.
> 
> Good idea.
> 
>>> +$(objpfx)tst-sonamemove-dlopen: $(libdl)
>>> +$(objpfx)tst-sonamemove.out: \
>>> +  $(objpfx)tst-sonamemove-runmod1.so \
>>> +  $(objpfx)tst-sonamemove-runmod2.so
>>> +
>>
>> The last three lines duplicate those above.
> 
> Fixed.
> 
>>> +/* moved_function is linked against tst-sonamemove-runmod1.so, but the
>>> +   actual implementation is in tst-sonamemove-runmod1.so. */
>>
>> I found this comment unclear. tst-sonamemove is linked against tst-
>> sonamemove-linkmod1.so, which has tst-sonamemove-runmod1.so soname.
>> When tst-sonamemove is run, tst-sonamemove-runmod1.so is loaded, which
>> imply loading tst-sonamemove-runmod2.so as a dependency. tst-
>> sonamemove-runmod2.so is where move_function's implementation is.
> 
> I tried to explain this more explictly:
> 
> /* At link time, moved_function is bound to the symbol version
>    SONAME_MOVE in tst-sonamemove-runmod1.so, using the
>    tst-sonamemove-linkmod1.so stub object.
> 
>    At run time, the process loads the real tst-sonamemove-runmod1.so,
>    which depends on tst-sonamemove-runmod2.so.
>    tst-sonamemove-runmod1.so does not define moved_function, but
>    tst-sonamemove-runmod2.so does.
> 
>    The net effect is that the versioned symbol
>    moved_function@SONAME_MOVE moved from the soname
>    tst-sonamemove-linkmod1.so at link time to the soname
>    tst-sonamemove-linkmod2.so at run time. */
> 
> Thanks,
> Florian
> 
> ld.so: Support moving versioned symbols between sonames [BZ #24741]
> 
> This change should be fully backwards-compatible because the old
> code aborted the load if a soname mismatch was encountered
> (instead of searching further for a matching symbol).  This means
> that no different symbols are found.
> 
> The soname check was explicitly disabled for the skip_map != NULL
> case.  However, this only happens with dl(v)sym and RTLD_NEXT,
> and those lookups do not come with a verneed entry that could be used
> for the check.
> 
> The error check was already explicitly disabled for the skip_map !=
> NULL case, that is, when dl(v)sym was called with RTLD_NEXT.  But
> _dl_vsym always sets filename in the struct r_found_version argument
> to NULL, so the check was not active anyway.  This means that
> symbol lookup results for the skip_map != NULL case do not change,
> either.

I am strong proponent of this change. I think it goes in the right
direction. While symbol binding to a version is important, that version
may need to be moved to another shared library and that flexibility is
important during shuffling of the implementation in glibc or any other
project. It could eventually allow us to split glibc into *further*
smaller libraries, or unify it into a single hyper-optimized library/loader,
whichever we pick, it helps.

It leaves the open question: Why was the binding so specific? I don't
have a good answer for that, and I think we should clarify the language
of the ELF symbol versioning specification for this change. I have gone
through the specification again and I see nothing that would require
this behaviour.

I have dropped Ulrich's text here in our wiki:
https://sourceware.org/glibc/wiki/ELFSymbolVersioning

There is a hint here:
~~~
These tests have to be recursively performed for all objects and their
dependencies.  This way it is possible to recognize libraries which
are too old and don't contain all the symbols or contain incompatible
implementations.  Without this kind of test one could end up with
runtime errors which don't provide helpful information.
~~~
Notice the wording "don't contain all the symbols", which is actually
the case here when you *move* a symbol from one DSO to another, you
no longer get an error that the library may be incomplete. It seems
overly restrictive to me, and doesn't match the semantics of symbol
interposition where you can get part of the implementation in a
preloaded library and the rest in the original library. Functionally
interposition supports the behaviour we want which is to have another
library provide those symbols, and this change simply makes it possible
to do this without LD_PRELOAD or changes in search ordering.

Can we update our wiki page to reflect what glibc actually implements
for symbol versioning? We could put it in git, or work with HJ to get
it upstream into the ABI docs, or just edit the wiki (for now).

> 2019-06-27  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24741]
> 	* elf/dl-lookup.c (do_lookup_x): Do not fail if there is a soname
> 	mismatch in a versioned symbol reference.
> 	(_dl_lookup_symbol_x): Do not report soname mismatch failures.
> 	* elf/Makefile [$(build-shared)] (tests): Add tst-sonamemove-link,
> 	tst-sonamemove-dlopen.
> 	(module-names): Add tst-sonamemove-linkmod1,
> 	tst-sonamemove-runmod1, tst-sonamemove-runmod2.
> 	(LDFLAGS-tst-sonamemove-linkmod1.so): Set.
> 	(LDFLAGS-tst-sonamemove-runmod1.so): Likewise.
> 	(LDFLAGS-tst-sonamemove-runmod2.so): Likewise.
> 	(tst-sonamemove-runmod1.so): Link against
> 	tst-sonamemove-runmod2.so.
> 	(tst-sonamemove-link): Link against tst-sonamemove-linkmod1.so.
> 	(tst-sonamemove-link.out): Depend on tst-sonamemove-runmod1.so,
> 	tst-sonamemove-runmod2.so.
> 	(tst-sonamemove-dlopen): Link with -ldl.
> 	(tst-sonamemove-dlopen.out): Likewise.
> 	* elf/tst-sonamemove-link.c: New file.
> 	* elf/tst-sonamemove-dlopen.c: Likewise.
> 	* elf/tst-sonamemove-linkmod1.c: Likewise.
> 	* elf/tst-sonamemove-linkmod1.map: Likewise.
> 	* elf/tst-sonamemove-runmod1.c: Likewise.
> 	* elf/tst-sonamemove-runmod1.map: Likewise.
> 	* elf/tst-sonamemove-runmod2.c: Likewise.
> 	* elf/tst-sonamemove-runmod2.map: Likewise.
> 	* support/xdlfcn.h (xdlvsym): Declare function.
> 	* support/xdlfcn.c (xdlvsym): Define funciton.
> 

OK for master if:

- we document the change in our copy of the standard describing symbol
  versioning.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/NEWS b/NEWS
> index 8a2fecef47..8cea9f5825 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -34,6 +34,12 @@ Major new features:
>    pointer subtraction within the allocated object, where results might
>    overflow the ptrdiff_t type.
>  
> +* The dynamic linker no longer refuses to load objects which reference
> +  versioned symbols whose implementation has moved to a different soname
> +  since the object has been linked.  The old error message, symbol
> +  FUNCTION-NAME, version SYMBOL-VERSION not defined in file DSO-NAME with
> +  link time reference, is gone.

OK. I actually like that the binding information tells you *where* the original
linkage was from, since that information may be important some day.

> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * The functions clock_gettime, clock_getres, clock_settime,
> diff --git a/elf/Makefile b/elf/Makefile
> index 27a2fa8c14..a3eefd1b1f 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -191,7 +191,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
> -	 tst-unwind-ctor tst-unwind-main tst-audit13
> +	 tst-unwind-ctor tst-unwind-main tst-audit13 \
> +	 tst-sonamemove-link tst-sonamemove-dlopen

OK. Two new tests.

>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -281,7 +282,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
>  		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>  		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
> -		tst-audit13mod1
> +		tst-audit13mod1 tst-sonamemove-linkmod1 \
> +		tst-sonamemove-runmod1 tst-sonamemove-runmod2

OK. Three new DSOs.

>  # Most modules build with _ISOMAC defined, but those filtered out
>  # depend on internal headers.
>  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1410,6 +1412,28 @@ $(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
>  LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
>  tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
>  
> +# tst-sonamemove links against an older implementation of the library.
> +LDFLAGS-tst-sonamemove-linkmod1.so = \
> +  -Wl,--version-script=tst-sonamemove-linkmod1.map \
> +  -Wl,-soname,tst-sonamemove-runmod1.so

OK. Same soname.

> +LDFLAGS-tst-sonamemove-runmod1.so = -Wl,--no-as-needed \
> +  -Wl,--version-script=tst-sonamemove-runmod1.map \
> +  -Wl,-soname,tst-sonamemove-runmod1.so

OK. Same soname.

> +LDFLAGS-tst-sonamemove-runmod2.so = \
> +  -Wl,--version-script=tst-sonamemove-runmod2.map \
> +  -Wl,-soname,tst-sonamemove-runmod2.so

OK. Different soname.

> +$(objpfx)tst-sonamemove-runmod1.so: $(objpfx)tst-sonamemove-runmod2.so
> +# Link against the link module, but depend on the run-time modules
> +# for execution.
> +$(objpfx)tst-sonamemove-link: $(objpfx)tst-sonamemove-linkmod1.so
> +$(objpfx)tst-sonamemove-link.out: \
> +  $(objpfx)tst-sonamemove-runmod1.so \
> +  $(objpfx)tst-sonamemove-runmod2.so

OK, Build the dsos before running the test.

> +$(objpfx)tst-sonamemove-dlopen: $(libdl)
> +$(objpfx)tst-sonamemove-dlopen.out: \
> +  $(objpfx)tst-sonamemove-runmod1.so \
> +  $(objpfx)tst-sonamemove-runmod2.so

OK. likewise.

> +
>  # Override -z defs, so that we can reference an undefined symbol.
>  # Force lazy binding for the same reason.
>  LDFLAGS-tst-latepthreadmod.so = \
> diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
> index e3f42a1efb..eb23cca4e3 100644
> --- a/elf/dl-lookup.c
> +++ b/elf/dl-lookup.c
> @@ -536,11 +536,7 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
>  	}
>  
>  skip:
> -      /* If this current map is the one mentioned in the verneed entry
> -	 and we have not found a weak entry, it is a bug.  */
> -      if (symidx == STN_UNDEF && version != NULL && version->filename != NULL
> -	  && __glibc_unlikely (_dl_name_match_p (version->filename, map)))
> -	return -1;
> +      ;

OK.

An audit of _dl_name_match_p() which is predominantly used for this purpose
shows callers in dl-load.c, dl-lookup.c, dl-misc.c, dl-version.c, and rtld.c.

I found one questionable assert in dl-lookup.c

  99   if (version != NULL)
 100     {
 101       if (__glibc_unlikely (verstab == NULL))
 102         {
 103           /* We need a versioned symbol but haven't found any.  If
 104              this is the object which is referenced in the verneed
 105              entry it is a bug in the library since a symbol must
 106              not simply disappear.
 107 
 108              It would also be a bug in the object since it means that
 109              the list of required versions is incomplete and so the
 110              tests in dl-version.c haven't found a problem.*/
 111           assert (version->filename == NULL
 112                   || ! _dl_name_match_p (version->filename, map));
 113 
 114           /* Otherwise we accept the symbol.  */

Why would we assert here? Can we reasonably handle this error?
If it is the object referenced in verneed then version->filename != NULL
and so the assert triggers? That seems overly restrictive, and I expect
we don't get here because earlier dl-version.c checks fail. I'm not asking
you to cleanup this code, but it does beg the next question.

What happens if you move the last versioned symbol out of a library?
Are you allowed to stop using the version script in the library that
has zero versioned symbols? I think the answer is "No." Because we
still check that all libraries provide the version sets that they
had at link time? Is that still OK? 

Everything else looks OK.


>      }
>    while (++i < n);
>  
> @@ -810,34 +806,10 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
>  
>    /* Search the relevant loaded objects for a definition.  */
>    for (size_t start = i; *scope != NULL; start = 0, ++scope)
> -    {
> -      int res = do_lookup_x (undef_name, new_hash, &old_hash, *ref,
> -			     &current_value, *scope, start, version, flags,
> -			     skip_map, type_class, undef_map);
> -      if (res > 0)
> -	break;
> -
> -      if (__glibc_unlikely (res < 0) && skip_map == NULL)
> -	{
> -	  /* Oh, oh.  The file named in the relocation entry does not
> -	     contain the needed symbol.  This code is never reached
> -	     for unversioned lookups.  */
> -	  assert (version != NULL);
> -	  const char *reference_name = undef_map ? undef_map->l_name : "";
> -	  struct dl_exception exception;
> -	  /* XXX We cannot translate the message.  */
> -	  _dl_exception_create_format
> -	    (&exception, DSO_FILENAME (reference_name),
> -	     "symbol %s version %s not defined in file %s"
> -	     " with link time reference%s",
> -	     undef_name, version->name, version->filename,
> -	     res == -2 ? " (no version symbols)" : "");
> -	  _dl_signal_cexception (0, &exception, N_("relocation error"));
> -	  _dl_exception_free (&exception);
> -	  *ref = NULL;
> -	  return 0;
> -	}
> -    }
> +    if (do_lookup_x (undef_name, new_hash, &old_hash, *ref,
> +		     &current_value, *scope, start, version, flags,
> +		     skip_map, type_class, undef_map) != 0)
> +      break;

OK. This removes the well known code that does this check from _dl_lookup_symbol_x.

I didn't know about the case in do_lookup_x.

>  
>    if (__glibc_unlikely (current_value.s == NULL))
>      {
> diff --git a/elf/tst-sonamemove-dlopen.c b/elf/tst-sonamemove-dlopen.c
> new file mode 100644
> index 0000000000..c496705044
> --- /dev/null
> +++ b/elf/tst-sonamemove-dlopen.c
> @@ -0,0 +1,35 @@
> +/* Check that a moved versioned symbol can be found using dlsym, dlvsym.

OK.

> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stddef.h>
> +#include <support/check.h>
> +#include <support/xdlfcn.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* tst-sonamemove-runmod1.so does not define moved_function, but it
> +     depends on tst-sonamemove-runmod2.so, which does.  */

OK.

> +  void *handle = xdlopen ("tst-sonamemove-runmod1.so", RTLD_NOW);
> +  TEST_VERIFY (xdlsym (handle, "moved_function") != NULL);
> +  TEST_VERIFY (xdlvsym (handle, "moved_function", "SONAME_MOVE") != NULL);

OK.

> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-sonamemove-link.c b/elf/tst-sonamemove-link.c
> new file mode 100644
> index 0000000000..4bc3bf32f8
> --- /dev/null
> +++ b/elf/tst-sonamemove-link.c
> @@ -0,0 +1,41 @@
> +/* Check that a versioned symbol can move from one library to another.

OK.

> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* At link time, moved_function is bound to the symbol version
> +   SONAME_MOVE in tst-sonamemove-runmod1.so, using the
> +   tst-sonamemove-linkmod1.so stub object.

OK.

> +
> +   At run time, the process loads the real tst-sonamemove-runmod1.so,
> +   which depends on tst-sonamemove-runmod2.so.
> +   tst-sonamemove-runmod1.so does not define moved_function, but
> +   tst-sonamemove-runmod2.so does.

OK.

> +
> +   The net effect is that the versioned symbol
> +   moved_function@SONAME_MOVE moved from the soname
> +   tst-sonamemove-linkmod1.so at link time to the soname
> +   tst-sonamemove-linkmod2.so at run time. */

OK.

> +void moved_function (void);
> +
> +static int
> +do_test (void)
> +{
> +  moved_function ();
> +  return 0;

OK.

> +}
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-sonamemove-linkmod1.c b/elf/tst-sonamemove-linkmod1.c
> new file mode 100644
> index 0000000000..b8a354e5e3
> --- /dev/null
> +++ b/elf/tst-sonamemove-linkmod1.c
> @@ -0,0 +1,25 @@
> +/* Link interface for (lack of) soname matching in versioned symbol refs.
> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* This function moved from tst-sonamemove-runmod1.so.  This module is
> +   intended for linking only, to simulate an old application which was
> +   linked against an older version of the library.  */
> +void
> +moved_function (void)
> +{
> +}

OK.

> diff --git a/elf/tst-sonamemove-linkmod1.map b/elf/tst-sonamemove-linkmod1.map
> new file mode 100644
> index 0000000000..8fe5904018
> --- /dev/null
> +++ b/elf/tst-sonamemove-linkmod1.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: moved_function;
> +};

OK.

> diff --git a/elf/tst-sonamemove-runmod1.c b/elf/tst-sonamemove-runmod1.c
> new file mode 100644
> index 0000000000..5c409e2289
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod1.c
> @@ -0,0 +1,23 @@
> +/* Run-time module whose moved_function moved to a library dependency.
> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Dummy function to add the required symbol version.  */
> +void
> +other_function (void)
> +{
> +}

OK. Ah, so you do need a dummy function to preserve the required version
set or we still get another error? I expect the answer is "Yes" and that
we're not entirely willing to remove this requirement. We're happy to move
symbols, but what about keeping the original symbol set intact? Would we
relax that in a subsequent patch?

> diff --git a/elf/tst-sonamemove-runmod1.map b/elf/tst-sonamemove-runmod1.map
> new file mode 100644
> index 0000000000..2ea81c6e6f
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod1.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: other_function;
> +};

OK.

> diff --git a/elf/tst-sonamemove-runmod2.c b/elf/tst-sonamemove-runmod2.c
> new file mode 100644
> index 0000000000..b5e482eff5
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod2.c
> @@ -0,0 +1,24 @@
> +/* Run-time module with the actual implementation of moved_function.
> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* In the test scenario, this function was originally in
> +   tst-sonamemove-runmod1.so.  */
> +void
> +moved_function (void)
> +{
> +}

OK.

> diff --git a/elf/tst-sonamemove-runmod2.map b/elf/tst-sonamemove-runmod2.map
> new file mode 100644
> index 0000000000..8fe5904018
> --- /dev/null
> +++ b/elf/tst-sonamemove-runmod2.map
> @@ -0,0 +1,3 @@
> +SONAME_MOVE {
> +  global: moved_function;
> +};

OK.

> diff --git a/support/xdlfcn.c b/support/xdlfcn.c
> index b2e5c21134..11fe4896d1 100644
> --- a/support/xdlfcn.c
> +++ b/support/xdlfcn.c
> @@ -51,6 +51,26 @@ xdlsym (void *handle, const char *symbol)
>    return sym;
>  }
>  
> +void *
> +xdlvsym (void *handle, const char *symbol, const char *version)
> +{
> +  /* Clear any pending errors.  */
> +  dlerror ();
> +
> +  void *sym = dlvsym (handle, symbol, version);
> +
> +  if (sym == NULL)
> +    {
> +      const char *error = dlerror ();
> +      if (error != NULL)
> +        FAIL_EXIT1 ("error: dlvsym: %s\n", error);
> +      /* If there was no error, we found a NULL symbol.  Return the
> +         NULL value in this case.  */
> +    }
> +
> +  return sym;
> +}

OK.

> +
>  void
>  xdlclose (void *handle)
>  {
> diff --git a/support/xdlfcn.h b/support/xdlfcn.h
> index c9cd810a8a..7f8d4930fc 100644
> --- a/support/xdlfcn.h
> +++ b/support/xdlfcn.h
> @@ -27,6 +27,7 @@ __BEGIN_DECLS
>  void *xdlopen (const char *filename, int flags);
>  void *xdlmopen (Lmid_t lmid, const char *filename, int flags);
>  void *xdlsym (void *handle, const char *symbol);
> +void *xdlvsym (void *handle, const char *symbol, const char *version);
>  void xdlclose (void *handle);
>  
>  __END_DECLS
> 

OK.
Florian Weimer June 27, 2019, 9:12 p.m. UTC | #7
* Carlos O'Donell:

> I have dropped Ulrich's text here in our wiki:

Uhm, I don't see a permission notice in the original document.
Did you ask Ulrich?

I don't think we should block this patch on the outcome of that
discussion.

(I would definitely prefer a text file in Git, by the way.)

> An audit of _dl_name_match_p() which is predominantly used for this purpose
> shows callers in dl-load.c, dl-lookup.c, dl-misc.c, dl-version.c, and rtld.c.
>
> I found one questionable assert in dl-lookup.c
>
>   99   if (version != NULL)
>  100     {
>  101       if (__glibc_unlikely (verstab == NULL))
>  102         {
>  103           /* We need a versioned symbol but haven't found any.  If
>  104              this is the object which is referenced in the verneed
>  105              entry it is a bug in the library since a symbol must
>  106              not simply disappear.
>  107 
>  108              It would also be a bug in the object since it means that
>  109              the list of required versions is incomplete and so the
>  110              tests in dl-version.c haven't found a problem.*/
>  111           assert (version->filename == NULL
>  112                   || ! _dl_name_match_p (version->filename, map));
>  113 
>  114           /* Otherwise we accept the symbol.  */
>
> Why would we assert here?

I think it's a corrupt object, where the symbol uses an index that's
outside the symbol table range.  For example, this code in _dl_fixup
doesn't do any range checks:

      if (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
	{
	  const ElfW(Half) *vernum =
	    (const void *) D_PTR (l, l_info[VERSYMIDX (DT_VERSYM)]);
	  ElfW(Half) ndx = vernum[ELFW(R_SYM) (reloc->r_info)] & 0x7fff;
	  version = &l->l_versions[ndx];
	  if (version->hash == 0)
	    version = NULL;
	}

That can of course segfault, or give you bad data, leading to
segfaults or asserts later.  I believe the assert you quoted would
fire if by accident we got data that looked like a genuine version
reference, thus implying that the soname needs to have version
information.  (dl-version.c should verify that l_versions references
are actually matched by the listed objects, so I don't see how you can
get the assert without some out-of-bounds lookup.)

I don't think we can write a valid test that hits that assert.

> Can we reasonably handle this error?
> If it is the object referenced in verneed then version->filename != NULL
> and so the assert triggers? That seems overly restrictive, and I expect
> we don't get here because earlier dl-version.c checks fail. I'm not asking
> you to cleanup this code, but it does beg the next question.

> What happens if you move the last versioned symbol out of a library?

You need to ensure that the symbol has the required version definition
records.  I think with BFD ld, you may need to keep an arbitrarily
named versioned symbol under that particular version to retain it
under a non-weak version.

> Are you allowed to stop using the version script in the library that
> has zero versioned symbols? I think the answer is "No." Because we
> still check that all libraries provide the version sets that they
> had at link time? Is that still OK? 

Yes, I think that's acceptable.  We might want to fix ld eventually to
avoid the need for dummy symbol definitions, but I don't think that's
immediatelly necessary.

>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +/* Dummy function to add the required symbol version.  */
>> +void
>> +other_function (void)
>> +{
>> +}
>
> OK. Ah, so you do need a dummy function to preserve the required version
> set or we still get another error?

I didn't even think about it and just added the symbol.  I tried now,
and BFD ld starts producing weak symbol definitions, altering behavior
rather drastically, if you don't do that.  We definitely don't want to
do that for glibc because we do not have the link module/run-time
module separation there, and I think it's valuable to align the test
to that (i.e. not have weak version definitions in the run-time
module).

> I expect the answer is "Yes" and that we're not entirely willing to
> remove this requirement. We're happy to move symbols, but what about
> keeping the original symbol set intact? Would we relax that in a
> subsequent patch?

No, I think we should keep that check by default.  It's perhaps less
important now that many binaries use BIND_NOW, but it's the check that
prevents people from running programs with older glibc versions that
do not provide the required symbol set.

We should really add an option to disable the symbol matching in
dl-version.c, so that you can LD_PRELOAD completely new symbol
versions, but that has quite different applications.

Carlos, this is yet another patch review where you write

  Reviewed-by: Carlos O'Donell <carlos@redhat.com>

with a condition that is difficult to fulfill.  (Sometimes, you also
request non-trivial changes which would potentially invalidate the
review.)  I think it's easier for patch submitters if you delay
approval until you are satisfied with the patch submissions and all
your preconditions have been met.
Carlos O'Donell June 27, 2019, 9:57 p.m. UTC | #8
On 6/27/19 5:12 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> I have dropped Ulrich's text here in our wiki:
> 
> Uhm, I don't see a permission notice in the original document.
> Did you ask Ulrich?

I have asked Ulrich for permission to use this text under a license
which would allow us to carry it as documentation for how GNU symbol
versioning is implemented.

I don't have a reply yet.

> I don't think we should block this patch on the outcome of that
> discussion.

I can agree to that if we agree to the need for documentation and
that we'll update it to follow the changes we make.

Agreed?

> (I would definitely prefer a text file in Git, by the way.)

Perfect, let's do that instead then.

I'm removing the wiki page then.

>> An audit of _dl_name_match_p() which is predominantly used for this purpose
>> shows callers in dl-load.c, dl-lookup.c, dl-misc.c, dl-version.c, and rtld.c.
>>
>> I found one questionable assert in dl-lookup.c
>>
>>   99   if (version != NULL)
>>  100     {
>>  101       if (__glibc_unlikely (verstab == NULL))
>>  102         {
>>  103           /* We need a versioned symbol but haven't found any.  If
>>  104              this is the object which is referenced in the verneed
>>  105              entry it is a bug in the library since a symbol must
>>  106              not simply disappear.
>>  107 
>>  108              It would also be a bug in the object since it means that
>>  109              the list of required versions is incomplete and so the
>>  110              tests in dl-version.c haven't found a problem.*/
>>  111           assert (version->filename == NULL
>>  112                   || ! _dl_name_match_p (version->filename, map));
>>  113 
>>  114           /* Otherwise we accept the symbol.  */
>>
>> Why would we assert here?
> 
> I think it's a corrupt object, where the symbol uses an index that's
> outside the symbol table range.  For example, this code in _dl_fixup
> doesn't do any range checks:
> 
>       if (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
> 	{
> 	  const ElfW(Half) *vernum =
> 	    (const void *) D_PTR (l, l_info[VERSYMIDX (DT_VERSYM)]);
> 	  ElfW(Half) ndx = vernum[ELFW(R_SYM) (reloc->r_info)] & 0x7fff;
> 	  version = &l->l_versions[ndx];
> 	  if (version->hash == 0)
> 	    version = NULL;
> 	}
> 
> That can of course segfault, or give you bad data, leading to
> segfaults or asserts later.  I believe the assert you quoted would
> fire if by accident we got data that looked like a genuine version
> reference, thus implying that the soname needs to have version
> information.  (dl-version.c should verify that l_versions references
> are actually matched by the listed objects, so I don't see how you can
> get the assert without some out-of-bounds lookup.)
> 
> I don't think we can write a valid test that hits that assert.

No worries then.

>> Can we reasonably handle this error?
>> If it is the object referenced in verneed then version->filename != NULL
>> and so the assert triggers? That seems overly restrictive, and I expect
>> we don't get here because earlier dl-version.c checks fail. I'm not asking
>> you to cleanup this code, but it does beg the next question.
> 
>> What happens if you move the last versioned symbol out of a library?
> 
> You need to ensure that the symbol has the required version definition
> records.  I think with BFD ld, you may need to keep an arbitrarily
> named versioned symbol under that particular version to retain it
> under a non-weak version.

That's what I thought.

>> Are you allowed to stop using the version script in the library that
>> has zero versioned symbols? I think the answer is "No." Because we
>> still check that all libraries provide the version sets that they
>> had at link time? Is that still OK? 
> 
> Yes, I think that's acceptable.  We might want to fix ld eventually to
> avoid the need for dummy symbol definitions, but I don't think that's
> immediatelly necessary.

Agreed.
 
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <http://www.gnu.org/licenses/>.  */
>>> +
>>> +/* Dummy function to add the required symbol version.  */
>>> +void
>>> +other_function (void)
>>> +{
>>> +}
>>
>> OK. Ah, so you do need a dummy function to preserve the required version
>> set or we still get another error?
> 
> I didn't even think about it and just added the symbol.  I tried now,
> and BFD ld starts producing weak symbol definitions, altering behavior
> rather drastically, if you don't do that.  We definitely don't want to
> do that for glibc because we do not have the link module/run-time
> module separation there, and I think it's valuable to align the test
> to that (i.e. not have weak version definitions in the run-time
> module).

Agreed. What you have is OK.

>> I expect the answer is "Yes" and that we're not entirely willing to
>> remove this requirement. We're happy to move symbols, but what about
>> keeping the original symbol set intact? Would we relax that in a
>> subsequent patch?
> 
> No, I think we should keep that check by default.  It's perhaps less
> important now that many binaries use BIND_NOW, but it's the check that
> prevents people from running programs with older glibc versions that
> do not provide the required symbol set.

Right.

> We should really add an option to disable the symbol matching in
> dl-version.c, so that you can LD_PRELOAD completely new symbol
> versions, but that has quite different applications.

OK.

> Carlos, this is yet another patch review where you write
> 
>   Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> with a condition that is difficult to fulfill.  (Sometimes, you also
> request non-trivial changes which would potentially invalidate the
> review.)  I think it's easier for patch submitters if you delay
> approval until you are satisfied with the patch submissions and all
> your preconditions have been met.

I'm trying to avoid a full-day delay due to review and TZ, and get
the patch through more quickly, therefore I try to provide my preconditions
for accepting the patch.

Your patch is OK for master as-is, and I won't block it on updating
documentation. I expect to update the docs when we have them ready for
committing to git.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Patch
diff mbox series

diff --git a/NEWS b/NEWS
index 8a2fecef47..8cea9f5825 100644
--- a/NEWS
+++ b/NEWS
@@ -34,6 +34,12 @@  Major new features:
   pointer subtraction within the allocated object, where results might
   overflow the ptrdiff_t type.
 
+* The dynamic linker no longer refuses to load objects which reference
+  versioned symbols whose implementation has moved to a different soname
+  since the object has been linked.  The old error message, symbol
+  FUNCTION-NAME, version SYMBOL-VERSION not defined in file DSO-NAME with
+  link time reference, is gone.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The functions clock_gettime, clock_getres, clock_settime,
diff --git a/elf/Makefile b/elf/Makefile
index 27a2fa8c14..76b0565054 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -191,7 +191,8 @@  tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
 	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
 	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
-	 tst-unwind-ctor tst-unwind-main tst-audit13
+	 tst-unwind-ctor tst-unwind-main tst-audit13 \
+	 tst-sonamemove tst-sonamemove-dlopen
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -281,7 +282,8 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
 		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
 		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
-		tst-audit13mod1
+		tst-audit13mod1 tst-sonamemove-linkmod1 \
+		tst-sonamemove-runmod1 tst-sonamemove-runmod2
 # Most modules build with _ISOMAC defined, but those filtered out
 # depend on internal headers.
 modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1410,6 +1412,28 @@  $(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
 LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
 tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
 
+# tst-sonamemove links against an older implementation of the library.
+LDFLAGS-tst-sonamemove-linkmod1.so = \
+  -Wl,--version-script=tst-sonamemove-linkmod1.map \
+  -Wl,-soname,tst-sonamemove-runmod1.so
+LDFLAGS-tst-sonamemove-runmod1.so = -Wl,--no-as-needed \
+  -Wl,--version-script=tst-sonamemove-runmod1.map \
+  -Wl,-soname,tst-sonamemove-runmod1.so
+LDFLAGS-tst-sonamemove-runmod2.so = \
+  -Wl,--version-script=tst-sonamemove-runmod2.map \
+  -Wl,-soname,tst-sonamemove-runmod2.so
+$(objpfx)tst-sonamemove-runmod1.so: $(objpfx)tst-sonamemove-runmod2.so
+# Link against the link module, but depend on the run-time modules
+# for execution.
+$(objpfx)tst-sonamemove: $(objpfx)tst-sonamemove-linkmod1.so
+$(objpfx)tst-sonamemove.out: \
+  $(objpfx)tst-sonamemove-runmod1.so \
+  $(objpfx)tst-sonamemove-runmod2.so
+$(objpfx)tst-sonamemove-dlopen: $(libdl)
+$(objpfx)tst-sonamemove.out: \
+  $(objpfx)tst-sonamemove-runmod1.so \
+  $(objpfx)tst-sonamemove-runmod2.so
+
 # Override -z defs, so that we can reference an undefined symbol.
 # Force lazy binding for the same reason.
 LDFLAGS-tst-latepthreadmod.so = \
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index e3f42a1efb..eb23cca4e3 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -536,11 +536,7 @@  do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
 	}
 
 skip:
-      /* If this current map is the one mentioned in the verneed entry
-	 and we have not found a weak entry, it is a bug.  */
-      if (symidx == STN_UNDEF && version != NULL && version->filename != NULL
-	  && __glibc_unlikely (_dl_name_match_p (version->filename, map)))
-	return -1;
+      ;
     }
   while (++i < n);
 
@@ -810,34 +806,10 @@  _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
 
   /* Search the relevant loaded objects for a definition.  */
   for (size_t start = i; *scope != NULL; start = 0, ++scope)
-    {
-      int res = do_lookup_x (undef_name, new_hash, &old_hash, *ref,
-			     &current_value, *scope, start, version, flags,
-			     skip_map, type_class, undef_map);
-      if (res > 0)
-	break;
-
-      if (__glibc_unlikely (res < 0) && skip_map == NULL)
-	{
-	  /* Oh, oh.  The file named in the relocation entry does not
-	     contain the needed symbol.  This code is never reached
-	     for unversioned lookups.  */
-	  assert (version != NULL);
-	  const char *reference_name = undef_map ? undef_map->l_name : "";
-	  struct dl_exception exception;
-	  /* XXX We cannot translate the message.  */
-	  _dl_exception_create_format
-	    (&exception, DSO_FILENAME (reference_name),
-	     "symbol %s version %s not defined in file %s"
-	     " with link time reference%s",
-	     undef_name, version->name, version->filename,
-	     res == -2 ? " (no version symbols)" : "");
-	  _dl_signal_cexception (0, &exception, N_("relocation error"));
-	  _dl_exception_free (&exception);
-	  *ref = NULL;
-	  return 0;
-	}
-    }
+    if (do_lookup_x (undef_name, new_hash, &old_hash, *ref,
+		     &current_value, *scope, start, version, flags,
+		     skip_map, type_class, undef_map) != 0)
+      break;
 
   if (__glibc_unlikely (current_value.s == NULL))
     {
diff --git a/elf/tst-sonamemove-dlopen.c b/elf/tst-sonamemove-dlopen.c
new file mode 100644
index 0000000000..c496705044
--- /dev/null
+++ b/elf/tst-sonamemove-dlopen.c
@@ -0,0 +1,35 @@ 
+/* Check that a moved versioned symbol can be found using dlsym, dlvsym.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stddef.h>
+#include <support/check.h>
+#include <support/xdlfcn.h>
+
+static int
+do_test (void)
+{
+  /* tst-sonamemove-runmod1.so does not define moved_function, but it
+     depends on tst-sonamemove-runmod2.so, which does.  */
+  void *handle = xdlopen ("tst-sonamemove-runmod1.so", RTLD_NOW);
+  TEST_VERIFY (xdlsym (handle, "moved_function") != NULL);
+  TEST_VERIFY (xdlvsym (handle, "moved_function", "SONAME_MOVE") != NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-sonamemove-linkmod1.c b/elf/tst-sonamemove-linkmod1.c
new file mode 100644
index 0000000000..b8a354e5e3
--- /dev/null
+++ b/elf/tst-sonamemove-linkmod1.c
@@ -0,0 +1,25 @@ 
+/* Link interface for (lack of) soname matching in versioned symbol refs.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This function moved from tst-sonamemove-runmod1.so.  This module is
+   intended for linking only, to simulate an old application which was
+   linked against an older version of the library.  */
+void
+moved_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-linkmod1.map b/elf/tst-sonamemove-linkmod1.map
new file mode 100644
index 0000000000..8fe5904018
--- /dev/null
+++ b/elf/tst-sonamemove-linkmod1.map
@@ -0,0 +1,3 @@ 
+SONAME_MOVE {
+  global: moved_function;
+};
diff --git a/elf/tst-sonamemove-runmod1.c b/elf/tst-sonamemove-runmod1.c
new file mode 100644
index 0000000000..5c409e2289
--- /dev/null
+++ b/elf/tst-sonamemove-runmod1.c
@@ -0,0 +1,23 @@ 
+/* Run-time module whose moved_function moved to a library dependency.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Dummy function to add the required symbol version.  */
+void
+other_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-runmod1.map b/elf/tst-sonamemove-runmod1.map
new file mode 100644
index 0000000000..2ea81c6e6f
--- /dev/null
+++ b/elf/tst-sonamemove-runmod1.map
@@ -0,0 +1,3 @@ 
+SONAME_MOVE {
+  global: other_function;
+};
diff --git a/elf/tst-sonamemove-runmod2.c b/elf/tst-sonamemove-runmod2.c
new file mode 100644
index 0000000000..b5e482eff5
--- /dev/null
+++ b/elf/tst-sonamemove-runmod2.c
@@ -0,0 +1,24 @@ 
+/* Run-time module with the actual implementation of moved_function.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* In the test scenario, this function was originally in
+   tst-sonamemove-runmod1.so.  */
+void
+moved_function (void)
+{
+}
diff --git a/elf/tst-sonamemove-runmod2.map b/elf/tst-sonamemove-runmod2.map
new file mode 100644
index 0000000000..8fe5904018
--- /dev/null
+++ b/elf/tst-sonamemove-runmod2.map
@@ -0,0 +1,3 @@ 
+SONAME_MOVE {
+  global: moved_function;
+};
diff --git a/elf/tst-sonamemove.c b/elf/tst-sonamemove.c
new file mode 100644
index 0000000000..a80ebcb36f
--- /dev/null
+++ b/elf/tst-sonamemove.c
@@ -0,0 +1,30 @@ 
+/* Check that a versioned symbol can move from one library to another.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* moved_function is linked against tst-sonamemove-runmod1.so, but the
+   actual implementation is in tst-sonamemove-runmod1.so. */
+void moved_function (void);
+
+static int
+do_test (void)
+{
+  moved_function ();
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/support/xdlfcn.c b/support/xdlfcn.c
index b2e5c21134..11fe4896d1 100644
--- a/support/xdlfcn.c
+++ b/support/xdlfcn.c
@@ -51,6 +51,26 @@  xdlsym (void *handle, const char *symbol)
   return sym;
 }
 
+void *
+xdlvsym (void *handle, const char *symbol, const char *version)
+{
+  /* Clear any pending errors.  */
+  dlerror ();
+
+  void *sym = dlvsym (handle, symbol, version);
+
+  if (sym == NULL)
+    {
+      const char *error = dlerror ();
+      if (error != NULL)
+        FAIL_EXIT1 ("error: dlvsym: %s\n", error);
+      /* If there was no error, we found a NULL symbol.  Return the
+         NULL value in this case.  */
+    }
+
+  return sym;
+}
+
 void
 xdlclose (void *handle)
 {
diff --git a/support/xdlfcn.h b/support/xdlfcn.h
index c9cd810a8a..7f8d4930fc 100644
--- a/support/xdlfcn.h
+++ b/support/xdlfcn.h
@@ -27,6 +27,7 @@  __BEGIN_DECLS
 void *xdlopen (const char *filename, int flags);
 void *xdlmopen (Lmid_t lmid, const char *filename, int flags);
 void *xdlsym (void *handle, const char *symbol);
+void *xdlvsym (void *handle, const char *symbol, const char *version);
 void xdlclose (void *handle);
 
 __END_DECLS