Bug 84761 - AddressSanitizer is not compatible with glibc 2.27 on x86
Summary: AddressSanitizer is not compatible with glibc 2.27 on x86
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 8.0.1
: P3 normal
Target Milestone: ---
Assignee: Jakub Jelinek
URL:
Keywords:
: 85477 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-08 14:34 UTC by Vincent Lefèvre
Modified: 2018-06-07 11:57 UTC (History)
12 users (show)

See Also:
Host:
Target: i686-linux-gnu
Build:
Known to work: 7.3.1, 8.1.0
Known to fail: 7.3.0
Last reconfirmed: 2018-03-19 00:00:00


Attachments
gcc8-pr84761.patch (1004 bytes, patch)
2018-03-19 13:38 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Lefèvre 2018-03-08 14:34:56 UTC
After an upgrade to glibc 2.27 on a Debian/unstable machine, on a program that does nothing, the AddressSanitizer segfaults with the 32-bit x86 ABI.

I have the following gcc-snapshot script:

#!/bin/sh
LD_LIBRARY_PATH=/usr/lib/gcc-snapshot/lib:$LD_LIBRARY_PATH
PATH=/usr/lib/gcc-snapshot/bin:$PATH
rpath=""
OLD_IFS="$IFS"
IFS=:
for i in $LD_RUN_PATH
do
  rpath="$rpath -Wl,-rpath -Wl,$i"
done
IFS="$OLD_IFS"
exec gcc -Wl,-rpath -Wl,/usr/lib/gcc-snapshot/lib \
         -Wl,-rpath -Wl,/usr/lib/gcc-snapshot/lib32 \
         -Wl,-rpath -Wl,/usr/lib/gcc-snapshot/libx32 $rpath "$@"

cventin:~> gcc-snapshot --version
gcc (Debian 20180216-1) 8.0.1 20180216 (experimental) [trunk revision 257720]
[...]
cventin:~> cat tst.c
int main (void)
{
  return 0;
}
cventin:~> gcc-snapshot -m32 -fsanitize=address tst.c -o tst
cventin:~> ./tst
AddressSanitizer:DEADLYSIGNAL
=================================================================
==25032==ERROR: AddressSanitizer: SEGV on unknown address 0xf7fa7e70 (pc 0xf7fa7e84 bp 0xffbf40ac sp 0xffbf406c T16777215)
==25032==The signal is caused by a WRITE memory access.
    #0 0xf7fa7e83 in _dl_get_tls_static_info (/lib/ld-linux.so.2+0x11e83)
    #1 0xf7ac147d  (/usr/lib/gcc-snapshot/lib32/libasan.so.5+0x10e47d)
    #2 0xf7aafd27  (/usr/lib/gcc-snapshot/lib32/libasan.so.5+0xfcd27)
    #3 0xf7fa591a  (/lib/ld-linux.so.2+0xf91a)
    #4 0xf7f96cb9  (/lib/ld-linux.so.2+0xcb9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/ld-linux.so.2+0x11e83) in _dl_get_tls_static_info
==25032==ABORTING

My original bug report:
  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=892096

The explanations given by Aurelien Jarno:

------------------------------------------------------------
The AddressSanitizer is using glibc internal functions though dlsym(), and such functions have the right to change in new major versions:

From libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:
|  void *get_tls_static_info_ptr = dlsym(RTLD_NEXT, "_dl_get_tls_static_info");

And on the glibc side:
| $ readelf -s /lib/ld-linux.so.2  | grep _dl_get_tls_static_info
|      4: 00011e70    35 FUNC    GLOBAL DEFAULT   12 _dl_get_tls_static_info@@GLIBC_PRIVATE

This has been discussed for example there:
https://www.sourceware.org/ml/libc-alpha/2018-02/msg00611.html

The AddressSanitizer people should discuss for a public API so that it doesn't happen again. Otherwise it might break at every new glibc version.
------------------------------------------------------------

In the mean time, it should at least be made compatible with glibc 2.27.
Comment 1 Florian Weimer 2018-03-19 12:16:43 UTC
This bit needs to change for glibc 2.27 and later:

#ifdef __i386__
# define DL_INTERNAL_FUNCTION __attribute__((regparm(3), stdcall))
#else
# define DL_INTERNAL_FUNCTION
#endif

We removed the regparm attribute for compatibility with CET.  The GCC manual says that regparm (3) is ABI-compatible on i386, but it really is not.

