diff mbox

Don't use IFUNC resolver for longjmp or system in libpthread (bug 21041)

Message ID mvmwp6em7uq.fsf@suse.de
State New
Headers show

Commit Message

Andreas Schwab Aug. 8, 2017, 2:50 p.m. UTC 
Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
there won't be a problem when the compiler does not turn this into a tail
call.

Andreas.

	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
	* nptl/pt-system.c (system): Likewise.

Comments

Florian Weimer Aug. 8, 2017, 2:55 p.m. UTC | #1 
On 08/08/2017 04:50 PM, Andreas Schwab wrote:
> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
> there won't be a problem when the compiler does not turn this into a tail
> call.
> 
> Andreas.
> 
> 	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
> 	* nptl/pt-system.c (system): Likewise.

Looks good to me.

Thanks,
Florian
H.J. Lu Aug. 8, 2017, 2:57 p.m. UTC | #2 
On Tue, Aug 8, 2017 at 7:50 AM, Andreas Schwab <schwab@suse.de> wrote:
> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
> there won't be a problem when the compiler does not turn this into a tail
> call.
>
> Andreas.
>
>         * nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
>         * nptl/pt-system.c (system): Likewise.


Missing BZ #.

Can you add a testcase from

https://sourceware.org/bugzilla/show_bug.cgi?id=21041


> diff --git a/nptl/pt-longjmp.c b/nptl/pt-longjmp.c
> index 2ef757e687..8f3c6b3a09 100644
> --- a/nptl/pt-longjmp.c
> +++ b/nptl/pt-longjmp.c
> @@ -25,21 +25,14 @@
>     symbol in libpthread, but the historical ABI requires it.  For static
>     linking, there is no need to provide anything here--the libc version
>     will be linked in.  For shared library ABI compatibility, there must be
> -   longjmp and siglongjmp symbols in libpthread.so; so we define them using
> -   IFUNC to redirect to the libc function.  */
> +   longjmp and siglongjmp symbols in libpthread.so.
>
> -#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
> -
> -# if HAVE_IFUNC
> -
> -#  undef INIT_ARCH
> -#  define INIT_ARCH()
> -#  define DEFINE_LONGJMP(name) libc_ifunc (name, &__libc_longjmp)
> -
> -extern __typeof(longjmp) longjmp_ifunc;
> -extern __typeof(siglongjmp) siglongjmp_ifunc;
> +   With an IFUNC resolver, it would be possible to avoid the indirection,
> +   but the IFUNC resolver might run before the __libc_longjmp symbol has
> +   been relocated, in which case the IFUNC resolver would not be able to
> +   provide the correct address.  */
>
> -# else  /* !HAVE_IFUNC */
> +#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
>
>  static void __attribute__ ((noreturn, used))
>  longjmp_compat (jmp_buf env, int val)
> @@ -47,14 +40,10 @@ longjmp_compat (jmp_buf env, int val)
>    __libc_longjmp (env, val);
>  }
>
> -# define DEFINE_LONGJMP(name) strong_alias (longjmp_compat, name)
> -
> -# endif  /* HAVE_IFUNC */
> -
> -DEFINE_LONGJMP (longjmp_ifunc)
> -compat_symbol (libpthread, longjmp_ifunc, longjmp, GLIBC_2_0);
> +strong_alias (longjmp_compat, longjmp_alias)
> +compat_symbol (libpthread, longjmp_alias, longjmp, GLIBC_2_0);
>
> -strong_alias (longjmp_ifunc, siglongjmp_ifunc)
> -compat_symbol (libpthread, siglongjmp_ifunc, siglongjmp, GLIBC_2_0);
> +strong_alias (longjmp_alias, siglongjmp_alias)
> +compat_symbol (libpthread, siglongjmp_alias, siglongjmp, GLIBC_2_0);
>
>  #endif
> diff --git a/nptl/pt-system.c b/nptl/pt-system.c
> index f8ca6ba0d9..b30ddf2b39 100644
> --- a/nptl/pt-system.c
> +++ b/nptl/pt-system.c
> @@ -25,29 +25,21 @@
>     libpthread, but the historical ABI requires it.  For static linking,
>     there is no need to provide anything here--the libc version will be
>     linked in.  For shared library ABI compatibility, there must be a
> -   'system' symbol in libpthread.so; so we define it using IFUNC to
> -   redirect to the libc function.  */
> +   'system' symbol in libpthread.so.
>
> -#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
> -
> -# if HAVE_IFUNC
> -
> -extern __typeof(system) system_ifunc;
> -#  undef INIT_ARCH
> -#  define INIT_ARCH()
> -libc_ifunc (system_ifunc, &__libc_system)
> +   With an IFUNC resolver, it would be possible to avoid the indirection,
> +   but the IFUNC resolver might run before the __libc_system symbol has
> +   been relocated, in which case the IFUNC resolver would not be able to
> +   provide the correct address.  */
>
> -# else  /* !HAVE_IFUNC */
> +#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
>
>  static int __attribute__ ((used))
>  system_compat (const char *line)
>  {
>    return __libc_system (line);
>  }
> -strong_alias (system_compat, system_ifunc)
> -
> -# endif  /* HAVE_IFUNC */
> -
> -compat_symbol (libpthread, system_ifunc, system, GLIBC_2_0);
> +strong_alias (system_compat, system_alias)
> +compat_symbol (libpthread, system_alias, system, GLIBC_2_0);
>
>  #endif
> --
> 2.14.0
>
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Joseph Myers Aug. 8, 2017, 6:09 p.m. UTC | #3 
I suspect this of having caused s390 build failures.

