Make dlerror(3) thread-safe.

I gave up trying to use the usual thread-local buffer idiom; calls to
calloc(3) and free(3) from any of the "dl" functions -- which live in
the dynamic linker -- end up resolving to the dynamic linker's stubs.
I tried to work around that, but was just making things more complicated.
This alternative costs us a well-known TLS slot (instead of the
dynamically-allocated TLS slot we'd have used otherwise, so no difference
there), plus an extra buffer inside every pthread_internal_t.

Bug: 5404023
Change-Id: Ie9614edd05b6d1eeaf7bf9172792d616c6361767

libc/bionic/pthread.c

@@ -163,20 +163,20 @@ __get_stack_base(int  *p_stack_size)
 }
 
 
-void  __init_tls(void**  tls, void*  thread)
+void  __init_tls(void** tls, void* thread) {
-{
+  ((pthread_internal_t*) thread)->tls = tls;
-    int  nn;
 
-    ((pthread_internal_t*)thread)->tls = tls;
+  // Zero-initialize all the slots.
+  for (size_t i = 0; i < BIONIC_TLS_SLOTS; ++i) {
+    tls[i] = NULL;
+  }
 
-    // slot 0 must point to the tls area, this is required by the implementation
+  // Slot 0 must point to the tls area, this is required by the implementation
-    // of the x86 Linux kernel thread-local-storage
+  // of the x86 Linux kernel thread-local-storage.
-    tls[TLS_SLOT_SELF]      = (void*)tls;
+  tls[TLS_SLOT_SELF]      = (void*) tls;
-    tls[TLS_SLOT_THREAD_ID] = thread;
+  tls[TLS_SLOT_THREAD_ID] = thread;
-    for (nn = TLS_SLOT_ERRNO; nn < BIONIC_TLS_SLOTS; nn++)
-       tls[nn] = 0;
 
-    __set_tls( (void*)tls );
+  __set_tls((void*) tls);
 }
 
 

libc/bionic/pthread_internal.h

@@ -45,6 +45,13 @@ typedef struct pthread_internal_t
     int                         internal_flags;
     __pthread_cleanup_t*        cleanup_stack;
     void**                      tls;         /* thread-local storage area */
+
+    /*
+     * The dynamic linker implements dlerror(3), which makes it hard for us to implement this
+     * per-thread buffer by simply using malloc(3) and free(3).
+     */
+#define __BIONIC_DLERROR_BUFFER_SIZE 512
+    char dlerror_buffer[__BIONIC_DLERROR_BUFFER_SIZE];
 } pthread_internal_t;
 
 int _init_thread(pthread_internal_t* thread, pid_t kernel_id, pthread_attr_t* attr,

libc/bionic/strerror.cpp

@@ -41,6 +41,6 @@ char* strerror(int error_number) {
   }
 
   LOCAL_INIT_THREAD_LOCAL_BUFFER(char*, strerror, NL_TEXTMAX);
-  strerror_r(error_number, strerror_buffer, strerror_buffer_size);
+  strerror_r(error_number, strerror_tls_buffer, strerror_tls_buffer_size);
-  return strerror_buffer;
+  return strerror_tls_buffer;
 }

libc/bionic/strsignal.cpp

@@ -42,5 +42,5 @@ char* strsignal(int signal_number) {
   }
 
   LOCAL_INIT_THREAD_LOCAL_BUFFER(char*, strsignal, NL_TEXTMAX);
-  return const_cast<char*>(__strsignal(signal_number, strsignal_buffer, strsignal_buffer_size));
+  return const_cast<char*>(__strsignal(signal_number, strsignal_tls_buffer, strsignal_tls_buffer_size));
 }

libc/private/ThreadLocalBuffer.h

@@ -37,23 +37,24 @@
 // TODO: move __cxa_guard_acquire and __cxa_guard_release into libc.
 
 #define GLOBAL_INIT_THREAD_LOCAL_BUFFER(name) \
-  static pthread_once_t name ## _once; \
+  static pthread_once_t __bionic_tls_ ## name ## _once; \
-  static pthread_key_t name ## _key; \
+  static pthread_key_t __bionic_tls_ ## name ## _key; \
-  static void name ## _key_destroy(void* buffer) { \
+  static void __bionic_tls_ ## name ## _key_destroy(void* buffer) { \
     free(buffer); \
   } \