We could perhaps add an alias to newer glibcs (including 2.27), so that libasan can detect that glibc is more recent and call the function with the right ABI.  Or expose the information via dlinfo (but the interface is problematic because it assumes that the static TLS size is fixed—we could eventually switch to a model where we have a larger reserved region for the initial thread).
Comment 2 rguenther@suse.de 2018-03-19 12:27:08 UTC
On Mon, 19 Mar 2018, fw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84761
> 
> --- Comment #1 from Florian Weimer <fw at gcc dot gnu.org> ---
> This bit needs to change for glibc 2.27 and later:
> 
> #ifdef __i386__
> # define DL_INTERNAL_FUNCTION __attribute__((regparm(3), stdcall))
> #else
> # define DL_INTERNAL_FUNCTION
> #endif
> 
> We removed the regparm attribute for compatibility with CET.  The GCC manual
> says that regparm (3) is ABI-compatible on i386, but it really is not.
> 
> We could perhaps add an alias to newer glibcs (including 2.27), so that libasan
> can detect that glibc is more recent and call the function with the right ABI. 
> Or expose the information via dlinfo (but the interface is problematic because
> it assumes that the static TLS size is fixed—we could eventually switch to a
> model where we have a larger reserved region for the initial thread).

I think as there are already quite some __GLIBC_PREREQ uses in the
sanitizer lib changing the above prototype appropriately would be
good enough.

Of course exposing the desired information without the need to resort
to a private interface would be nice.

So sth like

Index: libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
===================================================================
--- libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc    (revision 
258641)
+++ libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc    (working 
copy)
@@ -152,7 +152,14 @@ bool SanitizerGetThreadName(char *name,
 static uptr g_tls_size;
 
 #ifdef __i386__
+#ifndef __GLIBC_PREREQ
+#define __GLIBC_PREREQ(x, y) 0
+#endif
+#ifdef SANITIZER_LINUX && __GLIBC_PREREQ(2, 27)
+# define DL_INTERNAL_FUNCTION
+#else
 # define DL_INTERNAL_FUNCTION __attribute__((regparm(3), stdcall))
+#endif
 #else
 # define DL_INTERNAL_FUNCTION
 #endif
Comment 3 Florian Weimer 2018-03-19 12:48:27 UTC
(In reply to rguenther@suse.de from comment #2)
> I think as there are already quite some __GLIBC_PREREQ uses in the
> sanitizer lib changing the above prototype appropriately would be
> good enough.


If that's the way to go, then your patch should work.  I'd add a comment:

/* glibc 2.27 changed internal ABI and removed internal_function.  */

And perhaps the condition can be simplified to:

#if defined (__i386__) && defined (__GLIBC_PREREQ) && !__GLIBC_PREREQ(2, 27)
Comment 4 Jakub Jelinek 2018-03-19 12:52:48 UTC
That won't really work, if you configure against one glibc version and run against another one, that will still not work properly.
So I think we need something like:
--- libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc	2017-10-19 13:20:58.972958379 +0200
+++ libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc	2018-03-19 13:50:41.978147828 +0100
@@ -151,25 +151,35 @@ bool SanitizerGetThreadName(char *name,
     !SANITIZER_NETBSD
 static uptr g_tls_size;
 
-#ifdef __i386__
-# define DL_INTERNAL_FUNCTION __attribute__((regparm(3), stdcall))
-#else
-# define DL_INTERNAL_FUNCTION
-#endif
-
 void InitTlsSize() {
 // all current supported platforms have 16 bytes stack alignment
   const size_t kStackAlign = 16;
-  typedef void (*get_tls_func)(size_t*, size_t*) DL_INTERNAL_FUNCTION;
-  get_tls_func get_tls;
-  void *get_tls_static_info_ptr = dlsym(RTLD_NEXT, "_dl_get_tls_static_info");
-  CHECK_EQ(sizeof(get_tls), sizeof(get_tls_static_info_ptr));
-  internal_memcpy(&get_tls, &get_tls_static_info_ptr,
-                  sizeof(get_tls_static_info_ptr));
-  CHECK_NE(get_tls, 0);
   size_t tls_size = 0;
   size_t tls_align = 0;
-  get_tls(&tls_size, &tls_align);
+  void *get_tls_static_info_ptr = dlsym(RTLD_NEXT, "_dl_get_tls_static_info");
+#ifdef __i386__
+  /* On i?86, _dl_get_tls_static_info used to be regparm(3) before glibc 2.27
+     and is now a normal function.  */
+  if (!dlvsym(RTLD_NEXT, "glob", "GLIBC_2.27")) {
+    typedef void (*get_tls_func)(size_t*, size_t*)
+      __attribute__((regparm(3), stdcall));
+    get_tls_func get_tls;
+    CHECK_EQ(sizeof(get_tls), sizeof(get_tls_static_info_ptr));
+    internal_memcpy(&get_tls, &get_tls_static_info_ptr,
+                    sizeof(get_tls_static_info_ptr));
+    CHECK_NE(get_tls, 0);
+    get_tls(&tls_size, &tls_align);
+  } else
+#endif
+  {
+    typedef void (*get_tls_func)(size_t*, size_t*);
+    get_tls_func get_tls;
+    CHECK_EQ(sizeof(get_tls), sizeof(get_tls_static_info_ptr));
+    internal_memcpy(&get_tls, &get_tls_static_info_ptr,
+                    sizeof(get_tls_static_info_ptr));
+    CHECK_NE(get_tls, 0);
+    get_tls(&tls_size, &tls_align);
+  }
   if (tls_align < kStackAlign)
     tls_align = kStackAlign;
   g_tls_size = RoundUpTo(tls_size, tls_align);

instead.  If we only support using the glibc with which gcc has been configured or later, perhaps that dlvsym call could be avoided if __GLIBC_PREREQ(2, 27).
Comment 5 Jakub Jelinek 2018-03-19 13:38:46 UTC
Created attachment 43701 [details]
gcc8-pr84761.patch

Full untested patch, tested so far just with building against glibc 2.26 and running a simple 32-bit -fsanitize=address sanitized program against:
1) unpatched libasan.so.5 and glibc 2.26
2) unpatched libasan.so.5 and glibc 2.27 (this crashes)
3) patched libasan.so.5 and glibc 2.26
4) patched libasan.so.5 and glibc 2.27
Comment 6 Jakub Jelinek 2018-03-19 13:50:34 UTC
Filed https://reviews.llvm.org/D44623 upstream.
Comment 7 Alexander Monakov 2018-03-19 13:51:50 UTC
Is it possible that a distribution would backport glob() changes together with its symver update (without also backporting the regparm change)? In that case the dlvsym check shown above will be wrong I think.