s390x-glibc-linux-gnu-gcc -m31 
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c -c -std=gnu11 -fgnu89-inline  
-O2 -Wall -Werror -Wundef -Wwrite-strings -fmerge-all-constants 
-fno-stack-protector -frounding-math -g -Wstrict-prototypes 
-Wold-style-definition -mlong-double-128  -fPIC   -ftls-model=initial-exec      
-I../include 
-I/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl  
-I/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc  
-I../sysdeps/unix/sysv/linux/s390/s390-32  
-I../sysdeps/unix/sysv/linux/s390/fpu  -I../sysdeps/s390/fpu  
-I../sysdeps/unix/sysv/linux/s390  -I../sysdeps/s390/nptl  
-I../sysdeps/ieee754/ldbl-64-128  -I../sysdeps/ieee754/ldbl-opt  
-I../sysdeps/unix/sysv/linux/include -I../sysdeps/unix/sysv/linux  
-I../sysdeps/nptl  -I../sysdeps/pthread  -I../sysdeps/gnu  
-I../sysdeps/unix/inet  -I../sysdeps/unix/sysv  -I../sysdeps/unix  
-I../sysdeps/posix  -I../sysdeps/s390/s390-32/multiarch  
-I../sysdeps/s390/s390-32  -I../sysdeps/wordsize-32  
-I../sysdeps/s390/multiarch  -I../sysdeps/s390  
-I../sysdeps/ieee754/ldbl-128  -I../sysdeps/ieee754/dbl-64  
-I../sysdeps/ieee754/flt-32  -I../sysdeps/ieee754  -I../sysdeps/generic  
-I.. -I../libio -I.   -D_LIBC_REENTRANT -include 
/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/libc-modules.h 
-DMODULE_NAME=libpthread -include ../include/libc-symbols.h  -DPIC 
-DSHARED     -DTOP_NAMESPACE=glibc -o 
/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl/pt-longjmp.os 
-MD -MP -MF 
/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl/pt-longjmp.os.dt 
-MT 
/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl/pt-longjmp.os
In file included from <command-line>:0:0:
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:29:15: error: 'longjmp_ifunc' 
undeclared here (not in a function); did you mean 'longjmp_alias'?
 strong_alias (longjmp_ifunc, __v2longjmp)
               ^
