Differential D21088
Avoid conflicts with libc symbols in libthr jump table. Authored by kib on Jul 27 2019, 4:24 PM. Tags None Referenced Files
Subscribers
Details In some corner cases of static linking and unexpected libraries order on the linker command line, libc symbol might be preferred over the same libthr symbol, in which case libthr jump table points back to libc causing either infinite recursion or loop. Handle several of such symbols by using libthr names for them, ensuring that the right pointers are installed into the table. Reported by: arichardson
Diff Detail
Event TimelineComment Actions Thanks! I can confirm that it works for the testcase from the PR if I apply the suggested fix.
Comment Actions However, I just noticed that the following test case still results in an infinite loop/recursion even with this patch. I prints the address of pthread_create, but then freezes. Guess we need to do the same trick for even more functions. #include <stdio.h>
#include <pthread.h>
void* thread_func(void* arg) {
return (void*)0x1234;
}
int main() {
printf("pthread_create = %p\n", (void*)&pthread_create);
pthread_t thr;
pthread_create(&thr, NULL, &thread_func, NULL);
void* result;
pthread_join(thr, &result);
printf("result = %p\n", result);
return 0;
}root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first pthread_create = 0x269d70 result = 0x1234 root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first pthread_create = 0x2a58a0 ^C Looks like the same problem, just this time with pthread_join: root@qemu-native-alr48:~ # lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 801 launching
Process 801 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 801 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
frame #0: 0x00000000002a5570 test-c-first`pthread_join_int(p0=0x00000008002ce500, p1=0x00007fffffffeb58) at _pthread_stubs.c:281
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
* frame #0: 0x00000000002a5570 test-c-first`pthread_join_int(p0=0x00000008002ce500, p1=0x00007fffffffeb58) at _pthread_stubs.c:281
frame #1: 0x00000000002a56ef test-c-first`main + 95
frame #2: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7Comment Actions Seems like it will still break for all other functions that aren't updated?
Comment Actions Yes, but I think I will do them all later. For now lets finish this piece that demonstrates the the approach is viable.
Comment Actions Approach is definitely viable. However, if we need to update all functions a macro is probably a good idea. The extended test case now fails in pthread_self: Process 712 launching
Process 712 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 712 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
frame #0: 0x00000000002a51b0 test-c-first`pthread_self_int at _pthread_stubs.c:251
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSTOP
* frame #0: 0x00000000002a51b0 test-c-first`pthread_self_int at _pthread_stubs.c:251
frame #1: 0x00000000002a3d5f test-c-first`_flockfile(fp=0x00000000002b5ff8) at _flock_stub.c:66:24
frame #2: 0x0000000000281e58 test-c-first`vfprintf_l(fp=0x00000000002b5ff8, locale=0x00000000002b4bf8, fmt0="r2
frame #3: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
frame #4: 0x00000000002a5707 test-c-first`main + 119
frame #5: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7Comment Actions I am now getting a very strange error: root@qemu-native-alr48:~ # clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x2693d0
result = 0x1234
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first
pthread_create = 0x2a58a0
Segmentation fault (core dumped)
root@qemu-native-alr48:~ # clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && lldb -- ./test-c-first
(lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) r
Process 746 launching
Process 746 launched: '/root/test-c-first' (x86_64)
pthread_create = 0x2a58a0
Process 746 stopped
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = signal SIGSEGV: invalid address (fault address: 0x186e8)
* frame #0: 0x00000000002a4b8d test-c-first`memcpy at memmove.S:306
frame #1: 0x00000000002a2b53 test-c-first`__sfvwrite(fp=<unavailable>, uio=0x00007fffffffe888) at fvwrite.c:188:5
frame #2: 0x0000000000285442 test-c-first`__vfprintf [inlined] __sprint(fp=<unavailable>, uio=<unavailable>, locale=<unavailable>) at vfprintf.c:166:8
frame #3: 0x0000000000285436 test-c-first`__vfprintf [inlined] io_flush(iop=<unavailable>, locale=<unavailable>) at printfcommon.h:157
frame #4: 0x000000000028541f test-c-first`__vfprintf(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0=<unavailable>, ap=0x00007fffffffeb10) at vfprintf.c:1033
frame #5: 0x0000000000281ebd test-c-first`vfprintf_l(fp=0x00000000002b7ff8, locale=0x00000000002b6bf8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:285:9
frame #6: 0x00000000002a57ba test-c-first`printf(fmt=<unavailable>) at printf.c:57:8
frame #7: 0x00000000002a5707 test-c-first`main + 119
frame #8: 0x000000000022510f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7
Comment Actions Indeed strange. I cannot reproduce it locally. For me it is just x% ./test-c-first /usr/home/pooma/build/bsd/DEV/stuff/tests pthread_create = 0x2a58b0 x% Can you build libc with debugging info and lower optimization level (DEBUG_FLAGS=-g should be enough), and then print out the stdout FILE state ? I suspect there is some problem there.
Comment Actions I'll try an O0 build of libthr and libc later. I tried applying this patch to cheribsd and fp->_p ended up being an invalid capability without a tag so memcpy fails. Not sure how it got corrupted. Guess the same thing is happening on x86 and causing the segfault. I'll investigate further... Comment Actions Just to be clear this only happens if I actually start a thread and call pthread_join (using the program I posted here). The simple case from the PR works fine. Comment Actions With -O0 at the time of the crash: (lldb) p *__stdoutp
(FILE) $1 = {
_p = 0x00000000000186e8 ""
_r = 0
_w = 0
_flags = 2185
_file = 1
_bf = (_base = "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5)
_lbfsize = -4096
_cookie = 0x000000000031d2e8
_close = 0x00000000002fe580 (test-c-first`__sclose at stdio.c:80)
_read = 0x00000000002fe4c0 (test-c-first`__sread at stdio.c:56)
_seek = 0x00000000002fe540 (test-c-first`__sseek at stdio.c:72)
_write = 0x00000000002fe500 (test-c-first`__swrite at stdio.c:64)
_ub = (_base = 0x0000000000000000, _size = 0)
_up = 0x0000000000000000
_ur = 0
_ubuf = ""
_nbuf = ""
_lb = (_base = 0x0000000000000000, _size = 0)
_blksize = 4096
_offset = 0
_fl_mutex = 0x0000000000000000
_fl_owner = 0x0000000000000000
_fl_count = 1
_orientation = 0
_mbstate = (__mbstate8 = "", _mbstateL = 0)
_flags2 = 0
}
(lldb) p __stdoutp->_bf._base
(unsigned char *) $4 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa.
(lldb) p __stdoutp->_bf._size
(int) $5 = 4096This is extremly odd. After lots of adding breakpoints and single-stepping (watchpoints didn't work, I guess because it's a syscall) it turns out the syscall in __sys_thr_self is causing the corruption. (lldb) target create "./test-c-first"
Current executable set to './test-c-first' (x86_64).
(lldb) b __sys_thr_self
Breakpoint 1: where = test-c-first`__sys_thr_self + 5, address = 0x00000000002fd4b5
(lldb) r
Process 771 launching
Process 771 launched: '/root/test-c-first' (x86_64)
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
* frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
frame #1: 0x00000000003041f5 test-c-first`init_main_thread(thread=0x0000000800335000) at thr_init.c:379:2
frame #2: 0x0000000000303dac test-c-first`_libpthread_init(curthread=0x0000000800335000) at thr_init.c:339:3
frame #3: 0x0000000000303cfd test-c-first`_thread_init_hack at thr_init.c:295:2
frame #4: 0x00000000002361e9 test-c-first`handle_static_init(argc=1, argv=0x00007fffffffebd8, env=0x00007fffffffebe8) at ignore_init.c:124:4
frame #5: 0x0000000000236102 test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:75:2
(lldb) c
Process 771 resuming
pthread_create = 0x301e10
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) bt
* thread #1, name = 'test-c-first', stop reason = breakpoint 1.1
* frame #0: 0x00000000002fd4b5 test-c-first`__sys_thr_self at thr_self.S:3
frame #1: 0x0000000000300787 test-c-first`pthread_self_int at _pthread_stubs.c:251:1
frame #2: 0x00000000002fea01 test-c-first`_flockfile(fp=0x000000000031d2e8) at _flock_stub.c:66:24
frame #3: 0x00000000002cb85a test-c-first`vfprintf_l(fp=0x000000000031d2e8, locale=0x000000000031bec0, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprint2
frame #4: 0x00000000002d0875 test-c-first`vfprintf(fp=0x000000000031d2e8, fmt0="result = %p\n", ap=0x00007fffffffeb10) at vfprintf.c:292:9
frame #5: 0x0000000000301c1f test-c-first`printf(fmt="result = %p\n") at printf.c:57:8
frame #6: 0x0000000000301a97 test-c-first`main at test.c:14:2
frame #7: 0x000000000023610f test-c-first`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7
(lldb) p __stdoutp->_p
(unsigned char *) $0 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5.
(lldb) si
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = instruction step into
frame #0: 0x00000000002fd4b8 test-c-first`__sys_thr_self at thr_self.S:3
(lldb) disassemble
test-c-first`__sys_thr_self:
0x2fd4b0 <+0>: movl $0x1b0, %eax ; imm = 0x1B0
0x2fd4b5 <+5>: movq %rcx, %r10
-> 0x2fd4b8 <+8>: syscall
0x2fd4ba <+10>: jb 0x2ff904 ; .cerror
0x2fd4c0 <+16>: retq
(lldb) p __stdoutp->_p
(unsigned char *) $1 = 0x000000080033b000 "pthread_create = 0x301e10\n\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5\xa5.
(lldb) si
Process 771 stopped
* thread #1, name = 'test-c-first', stop reason = instruction step into
frame #0: 0x00000000002fd4ba test-c-first`__sys_thr_self at thr_self.S:3
(lldb) p __stdoutp->_p
(unsigned char *) $2 = 0x00000000000186e3 ""Comment Actions Some more information: (lldb) register read
General Purpose Registers:
rax = 0x00000000000001b0
rbx = 0x0000000000000001
rcx = 0x00007fffffffeb10
rdx = 0x00000000002050fd
rdi = 0x000000000031d2e8 __sF + 312
rsi = 0x000000000031bec0 __xlocale_global_locale
rbp = 0x00007fffffffe890
rsp = 0x00007fffffffe878
r8 = 0x00007fffffffeb10
r9 = 0x00007fffffffeb10
r10 = 0x00007fffffffeb10
r11 = 0x0000000000000201
r12 = 0x00000000002002e0
r13 = 0x00000000002002e0
r14 = 0x00007fffffffebd8
r15 = 0x00007fffffffebe8
rip = 0x00000000002fd4b8 test-c-first`__sys_thr_self + 8
rflags = 0x0000000000000202
cs = 0x0000000000000043
ss = 0x000000000000003brdi contains the address of __stdoutp->_p 0x000000000031d2e8. I'm not familiar with the x86 syscall convention but I guess the syscall is writing the return value to *rdi?
Comment Actions I can confirm that it works when I apply the following change on top of your patch: diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c index d9bde8a37c9d..78146c2d6dcd 100644 --- a/lib/libthr/thread/thr_init.c +++ b/lib/libthr/thread/thr_init.c @@ -257,7 +257,7 @@ static pthread_func_t jmp_table[][2] = { [PJT_RWLOCK_TRYWRLOCK] = {DUAL_ENTRY(_Tthr_rwlock_trywrlock)}, [PJT_RWLOCK_UNLOCK] = {DUAL_ENTRY(_Tthr_rwlock_unlock)}, [PJT_RWLOCK_WRLOCK] = {DUAL_ENTRY(_Tthr_rwlock_wrlock)}, - [PJT_SELF] = {DUAL_ENTRY(_thr_self)}, + [PJT_SELF] = {DUAL_ENTRY(_Tthr_self)}, [PJT_SETCANCELSTATE] = {DUAL_ENTRY(_thr_setcancelstate)}, [PJT_SETCANCELTYPE] = {DUAL_ENTRY(_thr_setcanceltype)}, [PJT_SETSPECIFIC] = {DUAL_ENTRY(_thr_setspecific)}, diff --git a/lib/libthr/thread/thr_private.h b/lib/libthr/thread/thr_private.h index 90385d1ab31c..47b82009765d 100644 --- a/lib/libthr/thread/thr_private.h +++ b/lib/libthr/thread/thr_private.h @@ -1069,7 +1069,7 @@ int _thr_setspecific(pthread_key_t, const void *); void *_thr_getspecific(pthread_key_t); int _thr_setcancelstate(int, int *); int _thr_setcanceltype(int, int *); -pthread_t _thr_self(void); +pthread_t _Tthr_self(void); int _thr_rwlock_init(pthread_rwlock_t *, const pthread_rwlockattr_t *); int _thr_rwlock_destroy(pthread_rwlock_t *); int _Tthr_rwlock_rdlock(pthread_rwlock_t *); diff --git a/lib/libthr/thread/thr_self.c b/lib/libthr/thread/thr_self.c index 168d91721e0b..68bf5bc39312 100644 --- a/lib/libthr/thread/thr_self.c +++ b/lib/libthr/thread/thr_self.c @@ -38,11 +38,11 @@ __FBSDID("$FreeBSD$"); #include "thr_private.h" -__weak_reference(_thr_self, pthread_self); -__weak_reference(_thr_self, _pthread_self); +__weak_reference(_Tthr_self, pthread_self); +__weak_reference(_Tthr_self, _pthread_self); pthread_t -_thr_self(void) +_Tthr_self(void) { _thr_check_init(); It might make sense to include a test that we can link libthr statically? Comment Actions This totally makes sense and is obvious after the diagnostic. Thank you very much for tracking it down.
Perhaps. I wil ask pho to do some sanity check of the combined patch later today. Comment Actions Just found one more issue but otherwise LGTM. Maybe better to consistently use the _Tthr prefix.
Comment Actions I ran all of the threaded tests from stress2 on amd64 with D21088.60283.diff.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There might be a micro optimization here to do the V_broadcast_zero test LAST, that way the additional test in the fast path only occurs after the tests we have been doing all along.