From 5e2285d3ccdbb64a49ad2e5e521f50c897a3954d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 22 Feb 2017 12:19:05 -0800 Subject: [PATCH] Allocate thread local buffers in __init_tls. Thread local buffers were using pthread_setspecific for storage with lazy initialization. pthread_setspecific shares TLS slots between the linker and libc.so, so thread local buffers being initialized in a different order between libc.so and the linker meant that bad things would happen (manifesting as snprintf not working because the locale was mangled) Bug: http://b/20464031 Test: /data/nativetest64/bionic-unit-tests/bionic-unit-tests everything passes Test: /data/nativetest/bionic-unit-tests/bionic-unit-tests thread_local tests are failing both before and after (KUSER_HELPERS?) Test: /data/nativetest64/bionic-unit-tests-static/bionic-unit-tests-static no additional failures Change-Id: I9f445a77c6e86979f3fa49c4a5feecf6ec2b0c3f --- libc/bionic/grp_pwd.cpp | 41 ++++++++------------- libc/bionic/libgen.cpp | 13 +++---- libc/bionic/locale.cpp | 16 +++----- libc/bionic/mntent.cpp | 10 ++--- libc/bionic/pthread_create.cpp | 12 ++++++ libc/bionic/pthread_internal.cpp | 4 ++ libc/bionic/pthread_internal.h | 6 +++ libc/bionic/pty.cpp | 15 ++++---- libc/bionic/strerror.cpp | 10 ++--- libc/bionic/strsignal.cpp | 9 ++--- libc/private/bionic_tls.h | 41 ++++++++++++++------- libc/private/{ThreadLocalBuffer.h => grp_pwd.h} | 49 +++++++++---------------- linker/dlfcn.cpp | 1 - 13 files changed, 112 insertions(+), 115 deletions(-) rename libc/private/{ThreadLocalBuffer.h => grp_pwd.h} (60%) diff --git a/libc/bionic/grp_pwd.cpp b/libc/bionic/grp_pwd.cpp index e99eaca29..5d565c4eb 100644 --- a/libc/bionic/grp_pwd.cpp +++ b/libc/bionic/grp_pwd.cpp @@ -39,9 +39,9 @@ #include "private/android_filesystem_config.h" #include "private/bionic_macros.h" +#include "private/grp_pwd.h" #include "private/ErrnoRestorer.h" #include "private/libc_logging.h" -#include "private/ThreadLocalBuffer.h" // Generated android_ids array #include "generated_android_ids.h" @@ -52,25 +52,14 @@ // okay for all the functions to share state, and all the // functions to share state, but functions can't clobber // functions' state and vice versa. +#include "bionic/pthread_internal.h" +static group_state_t* get_group_tls_buffer() { + return &__get_bionic_tls().group; +} -struct group_state_t { - group group_; - char* group_members_[2]; - char group_name_buffer_[32]; - // Must be last so init_group_state can run a simple memset for the above - ssize_t getgrent_idx; -}; - -struct passwd_state_t { - passwd passwd_; - char name_buffer_[32]; - char dir_buffer_[32]; - char sh_buffer_[32]; - ssize_t getpwent_idx; -}; - -static ThreadLocalBuffer g_group_tls_buffer; -static ThreadLocalBuffer g_passwd_tls_buffer; +static passwd_state_t* get_passwd_tls_buffer() { + return &__get_bionic_tls().passwd; +} static void init_group_state(group_state_t* state) { memset(state, 0, sizeof(group_state_t) - sizeof(state->getgrent_idx)); @@ -78,7 +67,7 @@ static void init_group_state(group_state_t* state) { } static group_state_t* __group_state() { - group_state_t* result = g_group_tls_buffer.get(); + group_state_t* result = get_group_tls_buffer(); if (result != nullptr) { init_group_state(result); } @@ -432,7 +421,7 @@ static group* app_id_to_group(gid_t gid, group_state_t* state) { } passwd* getpwuid(uid_t uid) { // NOLINT: implementing bad function. - passwd_state_t* state = g_passwd_tls_buffer.get(); + passwd_state_t* state = get_passwd_tls_buffer(); if (state == NULL) { return NULL; } @@ -450,7 +439,7 @@ passwd* getpwuid(uid_t uid) { // NOLINT: implementing bad function. } passwd* getpwnam(const char* login) { // NOLINT: implementing bad function. - passwd_state_t* state = g_passwd_tls_buffer.get(); + passwd_state_t* state = get_passwd_tls_buffer(); if (state == NULL) { return NULL; } @@ -483,7 +472,7 @@ char* getlogin() { // NOLINT: implementing bad function. } void setpwent() { - passwd_state_t* state = g_passwd_tls_buffer.get(); + passwd_state_t* state = get_passwd_tls_buffer(); if (state) { state->getpwent_idx = 0; } @@ -494,7 +483,7 @@ void endpwent() { } passwd* getpwent() { - passwd_state_t* state = g_passwd_tls_buffer.get(); + passwd_state_t* state = get_passwd_tls_buffer(); if (state == NULL) { return NULL; } @@ -608,7 +597,7 @@ int getgrnam_r(const char* name, struct group* grp, char* buf, size_t buflen, } void setgrent() { - group_state_t* state = g_group_tls_buffer.get(); + group_state_t* state = get_group_tls_buffer(); if (state) { state->getgrent_idx = 0; } @@ -619,7 +608,7 @@ void endgrent() { } group* getgrent() { - group_state_t* state = g_group_tls_buffer.get(); + group_state_t* state = get_group_tls_buffer(); if (state == NULL) { return NULL; } diff --git a/libc/bionic/libgen.cpp b/libc/bionic/libgen.cpp index c415c0f74..33b46a122 100644 --- a/libc/bionic/libgen.cpp +++ b/libc/bionic/libgen.cpp @@ -34,10 +34,7 @@ #include #include -#include "private/ThreadLocalBuffer.h" - -static ThreadLocalBuffer g_basename_tls_buffer; -static ThreadLocalBuffer g_dirname_tls_buffer; +#include "bionic/pthread_internal.h" static int __basename_r(const char* path, char* buffer, size_t buffer_size) { const char* startp = NULL; @@ -161,13 +158,13 @@ __LIBC32_LEGACY_PUBLIC__ int dirname_r(const char* path, char* buffer, size_t bu } char* basename(const char* path) { - char* buf = g_basename_tls_buffer.get(); - int rc = __basename_r(path, buf, g_basename_tls_buffer.size()); + char* buf = __get_bionic_tls().basename_buf; + int rc = __basename_r(path, buf, sizeof(__get_bionic_tls().basename_buf)); return (rc < 0) ? NULL : buf; } char* dirname(const char* path) { - char* buf = g_dirname_tls_buffer.get(); - int rc = __dirname_r(path, buf, g_dirname_tls_buffer.size()); + char* buf = __get_bionic_tls().dirname_buf; + int rc = __dirname_r(path, buf, sizeof(__get_bionic_tls().dirname_buf)); return (rc < 0) ? NULL : buf; } diff --git a/libc/bionic/locale.cpp b/libc/bionic/locale.cpp index 113118d22..38e15b713 100644 --- a/libc/bionic/locale.cpp +++ b/libc/bionic/locale.cpp @@ -37,6 +37,8 @@ #include "private/bionic_macros.h" +#include "bionic/pthread_internal.h" + // We only support two locales, the "C" locale (also known as "POSIX"), // and the "C.UTF-8" locale (also known as "en_US.UTF-8"). @@ -161,17 +163,9 @@ char* setlocale(int category, const char* locale_name) { return const_cast(__bionic_current_locale_is_utf8 ? "C.UTF-8" : "C"); } -// We can't use a constructor to create g_uselocal_key, because it may be used in constructors. -static pthread_once_t g_uselocale_once = PTHREAD_ONCE_INIT; -static pthread_key_t g_uselocale_key; - -static void g_uselocale_key_init() { - pthread_key_create(&g_uselocale_key, NULL); -} - locale_t uselocale(locale_t new_locale) { - pthread_once(&g_uselocale_once, g_uselocale_key_init); - locale_t old_locale = static_cast(pthread_getspecific(g_uselocale_key)); + locale_t* locale_storage = &__get_bionic_tls().locale; + locale_t old_locale = *locale_storage; // If this is the first call to uselocale(3) on this thread, we return LC_GLOBAL_LOCALE. if (old_locale == NULL) { @@ -179,7 +173,7 @@ locale_t uselocale(locale_t new_locale) { } if (new_locale != NULL) { - pthread_setspecific(g_uselocale_key, new_locale); + *locale_storage = new_locale; } return old_locale; diff --git a/libc/bionic/mntent.cpp b/libc/bionic/mntent.cpp index 994b84d48..92284ce96 100644 --- a/libc/bionic/mntent.cpp +++ b/libc/bionic/mntent.cpp @@ -29,15 +29,11 @@ #include #include -#include "private/ThreadLocalBuffer.h" - -static ThreadLocalBuffer g_getmntent_mntent_tls_buffer; -static ThreadLocalBuffer g_getmntent_strings_tls_buffer; +#include "bionic/pthread_internal.h" mntent* getmntent(FILE* fp) { - return getmntent_r(fp, g_getmntent_mntent_tls_buffer.get(), - g_getmntent_strings_tls_buffer.get(), - g_getmntent_strings_tls_buffer.size()); + auto& tls = __get_bionic_tls(); + return getmntent_r(fp, &tls.mntent_buf, tls.mntent_strings, sizeof(tls.mntent_strings)); } mntent* getmntent_r(FILE* fp, struct mntent* e, char* buf, int buf_len) { diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp index ab9285368..f591c869d 100644 --- a/libc/bionic/pthread_create.cpp +++ b/libc/bionic/pthread_create.cpp @@ -55,6 +55,18 @@ void __init_tls(pthread_internal_t* thread) { // Slot 0 must point to itself. The x86 Linux kernel reads the TLS from %fs:0. thread->tls[TLS_SLOT_SELF] = thread->tls; thread->tls[TLS_SLOT_THREAD_ID] = thread; + + // Add a guard page before and after. + size_t allocation_size = BIONIC_TLS_SIZE + 2 * PAGE_SIZE; + void* allocation = mmap(nullptr, allocation_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (allocation == MAP_FAILED) { + __libc_fatal("failed to allocate TLS"); + } + + thread->bionic_tls = reinterpret_cast(static_cast(allocation) + PAGE_SIZE); + if (mprotect(thread->bionic_tls, BIONIC_TLS_SIZE, PROT_READ | PROT_WRITE) != 0) { + __libc_fatal("failed to mprotect TLS"); + } } void __init_thread_stack_guard(pthread_internal_t* thread) { diff --git a/libc/bionic/pthread_internal.cpp b/libc/bionic/pthread_internal.cpp index 5819bc114..2bc2bfbf0 100644 --- a/libc/bionic/pthread_internal.cpp +++ b/libc/bionic/pthread_internal.cpp @@ -86,6 +86,10 @@ void __pthread_internal_remove(pthread_internal_t* thread) { } static void __pthread_internal_free(pthread_internal_t* thread) { + // Unmap the TLS, including guard pages. + void* allocation = reinterpret_cast(thread->bionic_tls) - PAGE_SIZE; + munmap(allocation, BIONIC_TLS_SIZE + 2 * PAGE_SIZE); + if (thread->mmap_size != 0) { // Free mapped space, including thread stack and pthread_internal_t. munmap(thread->attr.stack_base, thread->mmap_size); diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h index d2abea0c1..b17029968 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -110,6 +110,8 @@ class pthread_internal_t { */ #define __BIONIC_DLERROR_BUFFER_SIZE 512 char dlerror_buffer[__BIONIC_DLERROR_BUFFER_SIZE]; + + bionic_tls* bionic_tls; }; __LIBC_HIDDEN__ int __init_thread(pthread_internal_t* thread); @@ -133,6 +135,10 @@ static inline __always_inline pthread_internal_t* __get_thread() { return nullptr; } +static inline __always_inline bionic_tls& __get_bionic_tls() { + return *__get_thread()->bionic_tls; +} + __LIBC_HIDDEN__ void pthread_key_clean_all(void); #if defined(__LP64__) diff --git a/libc/bionic/pty.cpp b/libc/bionic/pty.cpp index d699ff550..bdabf3617 100644 --- a/libc/bionic/pty.cpp +++ b/libc/bionic/pty.cpp @@ -36,10 +36,7 @@ #include #include -#include "private/ThreadLocalBuffer.h" - -static ThreadLocalBuffer g_ptsname_tls_buffer; -static ThreadLocalBuffer g_ttyname_tls_buffer; +#include "bionic/pthread_internal.h" int getpt() { return posix_openpt(O_RDWR|O_NOCTTY); @@ -54,8 +51,9 @@ int posix_openpt(int flags) { } char* ptsname(int fd) { - char* buf = g_ptsname_tls_buffer.get(); - int error = ptsname_r(fd, buf, g_ptsname_tls_buffer.size()); + bionic_tls& tls = __get_bionic_tls(); + char* buf = tls.ptsname_buf; + int error = ptsname_r(fd, buf, sizeof(tls.ptsname_buf)); return (error == 0) ? buf : NULL; } @@ -80,8 +78,9 @@ int ptsname_r(int fd, char* buf, size_t len) { } char* ttyname(int fd) { - char* buf = g_ttyname_tls_buffer.get(); - int error = ttyname_r(fd, buf, g_ttyname_tls_buffer.size()); + bionic_tls& tls = __get_bionic_tls(); + char* buf = tls.ttyname_buf; + int error = ttyname_r(fd, buf, sizeof(tls.ttyname_buf)); return (error == 0) ? buf : NULL; } diff --git a/libc/bionic/strerror.cpp b/libc/bionic/strerror.cpp index f74194fd9..99692ca0f 100644 --- a/libc/bionic/strerror.cpp +++ b/libc/bionic/strerror.cpp @@ -27,11 +27,10 @@ */ #include -#include "private/ThreadLocalBuffer.h" -extern "C" const char* __strerror_lookup(int); +#include "bionic/pthread_internal.h" -static ThreadLocalBuffer g_strerror_tls_buffer; +extern "C" const char* __strerror_lookup(int); char* strerror(int error_number) { // Just return the original constant in the easy cases. @@ -40,7 +39,8 @@ char* strerror(int error_number) { return result; } - result = g_strerror_tls_buffer.get(); - strerror_r(error_number, result, g_strerror_tls_buffer.size()); + bionic_tls& tls = __get_bionic_tls(); + result = tls.strerror_buf; + strerror_r(error_number, result, sizeof(tls.strerror_buf)); return result; } diff --git a/libc/bionic/strsignal.cpp b/libc/bionic/strsignal.cpp index c389ddd3b..81a8f95fe 100644 --- a/libc/bionic/strsignal.cpp +++ b/libc/bionic/strsignal.cpp @@ -27,13 +27,12 @@ */ #include -#include "private/ThreadLocalBuffer.h" + +#include "bionic/pthread_internal.h" extern "C" const char* __strsignal_lookup(int); extern "C" const char* __strsignal(int, char*, size_t); -static ThreadLocalBuffer g_strsignal_tls_buffer; - char* strsignal(int signal_number) { // Just return the original constant in the easy cases. char* result = const_cast(__strsignal_lookup(signal_number)); @@ -41,6 +40,6 @@ char* strsignal(int signal_number) { return result; } - return const_cast(__strsignal(signal_number, g_strsignal_tls_buffer.get(), - g_strsignal_tls_buffer.size())); + bionic_tls& tls = __get_bionic_tls(); + return const_cast(__strsignal(signal_number, tls.strsignal_buf, sizeof(tls.strsignal_buf))); } diff --git a/libc/private/bionic_tls.h b/libc/private/bionic_tls.h index c61e2ffdd..852b9aebc 100644 --- a/libc/private/bionic_tls.h +++ b/libc/private/bionic_tls.h @@ -29,10 +29,15 @@ #ifndef __BIONIC_PRIVATE_BIONIC_TLS_H_ #define __BIONIC_PRIVATE_BIONIC_TLS_H_ +#include +#include +#include #include +#include #include "bionic_macros.h" #include "__get_tls.h" +#include "grp_pwd.h" __BEGIN_DECLS @@ -77,6 +82,28 @@ enum { BIONIC_TLS_SLOTS // Must come last! }; +// ~3 pages. +struct bionic_tls { + locale_t locale; + + char basename_buf[MAXPATHLEN]; + char dirname_buf[MAXPATHLEN]; + + mntent mntent_buf; + char mntent_strings[BUFSIZ]; + + char ptsname_buf[32]; + char ttyname_buf[64]; + + char strerror_buf[NL_TEXTMAX]; + char strsignal_buf[NL_TEXTMAX]; + + group_state_t group; + passwd_state_t passwd; +}; + +#define BIONIC_TLS_SIZE (BIONIC_ALIGN(sizeof(bionic_tls), PAGE_SIZE)) + /* * Bionic uses some pthread keys internally. All pthread keys used internally * should be created in constructors, except for keys that may be used in or @@ -86,22 +113,10 @@ enum { * pthread_test should fail if we forget. * * These are the pthread keys currently used internally by libc: - * - * basename libc (ThreadLocalBuffer) - * dirname libc (ThreadLocalBuffer) - * uselocale libc (can be used in constructors) - * getmntent_mntent libc (ThreadLocalBuffer) - * getmntent_strings libc (ThreadLocalBuffer) - * ptsname libc (ThreadLocalBuffer) - * ttyname libc (ThreadLocalBuffer) - * strerror libc (ThreadLocalBuffer) - * strsignal libc (ThreadLocalBuffer) - * passwd libc (ThreadLocalBuffer) - * group libc (ThreadLocalBuffer) * _res_key libc (constructor in BSD code) */ -#define LIBC_PTHREAD_KEY_RESERVED_COUNT 12 +#define LIBC_PTHREAD_KEY_RESERVED_COUNT 1 /* Internally, jemalloc uses a single key for per thread data. */ #define JEMALLOC_PTHREAD_KEY_RESERVED_COUNT 1 diff --git a/libc/private/ThreadLocalBuffer.h b/libc/private/grp_pwd.h similarity index 60% rename from libc/private/ThreadLocalBuffer.h rename to libc/private/grp_pwd.h index 5e436659a..e1aff4f9e 100644 --- a/libc/private/ThreadLocalBuffer.h +++ b/libc/private/grp_pwd.h @@ -1,5 +1,7 @@ +#pragma once + /* - * Copyright (C) 2012 The Android Open Source Project + * Copyright (C) 2017 The Android Open Source Project * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -26,36 +28,21 @@ * SUCH DAMAGE. */ -#ifndef _BIONIC_THREAD_LOCAL_BUFFER_H_included -#define _BIONIC_THREAD_LOCAL_BUFFER_H_included - -#include -#include - -// TODO: use __thread instead? - -template -class ThreadLocalBuffer { - public: - ThreadLocalBuffer() { - // We used to use pthread_once to initialize the keys, but life is more predictable - // if we allocate them all up front when the C library starts up, via __constructor__. - pthread_key_create(&key_, free); - } +#include +#include - T* get() { - T* result = reinterpret_cast(pthread_getspecific(key_)); - if (result == nullptr) { - result = reinterpret_cast(calloc(1, Size)); - pthread_setspecific(key_, result); - } - return result; - } - - size_t size() { return Size; } - - private: - pthread_key_t key_; +struct group_state_t { + group group_; + char* group_members_[2]; + char group_name_buffer_[32]; + // Must be last so init_group_state can run a simple memset for the above + ssize_t getgrent_idx; }; -#endif // _BIONIC_THREAD_LOCAL_BUFFER_H_included +struct passwd_state_t { + passwd passwd_; + char name_buffer_[32]; + char dir_buffer_[32]; + char sh_buffer_[32]; + ssize_t getpwent_idx; +}; diff --git a/linker/dlfcn.cpp b/linker/dlfcn.cpp index 15e192251..5ccd65612 100644 --- a/linker/dlfcn.cpp +++ b/linker/dlfcn.cpp @@ -40,7 +40,6 @@ #include #include "private/bionic_tls.h" #include "private/ScopedPthreadMutexLocker.h" -#include "private/ThreadLocalBuffer.h" static pthread_mutex_t g_dl_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; -- 2.11.0