Would the approach with confstr query for glibc version (discussed on irc) be less fragile?
Comment 8 Jakub Jelinek 2018-03-19 14:03:49 UTC
Distributions should never backport symbol additions to symbol versioned libraries, unless they backport all the corresponding changes.
So no, I don't think this is an issue.
Comment 9 Jakub Jelinek 2018-03-19 20:48:01 UTC
Author: jakub
Date: Mon Mar 19 20:47:29 2018
New Revision: 258663

URL: https://gcc.gnu.org/viewcvs?rev=258663&root=gcc&view=rev
Log:
	PR sanitizer/84761
	* sanitizer_common/sanitizer_linux_libcdep.cc (__GLIBC_PREREQ):
	Define if not defined.
	(DL_INTERNAL_FUNCTION): Don't define.
	(InitTlsSize): For __i386__ if not compiled against glibc 2.27+
	determine at runtime whether to use regparm(3), stdcall calling
	convention for older glibcs or normal calling convention for
	newer glibcs for call to _dl_get_tls_static_info.

Modified:
    trunk/libsanitizer/ChangeLog
    trunk/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
Comment 10 Jakub Jelinek 2018-03-19 20:50:51 UTC
Fixed for 8+ so far.
Comment 11 Martin Liška 2018-04-20 08:53:14 UTC
*** Bug 85477 has been marked as a duplicate of this bug. ***
Comment 12 Peter Wu 2018-05-08 22:51:06 UTC
I was not aware of this report or existing patches. The issue is reported in upstream compiler-rt here: https://github.com/google/sanitizers/issues/954

Jakub, according to upstream LLVM the compiler-rt fixes should first enter LLVM and then be merged downstream in GCC, is there any reason to deviate from this process? Upstream does not seem happy about it.

Also please see the LLVM review and linked bugs for comments (I think that using confstr is better than dlvsym, nobody uses pre-release glibc for production right?).
Comment 13 Richard Biener 2018-06-07 11:56:57 UTC
Author: rguenth
Date: Thu Jun  7 11:56:25 2018
New Revision: 261272

URL: https://gcc.gnu.org/viewcvs?rev=261272&root=gcc&view=rev
Log:
2018-06-07  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2018-03-19  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/84761
	* sanitizer_common/sanitizer_linux_libcdep.cc (__GLIBC_PREREQ):
	Define if not defined.
	(DL_INTERNAL_FUNCTION): Don't define.
	(InitTlsSize): For __i386__ if not compiled against glibc 2.27+
	determine at runtime whether to use regparm(3), stdcall calling
	convention for older glibcs or normal calling convention for
	newer glibcs for call to _dl_get_tls_static_info.

Modified:
    branches/gcc-7-branch/libsanitizer/ChangeLog
    branches/gcc-7-branch/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
Comment 14 Richard Biener 2018-06-07 11:57:45 UTC
Fixed on the GCC 7 branch as well.