./../include/libc-symbols.h:127:20: note: in definition of macro 
'_strong_alias'
   extern __typeof (name) aliasname __attribute__ ((alias (#name)));
                    ^~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:29:1: note: in expansion of 
macro 'strong_alias'
 strong_alias (longjmp_ifunc, __v2longjmp)
 ^~~~~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:15: error: 
'siglongjmp_ifunc' undeclared here (not in a function); did you mean 
'longjmp_ifunc'?
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
               ^
./../include/libc-symbols.h:127:20: note: in definition of macro 
'_strong_alias'
   extern __typeof (name) aliasname __attribute__ ((alias (#name)));
                    ^~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:1: note: in expansion of 
macro 'strong_alias'
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
 ^~~~~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:33: error: 
'__v2siglongjmp' aliased to undefined symbol 'siglongjmp_ifunc'
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
                                 ^
./../include/libc-symbols.h:127:20: note: in definition of macro 
'_strong_alias'
   extern __typeof (name) aliasname __attribute__ ((alias (#name)));
                    ^~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:1: note: in expansion of 
macro 'strong_alias'
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
 ^~~~~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:33: error: 
'__v2siglongjmp' aliased to undefined symbol 'siglongjmp_ifunc'
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
                                 ^
./../include/libc-symbols.h:127:26: note: in definition of macro 
'_strong_alias'
   extern __typeof (name) aliasname __attribute__ ((alias (#name)));
                          ^~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:31:1: note: in expansion of 
macro 'strong_alias'
 strong_alias (siglongjmp_ifunc, __v2siglongjmp)
 ^~~~~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:29:30: error: '__v2longjmp' 
aliased to undefined symbol 'longjmp_ifunc'
 strong_alias (longjmp_ifunc, __v2longjmp)
                              ^
./../include/libc-symbols.h:127:26: note: in definition of macro 
'_strong_alias'
   extern __typeof (name) aliasname __attribute__ ((alias (#name)));
                          ^~~~~~~~~
../sysdeps/unix/sysv/linux/s390/pt-longjmp.c:29:1: note: in expansion of 
macro 'strong_alias'
 strong_alias (longjmp_ifunc, __v2longjmp)
 ^~~~~~~~~~~~
/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/sysd-rules:151: 
recipe for target 
'/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl/pt-longjmp.os' 
failed
make[3]: *** 
[/scratch/jmyers/glibc-bot/build/glibcs/s390-linux-gnu/glibc/nptl/pt-longjmp.os] 
Error 1
Andreas Schwab Aug. 9, 2017, 8:55 a.m. UTC | #4 
Sorry, fixed now.

Andreas.
Romain Naour Nov. 1, 2017, 1:10 p.m. UTC | #5 
Hi All,

Le 08/08/2017 à 16:50, Andreas Schwab a écrit :
> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
> there won't be a problem when the compiler does not turn this into a tail
> call.
> 
> Andreas.
> 
> 	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
> 	* nptl/pt-system.c (system): Likewise.
> 

What's the status of this issue ?
This patch has been merged in master (upcoming 2.27) but the ticket is still
"NEW" [1].

The issue has been reported on the Buildroot mailing list by a user using glibc
2.25. I'm able to reproduce it using glibc 2.26-73.

It's safe to backport this patch to glibc 2.25 and 2.26 stable branches ?

Best regards,
Romain

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=21041
[2] http://lists.busybox.net/pipermail/buildroot/2017-October/205719.html
Andreas Schwab Nov. 6, 2017, 8:48 a.m. UTC | #6 
On Nov 01 2017, Romain Naour <romain.naour@gmail.com> wrote:

> Hi All,
>
> Le 08/08/2017 à 16:50, Andreas Schwab a écrit :
>> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
>> there won't be a problem when the compiler does not turn this into a tail
>> call.
>> 
>> Andreas.
>> 
>> 	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
>> 	* nptl/pt-system.c (system): Likewise.
>> 
>
> What's the status of this issue ?
> This patch has been merged in master (upcoming 2.27) but the ticket is still
> "NEW" [1].

The bug is about moving a symbol between libraries, as such still
unresolved.

> It's safe to backport this patch to glibc 2.25 and 2.26 stable branches ?

You can try.

Andreas.
Bartłomiej Piotrowski Nov. 6, 2017, 1:30 p.m. UTC | #7 
On 2017-11-06 09:48, Andreas Schwab wrote:
> On Nov 01 2017, Romain Naour <romain.naour@gmail.com> wrote:
> 
>> Hi All,
>>
>> Le 08/08/2017 à 16:50, Andreas Schwab a écrit :
>>> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
>>> there won't be a problem when the compiler does not turn this into a tail
>>> call.
>>>
>>> Andreas.
>>>
>>> 	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
>>> 	* nptl/pt-system.c (system): Likewise.
>>>
>>
>> What's the status of this issue ?
>> This patch has been merged in master (upcoming 2.27) but the ticket is still
>> "NEW" [1].
> 
> The bug is about moving a symbol between libraries, as such still
> unresolved.
> 
>> It's safe to backport this patch to glibc 2.25 and 2.26 stable branches ?
> 
> You can try.
> 
> Andreas.
> 

I backported this to 2.26 in Arch some time ago. Looks fine on my end.

Bartłomiej
Romain Naour Nov. 6, 2017, 9:12 p.m. UTC | #8 
Hi Andreas, Bartłomiej,

Le 06/11/2017 à 14:30, Bartłomiej Piotrowski a écrit :
> On 2017-11-06 09:48, Andreas Schwab wrote:
>> On Nov 01 2017, Romain Naour <romain.naour@gmail.com> wrote:
>>
>>> Hi All,
>>>
>>> Le 08/08/2017 à 16:50, Andreas Schwab a écrit :
>>>> Unlike the vfork forwarder and like the fork forwarder as in bug 19861,
>>>> there won't be a problem when the compiler does not turn this into a tail
>>>> call.
>>>>
>>>> Andreas.
>>>>
>>>> 	* nptl/pt-longjmp.c (longjmp, siglongjmp): Don't use IFUNC resolver.
>>>> 	* nptl/pt-system.c (system): Likewise.
>>>>
>>>
>>> What's the status of this issue ?
>>> This patch has been merged in master (upcoming 2.27) but the ticket is still
>>> "NEW" [1].
>>
>> The bug is about moving a symbol between libraries, as such still
>> unresolved.
>>
>>> It's safe to backport this patch to glibc 2.25 and 2.26 stable branches ?
>>
>> You can try.
>>
>> Andreas.
>>
> 
> I backported this to 2.26 in Arch some time ago. Looks fine on my end.

Thank you both for the feedback.

I did a runtime test with ffmpeg [1] and the "Relink" message is gone.
No regression so far.

[1] http://lists.busybox.net/pipermail/buildroot/2017-October/205865.html

Best regards,
Romain

> 
> Bartłomiej
>
diff mbox

Patch

diff --git a/nptl/pt-longjmp.c b/nptl/pt-longjmp.c
index 2ef757e687..8f3c6b3a09 100644
--- a/nptl/pt-longjmp.c
+++ b/nptl/pt-longjmp.c
@@ -25,21 +25,14 @@ 
    symbol in libpthread, but the historical ABI requires it.  For static
    linking, there is no need to provide anything here--the libc version
    will be linked in.  For shared library ABI compatibility, there must be
-   longjmp and siglongjmp symbols in libpthread.so; so we define them using
-   IFUNC to redirect to the libc function.  */
+   longjmp and siglongjmp symbols in libpthread.so.
 
-#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
-
-# if HAVE_IFUNC
-
-#  undef INIT_ARCH
-#  define INIT_ARCH()
-#  define DEFINE_LONGJMP(name) libc_ifunc (name, &__libc_longjmp)
-
-extern __typeof(longjmp) longjmp_ifunc;
-extern __typeof(siglongjmp) siglongjmp_ifunc;
+   With an IFUNC resolver, it would be possible to avoid the indirection,
+   but the IFUNC resolver might run before the __libc_longjmp symbol has
+   been relocated, in which case the IFUNC resolver would not be able to
+   provide the correct address.  */
 
-# else  /* !HAVE_IFUNC */
+#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
 
 static void __attribute__ ((noreturn, used))
 longjmp_compat (jmp_buf env, int val)
@@ -47,14 +40,10 @@  longjmp_compat (jmp_buf env, int val)
   __libc_longjmp (env, val);
 }
 
-# define DEFINE_LONGJMP(name) strong_alias (longjmp_compat, name)
-
-# endif  /* HAVE_IFUNC */
-
-DEFINE_LONGJMP (longjmp_ifunc)
-compat_symbol (libpthread, longjmp_ifunc, longjmp, GLIBC_2_0);
+strong_alias (longjmp_compat, longjmp_alias)
+compat_symbol (libpthread, longjmp_alias, longjmp, GLIBC_2_0);
 
-strong_alias (longjmp_ifunc, siglongjmp_ifunc)
-compat_symbol (libpthread, siglongjmp_ifunc, siglongjmp, GLIBC_2_0);
+strong_alias (longjmp_alias, siglongjmp_alias)
+compat_symbol (libpthread, siglongjmp_alias, siglongjmp, GLIBC_2_0);
 
 #endif
diff --git a/nptl/pt-system.c b/nptl/pt-system.c
index f8ca6ba0d9..b30ddf2b39 100644
--- a/nptl/pt-system.c
+++ b/nptl/pt-system.c
@@ -25,29 +25,21 @@ 
    libpthread, but the historical ABI requires it.  For static linking,
    there is no need to provide anything here--the libc version will be
    linked in.  For shared library ABI compatibility, there must be a
-   'system' symbol in libpthread.so; so we define it using IFUNC to
-   redirect to the libc function.  */
+   'system' symbol in libpthread.so.
 
-#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
-
-# if HAVE_IFUNC
-
-extern __typeof(system) system_ifunc;
-#  undef INIT_ARCH
-#  define INIT_ARCH()
-libc_ifunc (system_ifunc, &__libc_system)
+   With an IFUNC resolver, it would be possible to avoid the indirection,
+   but the IFUNC resolver might run before the __libc_system symbol has
+   been relocated, in which case the IFUNC resolver would not be able to
+   provide the correct address.  */
 
-# else  /* !HAVE_IFUNC */
+#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22)
 
 static int __attribute__ ((used))
 system_compat (const char *line)
 {
   return __libc_system (line);
 }
-strong_alias (system_compat, system_ifunc)
-
-# endif  /* HAVE_IFUNC */
-
-compat_symbol (libpthread, system_ifunc, system, GLIBC_2_0);
+strong_alias (system_compat, system_alias)
+compat_symbol (libpthread, system_alias, system, GLIBC_2_0);
 
 #endif