-  static void name ## _key_init() { \
+  static void __bionic_tls_ ## name ## _key_init() { \
-    pthread_key_create(&name ## _key, name ## _key_destroy); \
+    pthread_key_create(&__bionic_tls_ ## name ## _key, __bionic_tls_ ## name ## _key_destroy); \
   }
 
-// Leaves "name_buffer" and "name_byte_count" defined and initialized.
+// Leaves "name_tls_buffer" and "name_tls_buffer_size" defined and initialized.
 #define LOCAL_INIT_THREAD_LOCAL_BUFFER(type, name, byte_count) \
-  pthread_once(&name ## _once, name ## _key_init); \
+  pthread_once(&__bionic_tls_ ## name ## _once, __bionic_tls_ ## name ## _key_init); \
-  type name ## _buffer = reinterpret_cast<type>(pthread_getspecific(name ## _key)); \
+  type name ## _tls_buffer = \
-  if (name ## _buffer == NULL) { \
+      reinterpret_cast<type>(pthread_getspecific(__bionic_tls_ ## name ## _key)); \
-    name ## _buffer = reinterpret_cast<type>(malloc(byte_count)); \
+  if (name ## _tls_buffer == NULL) { \
-    pthread_setspecific(name ## _key, name ## _buffer); \
+    name ## _tls_buffer = reinterpret_cast<type>(calloc(1, byte_count)); \
+    pthread_setspecific(__bionic_tls_ ## name ## _key, name ## _tls_buffer); \
   } \
-  const size_t name ## _buffer_size = byte_count
+  const size_t name ## _tls_buffer_size __attribute__((unused)) = byte_count
 
 #endif // _BIONIC_THREAD_LOCAL_BUFFER_H_included

libc/private/bionic_tls.h

@@ -52,7 +52,7 @@ __BEGIN_DECLS
  * thread-specific segment descriptor...
  */
 
-/* Well known TLS slots */
+/* Well-known TLS slots. */
 #define TLS_SLOT_SELF               0
 #define TLS_SLOT_THREAD_ID          1
 #define TLS_SLOT_ERRNO              2
@@ -60,23 +60,16 @@ __BEGIN_DECLS
 #define TLS_SLOT_OPENGL_API         3
 #define TLS_SLOT_OPENGL             4
 
-/* this slot is only used to pass information from the dynamic linker to
+#define TLS_SLOT_DLERROR            5
+
+#define TLS_SLOT_MAX_WELL_KNOWN     TLS_SLOT_DLERROR
+
+/* This slot is only used to pass information from the dynamic linker to
  * libc.so when the C library is loaded in to memory. The C runtime init
  * function will then clear it. Since its use is extremely temporary,
  * we reuse an existing location.
  */
-#define  TLS_SLOT_BIONIC_PREINIT    (TLS_SLOT_ERRNO+1)
+#define  TLS_SLOT_BIONIC_PREINIT    TLS_SLOT_OPENGL_API
-
-/* small technical note: it is not possible to call pthread_setspecific
- * on keys that are <= TLS_SLOT_MAX_WELL_KNOWN, which is why it is set to
- * TLS_SLOT_ERRNO.
- *
- * later slots like TLS_SLOT_OPENGL are pre-allocated through the use of
- * TLS_DEFAULT_ALLOC_MAP. this means that there is no need to use
- * pthread_key_create() to initialize them. on the other hand, there is
- * no destructor associated to them (we might need to implement this later)
- */
-#define TLS_SLOT_MAX_WELL_KNOWN     TLS_SLOT_ERRNO
 
 #define TLS_DEFAULT_ALLOC_MAP       0x0000001F
 

linker/dlfcn.cpp

@@ -14,39 +14,52 @@
  * limitations under the License.
  */
 
+#include "linker.h"
+
 #include <dlfcn.h>
 #include <pthread.h>
 #include <stdio.h>
+#include <stdlib.h>
 
+#include <bionic/pthread_internal.h>
+#include <private/bionic_tls.h>
 #include <private/ScopedPthreadMutexLocker.h>
-
+#include <private/ThreadLocalBuffer.h>
-#include "linker.h"
-#include "linker_format.h"
 
 /* This file hijacks the symbols stubbed out in libdl.so. */
 
-static char dl_err_buf[1024];
-static const char* dl_err_str;
-
-#define likely(expr)   __builtin_expect (expr, 1)
-#define unlikely(expr) __builtin_expect (expr, 0)
-
 static pthread_mutex_t gDlMutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER;
 
-static void set_dlerror(const char* msg, const char* detail) {
+static const char* __bionic_set_dlerror(char* new_value) {
-  if (detail != NULL) {
+  void* tls = const_cast<void*>(__get_tls());
-    format_buffer(dl_err_buf, sizeof(dl_err_buf), "%s: %s", msg, detail);
+  char** dlerror_slot = &reinterpret_cast<char**>(tls)[TLS_SLOT_DLERROR];
-  } else {
+
-    format_buffer(dl_err_buf, sizeof(dl_err_buf), "%s", msg);
+  const char* old_value = *dlerror_slot;
-  }
+  *dlerror_slot = new_value;
-  dl_err_str = (const char*) &dl_err_buf[0];
+  return old_value;
 }
 
-void *dlopen(const char* filename, int flag) {
+static void __bionic_format_dlerror(const char* msg, const char* detail) {
+  char* buffer = __get_thread()->dlerror_buffer;
+  strlcpy(buffer, msg, __BIONIC_DLERROR_BUFFER_SIZE);
+  if (detail != NULL) {
+    strlcat(buffer, ": ", __BIONIC_DLERROR_BUFFER_SIZE);
+    strlcat(buffer, detail, __BIONIC_DLERROR_BUFFER_SIZE);
+  }
+
+  __bionic_set_dlerror(buffer);
+}
+
+const char* dlerror() {
+  const char* old_value = __bionic_set_dlerror(NULL);
+  return old_value;
+}
+
+void* dlopen(const char* filename, int flag) {
   ScopedPthreadMutexLocker locker(&gDlMutex);
   soinfo* result = find_library(filename);
   if (result == NULL) {
-    set_dlerror("dlopen failed", linker_get_error());
+    __bionic_format_dlerror("dlopen failed", linker_get_error());
     return NULL;
   }
   soinfo_call_constructors(result);
@@ -54,21 +67,15 @@ void *dlopen(const char* filename, int flag) {
   return result;
 }
 
-const char* dlerror() {
-  const char* old_value = dl_err_str;
-  dl_err_str = NULL;
-  return (const char*) old_value;
-}
-
 void* dlsym(void* handle, const char* symbol) {
   ScopedPthreadMutexLocker locker(&gDlMutex);
 
-  if (unlikely(handle == 0)) {
+  if (handle == NULL) {
-    set_dlerror("dlsym library handle is null", NULL);
+    __bionic_format_dlerror("dlsym library handle is null", NULL);
     return NULL;
   }
-  if (unlikely(symbol == 0)) {
+  if (symbol == NULL) {
-    set_dlerror("dlsym symbol name is null", NULL);
+    __bionic_format_dlerror("dlsym symbol name is null", NULL);
     return NULL;
   }
 
@@ -89,18 +96,18 @@ void* dlsym(void* handle, const char* symbol) {
     sym = soinfo_lookup(found, symbol);
   }
 
-  if (likely(sym != 0)) {
+  if (sym != NULL) {
     unsigned bind = ELF32_ST_BIND(sym->st_info);
 
-    if (likely((bind == STB_GLOBAL) && (sym->st_shndx != 0))) {
+    if (bind == STB_GLOBAL && sym->st_shndx != 0) {
       unsigned ret = sym->st_value + found->load_bias;
       return (void*) ret;
     }
 
-    set_dlerror("symbol found but not global", symbol);
+    __bionic_format_dlerror("symbol found but not global", symbol);
     return NULL;
   } else {
-    set_dlerror("undefined symbol", symbol);
+    __bionic_format_dlerror("undefined symbol", symbol);
     return NULL;
   }
 }
@@ -159,7 +166,7 @@ int dlclose(void* handle) {
       /* st_other */ 0, \
       shndx }
 
-static Elf32_Sym libdl_symtab[] = {
+static Elf32_Sym gLibDlSymtab[] = {
   // Total length of libdl_info.strtab, including trailing 0.
   // This is actually the STH_UNDEF entry. Technically, it's
   // supposed to have st_name == 0, but instead, it points to an index
@@ -179,24 +186,24 @@ static Elf32_Sym libdl_symtab[] = {
 
 // Fake out a hash table with a single bucket.
 // A search of the hash table will look through
-// libdl_symtab starting with index [1], then
+// gLibDlSymtab starting with index [1], then
-// use libdl_chains to find the next index to
+// use gLibDlChains to find the next index to
-// look at.  libdl_chains should be set up to
+// look at.  gLibDlChains should be set up to
-// walk through every element in libdl_symtab,
+// walk through every element in gLibDlSymtab,
 // and then end with 0 (sentinel value).
 //
-// That is, libdl_chains should look like
+// That is, gLibDlChains should look like
 // { 0, 2, 3, ... N, 0 } where N is the number
-// of actual symbols, or nelems(libdl_symtab)-1
+// of actual symbols, or nelems(gLibDlSymtab)-1
-// (since the first element of libdl_symtab is not
+// (since the first element of gLibDlSymtab is not
 // a real symbol).
 //
 // (see soinfo_elf_lookup())
 //
 // Note that adding any new symbols here requires
 // stubbing them out in libdl.
-static unsigned libdl_buckets[1] = { 1 };
+static unsigned gLibDlBuckets[1] = { 1 };
-static unsigned libdl_chains[7] = { 0, 2, 3, 4, 5, 6, 0 };
+static unsigned gLibDlChains[7] = { 0, 2, 3, 4, 5, 6, 0 };
 
 // This is used by the dynamic linker. Every process gets these symbols for free.
 soinfo libdl_info = {
@@ -210,12 +217,12 @@ soinfo libdl_info = {
     flags: FLAG_LINKED,
 
     strtab: ANDROID_LIBDL_STRTAB,
-    symtab: libdl_symtab,
+    symtab: gLibDlSymtab,
 
     nbucket: 1,
     nchain: 7,
-    bucket: libdl_buckets,
+    bucket: gLibDlBuckets,
-    chain: libdl_chains,
+    chain: gLibDlChains,
 
     plt_got: 0, plt_rel: 0, plt_rel_count: 0, rel: 0, rel_count: 0,
     preinit_array: 0, preinit_array_count: 0, init_array: 0, init_array_count: 0,

linker/linker.cpp

@@ -168,16 +168,15 @@ static char __linker_dl_err_buf[768];
         ERROR(fmt "\n", ##x); \
     } while(0)
 
-const char *linker_get_error(void)
+const char* linker_get_error() {
-{
+  return &__linker_dl_err_buf[0];
-    return (const char *)&__linker_dl_err_buf[0];
 }
 
 /*
  * This function is an empty stub where GDB locates a breakpoint to get notified
  * about linker activity.
  */
-extern "C" void __attribute__((noinline)) __attribute__((visibility("default"))) rtld_db_dlactivity(void);
+extern "C" void __attribute__((noinline)) __attribute__((visibility("default"))) rtld_db_dlactivity();
 
 static r_debug _r_debug = {1, NULL, &rtld_db_dlactivity,
                                   RT_CONSISTENT, 0};
@@ -1363,8 +1362,7 @@ static void call_destructors(soinfo *si)
 
 /* Force any of the closed stdin, stdout and stderr to be associated with
    /dev/null. */
-static int nullify_closed_stdio (void)
+static int nullify_closed_stdio() {
-{
     int dev_null, i, status;
     int return_value = 0;
 
@@ -2056,5 +2054,8 @@ extern "C" unsigned __linker_init(unsigned **elfdata) {
 
     // We have successfully fixed our own relocations. It's safe to run
     // the main part of the linker now.
-    return __linker_init_post_relocation(elfdata, linker_addr);
+    unsigned start_address = __linker_init_post_relocation(elfdata, linker_addr);
+
+    // Return the address that the calling assembly stub should jump to.
+    return start_address;
 }

tests/dlopen_test.cpp

@@ -58,6 +58,27 @@ TEST(dlopen, dlopen_failure) {
 #endif
 }
 
+static void* ConcurrentDlErrorFn(void* arg) {
+  dlopen("/child/thread", RTLD_NOW);
+  return reinterpret_cast<void*>(strdup(dlerror()));
+}
+
+TEST(dlopen, dlerror_concurrent) {
+  dlopen("/main/thread", RTLD_NOW);
+  const char* main_thread_error = dlerror();
+  ASSERT_SUBSTR("/main/thread", main_thread_error);
+
+  pthread_t t;
+  ASSERT_EQ(0, pthread_create(&t, NULL, ConcurrentDlErrorFn, NULL));
+  void* result;
+  ASSERT_EQ(0, pthread_join(t, &result));
+  char* child_thread_error = static_cast<char*>(result);
+  ASSERT_SUBSTR("/child/thread", child_thread_error);
+  free(child_thread_error);
+
+  ASSERT_SUBSTR("/main/thread", main_thread_error);
+}
+
 TEST(dlopen, dlsym_failures) {
   dlerror(); // Clear any pending errors.
   void* self = dlopen(NULL, RTLD_NOW);

tests/string_test.cpp

@@ -29,7 +29,7 @@ TEST(string, strerror) {
   ASSERT_STREQ("Unknown error 1234", strerror(1234));
 }
 
-void* ConcurrentStrErrorFn(void* arg) {
+static void* ConcurrentStrErrorFn(void* arg) {
   bool equal = (strcmp("Unknown error 2002", strerror(2002)) == 0);
   return reinterpret_cast<void*>(equal);
 }
@@ -88,7 +88,7 @@ TEST(string, strsignal) {
   ASSERT_STREQ("Unknown signal 1234", strsignal(1234)); // Too large.
 }
 
-void* ConcurrentStrSignalFn(void* arg) {
+static void* ConcurrentStrSignalFn(void* arg) {
   bool equal = (strcmp("Unknown signal 2002", strsignal(2002)) == 0);
   return reinterpret_cast<void*>(equal);
 }