Bug 239475 - Linking libthr with -nodefaultlibs statically can cause infinite recursion
Summary: Linking libthr with -nodefaultlibs statically can cause infinite recursion
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: threads (show other bugs)
Version: 11.2-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-threads (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-27 11:51 UTC by Alex Richardson
Modified: 2020-06-07 16:47 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Richardson freebsd_committer freebsd_triage 2019-07-27 11:51:41 UTC
For the following test program:

```
#include <stdio.h>
#include <pthread.h>

int main() {
  printf("pthread_create = %p\n", (void*)&pthread_create);
  return 0;
}
```

If I link -lthr after -lc, I get infinite recursion in the pthread interposing table since the value in libthr.a is resolved to the one from libc which then just calls itself.

#0  0x000000000026c0bc in __pthread_cleanup_push_imp ()
#1  0x000000000026c30a in printf ()
#2  0x000000000026c25d in main ()

The following fails:
$ clang -static -o test-c-first -nodefaultlibs -lc -lthr test.c -fuse-ld=lld && ./test-c-first

Whereas this works:
clang -static -o test-thr-first -nodefaultlibs -lthr -lc test.c -fuse-ld=lld && ./test-thr-first
pthread_create = 0x24dbc0


I am hitting this issue while writing some tests that want to avoid pulling in all default libs, but the order if libraries on the link command line is difficult to change.
I can probably work around it, but it seems like this infinite recursion should not happen even if you link in the wrong order. I'm not sure why the functions in the libthr table resolve to the libc ones instead of the ones in libthr.
Comment 1 Konstantin Belousov freebsd_committer freebsd_triage 2019-07-27 14:22:16 UTC
For me it was reproduced as infinite loop, perhaps due to the the tail call elimination.  The issue is that when -lc is passed first, __pthread_cleanup_push_imp is found in libc, and then the libthr jump table, which references the symbol, is satisfied with the same libc definition.

Try this, I did not even compiled with the patch.  It might require some additional tweaking to get the stuff actually working.

diff --git a/lib/libthr/thread/thr_clean.c b/lib/libthr/thread/thr_clean.c
index 5a93d94a7e5..7bc7d62b617 100644
--- a/lib/libthr/thread/thr_clean.c
+++ b/lib/libthr/thread/thr_clean.c
@@ -49,6 +49,10 @@ __FBSDID("$FreeBSD$");
 __weak_reference(_pthread_cleanup_push, pthread_cleanup_push);
 __weak_reference(_pthread_cleanup_pop, pthread_cleanup_pop);
 
+/* help static linking when libc symbols have preference */
+__weak_reference(__pthread_cleanup_push_imp, __pthread_cleanup_push_imp1);
+__weak_reference(__pthread_cleanup_pop_imp, pthread_cleanup_pop_imp1);
+
 void
 __pthread_cleanup_push_imp(void (*routine)(void *), void *arg,
 	struct _pthread_cleanup_info *info)
diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
index 7b043a38b1f..22802c0ae1a 100644
--- a/lib/libthr/thread/thr_init.c
+++ b/lib/libthr/thread/thr_init.c
@@ -202,6 +202,10 @@ STATIC_LIB_REQUIRE(_thread_state_running);
 #define	DUAL_ENTRY(entry)	\
 	(pthread_func_t)entry, (pthread_func_t)entry
 
+void __pthread_cleanup_push_imp1(void (*)(void *), void *,
+    struct _pthread_cleanup_info *);
+void __pthread_cleanup_pop_imp1(int);
+
 static pthread_func_t jmp_table[][2] = {
 	{DUAL_ENTRY(_pthread_atfork)},	/* PJT_ATFORK */
 	{DUAL_ENTRY(_pthread_attr_destroy)},	/* PJT_ATTR_DESTROY */
@@ -265,8 +269,8 @@ static pthread_func_t jmp_table[][2] = {
 	{DUAL_ENTRY(_pthread_setspecific)},	/* PJT_SETSPECIFIC */
 	{DUAL_ENTRY(_pthread_sigmask)},		/* PJT_SIGMASK */
 	{DUAL_ENTRY(_pthread_testcancel)},	/* PJT_TESTCANCEL */
-	{DUAL_ENTRY(__pthread_cleanup_pop_imp)},/* PJT_CLEANUP_POP_IMP */
-	{DUAL_ENTRY(__pthread_cleanup_push_imp)},/* PJT_CLEANUP_PUSH_IMP */
+	{DUAL_ENTRY(__pthread_cleanup_pop_imp1)},/* PJT_CLEANUP_POP_IMP */
+	{DUAL_ENTRY(__pthread_cleanup_push_imp1)},/* PJT_CLEANUP_PUSH_IMP */
 	{DUAL_ENTRY(_pthread_cancel_enter)},	/* PJT_CANCEL_ENTER */
 	{DUAL_ENTRY(_pthread_cancel_leave)},	/* PJT_CANCEL_LEAVE */
 	{DUAL_ENTRY(_pthread_mutex_consistent)},/* PJT_MUTEX_CONSISTENT */
Comment 2 Alex Richardson freebsd_committer freebsd_triage 2019-07-27 16:11:22 UTC
Thanks, I guess that should work.
However, we probably have to do it for all the functions in the table? Or maybe only the ones referenced in libc?

This seems to be:
void	_pthread_cancel_enter(int);
void	_pthread_cancel_leave(int);
void	___pthread_cleanup_push_imp(void (*)(void *), void *,
	    struct _pthread_cleanup_info *);
void	___pthread_cleanup_pop_imp(int);
Comment 3 Konstantin Belousov freebsd_committer freebsd_triage 2019-07-27 16:24:09 UTC
https://reviews.freebsd.org/D21088
Comment 4 commit-hook freebsd_committer freebsd_triage 2019-07-31 19:27:54 UTC
A commit references this bug:

Author: kib
Date: Wed Jul 31 19:27:24 UTC 2019
New revision: 350481
URL: https://svnweb.freebsd.org/changeset/base/350481

Log:
  Avoid conflicts with libc symbols in libthr jump table.

  In some corner cases of static linking and unexpected libraries order
  on the linker command line, libc symbol might preempt the same libthr
  symbol, in which case libthr jump table points back to libc causing
  either infinite recursion or loop.  Handle all of such symbols by
  using private libthr names for them, ensuring that the right pointers
  are installed into the table.

  In collaboration with: arichardson
  PR:	239475
  Tested by:	pho
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential revision:	https://reviews.freebsd.org/D21088

Changes:
  head/lib/libthr/thread/thr_attr.c
  head/lib/libthr/thread/thr_cancel.c
  head/lib/libthr/thread/thr_clean.c
  head/lib/libthr/thread/thr_cond.c
  head/lib/libthr/thread/thr_detach.c
  head/lib/libthr/thread/thr_equal.c
  head/lib/libthr/thread/thr_exit.c
  head/lib/libthr/thread/thr_fork.c
  head/lib/libthr/thread/thr_getthreadid_np.c
  head/lib/libthr/thread/thr_init.c
  head/lib/libthr/thread/thr_join.c
  head/lib/libthr/thread/thr_kill.c
  head/lib/libthr/thread/thr_main_np.c
  head/lib/libthr/thread/thr_mutex.c
  head/lib/libthr/thread/thr_mutexattr.c
  head/lib/libthr/thread/thr_once.c
  head/lib/libthr/thread/thr_private.h
  head/lib/libthr/thread/thr_rwlock.c
  head/lib/libthr/thread/thr_self.c
  head/lib/libthr/thread/thr_sig.c
  head/lib/libthr/thread/thr_spec.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2019-07-31 20:05:02 UTC
A commit references this bug:

Author: kib
Date: Wed Jul 31 20:04:40 UTC 2019
New revision: 350483
URL: https://svnweb.freebsd.org/changeset/base/350483

Log:
  Avoid conflicts with libc symbols in libthr jump table.

  In some corner cases of static linking and unexpected libraries order
  on the linker command line, libc symbol might preempt the same libthr
  symbol, in which case libthr jump table points back to libc causing
  either infinite recursion or loop.  Handle all of such symbols by
  using private libthr names for them, ensuring that the right pointers
  are installed into the table.

  In collaboration with: arichardson
  PR:	239475
  Tested by:	pho
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential revision:	https://reviews.freebsd.org/D21088

Changes:
  head/lib/libthr/thread/thr_private.h