Bug 24967 - jemalloc static linking causes runtime failure
Summary: jemalloc static linking causes runtime failure
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.30
: P2 normal
Target Milestone: 2.31
Assignee: Adhemerval Zanella
URL:
Keywords:
: 25413 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-09-05 03:09 UTC by Jiangning Liu
Modified: 2020-01-17 19:05 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2019-09-05 00:00:00
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jiangning Liu 2019-09-05 03:09:37 UTC
Since commit 979cfed05d0ee5a9d81d310ea1eb2d590739e36b on trunk, the case below crashed with jemalloc static linked.

$ gcc --version
gcc_new (GCC) 10.0.0 20190903 (experimental)
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc -static ./test_je.c /home/amptest/jemalloc/jemalloc-5.2.0/install_master/lib/libjemalloc.a -pthread -ldl -Wl,-Map,xx.map
$ ./a.out
Segmentation fault (core dumped)
$ cat test_je.c 
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main()
{
        size_t i;
        void *ptr;
        unsigned long long s = 0;

        for (i=0; i<1024*1024; i++) {
                ptr = malloc(i);
                if (!ptr) {
                        printf("Failed to allocated memory!\n");
                        return -1;
                }
                s += (unsigned long long)ptr;
                memset(ptr, 0, i);
                free(ptr);
        }

        printf("%llu\n", s);
}
Comment 1 Florian Weimer 2019-09-05 06:52:18 UTC
Does this happen on all architectures? Do you have a backtrace? Thanks.
Comment 2 Jiangning Liu 2019-09-05 06:55:36 UTC
This is the backtrace on aarch64,

(gdb) run
Starting program: /home/amptest/test/a.out 

Program received signal SIGBUS, Bus error.
0x03c01f329c6e4ce3 in ?? ()
(gdb) bt
#0  0x03c01f329c6e4ce3 in ?? ()
#1  0x0000000000478588 in __clock_gettime (clock_id=clock_id@entry=6, tp=0xffffffffb578, tp@entry=0xffffffffb598) at ../sysdeps/unix/clock_gettime.c:115
#2  0x0000000000439338 in nstime_get (time=0x400000206108) at src/nstime.c:119
#3  nstime_update_impl (time=0x400000206108) at src/nstime.c:160
#4  0x000000000041173c in arena_decay_reinit (decay_ms=<optimized out>, decay=0x400000206078) at src/arena.c:660
#5  arena_decay_init (stats=0x4000002009b0, decay_ms=<optimized out>, decay=0x400000206078) at src/arena.c:681
#6  je_arena_new (tsdn=tsdn@entry=0x0, ind=ind@entry=0, extent_hooks=extent_hooks@entry=0x4e4ca8 <je_extent_hooks_default>) at src/arena.c:2011
#7  0x0000000000401f08 in arena_init_locked (extent_hooks=0x4e4ca8 <je_extent_hooks_default>, ind=0, tsdn=0x0) at src/jemalloc.c:337
#8  je_arena_init (tsdn=tsdn@entry=0x0, ind=ind@entry=0, extent_hooks=0x4e4ca8 <je_extent_hooks_default>) at src/jemalloc.c:365
#9  0x0000000000402bcc in malloc_init_hard_a0_locked () at src/jemalloc.c:1485
#10 0x0000000000403848 in malloc_init_hard () at src/jemalloc.c:1682
#11 0x0000000000406a00 in malloc_init () at src/jemalloc.c:220
#12 imalloc_init_check (dopts=<synthetic pointer>, sopts=<synthetic pointer>) at src/jemalloc.c:2151
#13 imalloc (dopts=<synthetic pointer>, sopts=<synthetic pointer>) at src/jemalloc.c:2182
#14 je_malloc_default (size=20) at src/jemalloc.c:2211
#15 0x0000000000406dd4 in malloc (size=size@entry=20) at src/jemalloc.c:2310
#16 0x00000000004804fc in _dl_get_origin () at ../sysdeps/unix/sysv/linux/generic/dl-origin.c:51
#17 0x0000000000483a98 in _dl_non_dynamic_init () at dl-support.c:313
#18 0x0000000000485318 in __libc_init_first (argc=1, argc@entry=0, argv=0xfffffffff528, argv@entry=0x0, envp=0xfffffffff538) at ../csu/init-first.c:74
#19 0x0000000000463778 in __libc_start_main (main=0x0, argc=0, argv=0x0, init=0x463c78 <__libc_csu_init>, fini=0x463d38 <__libc_csu_fini>, rtld_fini=0x0, stack_end=<optimized out>) at ../csu/libc-start.c:244
#20 0x0000000000400340 in _start () at ../sysdeps/aarch64/start.S:92
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Comment 3 Florian Weimer 2019-09-05 07:38:10 UTC
_dl_get_origin calls malloc, which in the case of jemalloc calls clock_gettime, and the pointers to vDSO functions have not been initialized yet at this point.

We may need a minimal malloc for the static linking case as well and call that until the binary has fully relocated itself.
Comment 4 Szabolcs Nagy 2019-09-05 09:12:20 UTC
i think it also works if the vdso pointers are static initialized to syscall wrappers and instead of using pointer mangling put them in relro so readonly protected after vdso setup is done (is there relro with static linking?).

then the pointers work during early startup (just slower).
Comment 5 Adhemerval Zanella 2019-09-05 13:50:21 UTC
(In reply to Florian Weimer from comment #3)
> _dl_get_origin calls malloc, which in the case of jemalloc calls
> clock_gettime, and the pointers to vDSO functions have not been initialized
> yet at this point.
> 
> We may need a minimal malloc for the static linking case as well and call
> that until the binary has fully relocated itself.

The {INTERNAL,INLINE}_VSYSCALL macros should detect if the value was not initialized and issue the syscall directly using INTERNAL_SYSCALL macro. The issue is the PTR_DEMANGLE is applied *before* checking the __vdso_* value, so an uninitialized __vdso_* will never be null after the demangle and in turn, the VSYSCALL macro will try to branch to a bogus address.

A straightforward fix would be to demangle the __vdso_* after iff __vdso_* is not NULL as

--
diff --git a/sysdeps/unix/sysv/linux/sysdep-vdso.h b/sysdeps/unix/sysv/linux/sysdep-vdso.h
index 5fec208380..67d380d2c0 100644
--- a/sysdeps/unix/sysv/linux/sysdep-vdso.h
+++ b/sysdeps/unix/sysv/linux/sysdep-vdso.h
@@ -38,9 +38,9 @@
     long int sc_ret;                                                         \
                                                                              \
     __typeof (__vdso_##name) vdsop = __vdso_##name;                          \
-    PTR_DEMANGLE (vdsop);                                                    \
     if (vdsop != NULL)                                                       \
       {                                                                              \
+        PTR_DEMANGLE (vdsop);                                                \
        sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, sc_err, nr, ##args);          \
        if (!INTERNAL_SYSCALL_ERROR_P (sc_ret, sc_err))                       \
          goto out;                                                           \
@@ -65,9 +65,9 @@
     long v_ret;                                                                      \
                                                                              \
     __typeof (__vdso_##name) vdsop = __vdso_##name;                          \
-    PTR_DEMANGLE (vdsop);                                                    \
     if (vdsop != NULL)                                                       \
       {                                                                              \
+        PTR_DEMANGLE (vdsop);                                                \
        v_ret = INTERNAL_VSYSCALL_CALL (vdsop, err, nr, ##args);              \
        if (!INTERNAL_SYSCALL_ERROR_P (v_ret, err)                            \
--

So for uninitialized cases, the syscall will be issued, once vDSO internal pointers are properly initialized, the vDSO will be used instead.

There is one caveat, which are the iFUNC optimizations on some architecture that bypass the {INTERNAL,INLINE}_VSYSCALL. Calling gettimeofday on x86, powerpc, and aarch64 and/or time on x86 and powerpc64 in this scenario will always trigger the syscall path because on iFUNC resolution _dl_vdso_vsym can't resolve the vDSO symbols. A way to enable vDSO support even on these cases is by using INLINE_VSYSCALL on fallback path (which require adding the __vdso_* on some architectures, x86 for instance).
Comment 6 Florian Weimer 2019-09-05 13:55:43 UTC
(In reply to Adhemerval Zanella from comment #5)
> (In reply to Florian Weimer from comment #3)
> > _dl_get_origin calls malloc, which in the case of jemalloc calls
> > clock_gettime, and the pointers to vDSO functions have not been initialized
> > yet at this point.
> > 
> > We may need a minimal malloc for the static linking case as well and call
> > that until the binary has fully relocated itself.
> 
> The {INTERNAL,INLINE}_VSYSCALL macros should detect if the value was not
> initialized and issue the syscall directly using INTERNAL_SYSCALL macro. The
> issue is the PTR_DEMANGLE is applied *before* checking the __vdso_* value,
> so an uninitialized __vdso_* will never be null after the demangle and in
> turn, the VSYSCALL macro will try to branch to a bogus address.
> 
> A straightforward fix would be to demangle the __vdso_* after iff __vdso_*
> is not NULL as

Sorry, I don't think that's correct.  The NULL check must come after demangling, otherwise it will sporadically deliver incorrect results.
Comment 7 Andreas Schwab 2019-09-05 13:58:05 UTC
You also have to fix the various _libc_vdso_platform_setup functions to not mangle a NULL pointer.
Comment 8 Florian Weimer 2019-09-05 14:01:41 UTC
You also need to make sure that mangling a function pointer never produces a null pointer.
Comment 9 Adhemerval Zanella 2019-09-05 14:28:50 UTC
(In reply to Andreas Schwab from comment #7)
> You also have to fix the various _libc_vdso_platform_setup functions to not
> mangle a NULL pointer.

(In reply to Florian Weimer from comment #6)
> (In reply to Adhemerval Zanella from comment #5)
> > (In reply to Florian Weimer from comment #3)
> > > _dl_get_origin calls malloc, which in the case of jemalloc calls
> > > clock_gettime, and the pointers to vDSO functions have not been initialized
> > > yet at this point.
> > > 
> > > We may need a minimal malloc for the static linking case as well and call
> > > that until the binary has fully relocated itself.
> > 
> > The {INTERNAL,INLINE}_VSYSCALL macros should detect if the value was not
> > initialized and issue the syscall directly using INTERNAL_SYSCALL macro. The
> > issue is the PTR_DEMANGLE is applied *before* checking the __vdso_* value,
> > so an uninitialized __vdso_* will never be null after the demangle and in
> > turn, the VSYSCALL macro will try to branch to a bogus address.
> > 
> > A straightforward fix would be to demangle the __vdso_* after iff __vdso_*
> > is not NULL as
> 
> Sorry, I don't think that's correct.  The NULL check must come after
> demangling, otherwise it will sporadically deliver incorrect results.

Not in the scenario where PTR_DEMANGLE is called *before* PTR_MANGLE.  However a valid mangled NULL pointer will be created if both the guard and the pointer are equal, so although unlikely it might occur.

So, another option would be to create another architecture hook that will setup the __vdso_ mangled values to the fallback syscall initially and once _libc_vdso_platform_setup is called it will setup the expected vDSO values. Something like:

--
extern __typeof (clock_gettime) __syscall_clock_gettime attribute_hidden;

_libc_vdso_platform_pre_setup (void)
  [...]
  p = __syscall_clock_gettime;
  PTR_MANGLE (p);
  VDSO_SYMBOL(clock_gettime) = p;
  [...]

#define VDSO_PRE_SETUP _libc_vdso_platform_pre_setup
--

Then it is called prior _dl_non_dynamic_init on csu/init-first.c:

--
#ifdef VDSO_PRE_SETUP
  VDSO_PRE_SETUP ();
#endif

#ifndef SHARED
  /* First the initialization which normally would be done by the
     dynamic linker.  */
  _dl_non_dynamic_init ();
#endif

#ifdef VDSO_SETUP
  VDSO_SETUP ();
#endif
--
  
It will require each architecture that supports vDSO to export also __syscall_clock_gettime, as x86_64 does.
Comment 10 Andreas Schwab 2019-09-05 14:50:53 UTC
You could just initialize the vdso pointers with a mangled NULL in the pre setup.
Comment 11 Adhemerval Zanella 2019-09-05 20:01:00 UTC
(In reply to Andreas Schwab from comment #10)
> You could just initialize the vdso pointers with a mangled NULL in the pre
> setup.

Yeah, this seems the simpler solution. I have implemented it and to avoid all the boilerplate I have used as base my vDSO refactor patchset, so I just need to fix it at one place instead of all the init-first.c implementations.

I will send it upstream to libc-alpha.
Comment 12 Adhemerval Zanella 2020-01-03 14:43:06 UTC
Fixed on 2.31 (commit 1bdda52fe92fd01b424cd6fbb63e3df96a95015c).
Comment 13 Adhemerval Zanella 2020-01-17 19:05:54 UTC
*** Bug 25413 has been marked as a duplicate of this bug. ***