From b7b1edf974a93cc4bb9a2de7a5e9c9bce9ad178b Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 11 Nov 2015 17:56:12 -0800 Subject: [PATCH 1/2] adb: remove exit-time destructors. On exit, these destructors get invoked while other threads might still be using them, potentially causing a crash, and definitely causing tsan to report a race condition. Bug: http://b/23384853 Change-Id: I94de55d22f97f4edd1d7cc1f34e8c1f8dfd56a5a --- adb/Android.mk | 25 +++++++++++++++++++++++-- adb/commandline.cpp | 2 +- adb/fdevent.cpp | 4 ++-- adb/remount_service.cpp | 4 +--- adb/sysdeps_win32.cpp | 4 ++-- adb/transport.cpp | 4 ++-- adb/usb_linux.cpp | 4 ++-- 7 files changed, 33 insertions(+), 14 deletions(-) diff --git a/adb/Android.mk b/adb/Android.mk index 903d1e15a..55231f2c8 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -17,6 +17,14 @@ ADB_COMMON_CFLAGS := \ -Wvla \ -DADB_REVISION='"$(adb_version)"' \ +ADB_COMMON_linux_CFLAGS := \ + -std=c++14 \ + -Wexit-time-destructors \ + +ADB_COMMON_darwin_CFLAGS := \ + -std=c++14 \ + -Wexit-time-destructors \ + # Define windows.h and tchar.h Unicode preprocessor symbols so that # CreateFile(), _tfopen(), etc. map to versions that take wchar_t*, breaking the # build if you accidentally pass char*. Fix by calling like: @@ -55,7 +63,10 @@ LIBADB_CFLAGS := \ -fvisibility=hidden \ LIBADB_linux_CFLAGS := \ - -std=c++14 \ + $(ADB_COMMON_linux_CFLAGS) \ + +LIBADB_darwin_CFLAGS := \ + $(ADB_COMMON_darwin_CFLAGS) \ LIBADB_windows_CFLAGS := \ $(ADB_COMMON_windows_CFLAGS) \ @@ -110,6 +121,7 @@ LOCAL_MODULE_HOST_OS := darwin linux windows LOCAL_CFLAGS := $(LIBADB_CFLAGS) -DADB_HOST=1 LOCAL_CFLAGS_windows := $(LIBADB_windows_CFLAGS) LOCAL_CFLAGS_linux := $(LIBADB_linux_CFLAGS) +LOCAL_CFLAGS_darwin := $(LIBADB_darwin_CFLAGS) LOCAL_SRC_FILES := \ $(LIBADB_SRC_FILES) \ adb_auth_host.cpp \ @@ -155,6 +167,7 @@ LOCAL_MODULE_HOST_OS := darwin linux windows LOCAL_CFLAGS := -DADB_HOST=1 $(LIBADB_CFLAGS) LOCAL_CFLAGS_windows := $(LIBADB_windows_CFLAGS) LOCAL_CFLAGS_linux := $(LIBADB_linux_CFLAGS) +LOCAL_CFLAGS_darwin := $(LIBADB_darwin_CFLAGS) LOCAL_SRC_FILES := \ $(LIBADB_TEST_SRCS) \ services.cpp \ @@ -189,6 +202,7 @@ LOCAL_MODULE := adb_device_tracker_test LOCAL_CFLAGS := -DADB_HOST=1 $(LIBADB_CFLAGS) LOCAL_CFLAGS_windows := $(LIBADB_windows_CFLAGS) LOCAL_CFLAGS_linux := $(LIBADB_linux_CFLAGS) +LOCAL_CFLAGS_darwin := $(LIBADB_darwin_CFLAGS) LOCAL_SRC_FILES := test_track_devices.cpp LOCAL_SANITIZE := $(adb_host_sanitize) LOCAL_SHARED_LIBRARIES := libbase @@ -204,7 +218,6 @@ include $(CLEAR_VARS) LOCAL_LDLIBS_linux := -lrt -ldl -lpthread LOCAL_LDLIBS_darwin := -lpthread -framework CoreFoundation -framework IOKit -framework Carbon -LOCAL_CFLAGS_darwin := -Wno-sizeof-pointer-memaccess -Wno-unused-parameter # Use wmain instead of main LOCAL_LDFLAGS_windows := -municode @@ -230,6 +243,13 @@ LOCAL_CFLAGS += \ LOCAL_CFLAGS_windows := \ $(ADB_COMMON_windows_CFLAGS) +LOCAL_CFLAGS_linux := \ + $(ADB_COMMON_linux_CFLAGS) \ + +LOCAL_CFLAGS_darwin := \ + $(ADB_COMMON_darwin_CFLAGS) \ + -Wno-sizeof-pointer-memaccess -Wno-unused-parameter \ + LOCAL_MODULE := adb LOCAL_MODULE_TAGS := debug LOCAL_MODULE_HOST_OS := darwin linux windows @@ -273,6 +293,7 @@ LOCAL_SRC_FILES := \ LOCAL_CFLAGS := \ $(ADB_COMMON_CFLAGS) \ + $(ADB_COMMON_linux_CFLAGS) \ -DADB_HOST=0 \ -D_GNU_SOURCE \ -Wno-deprecated-declarations \ diff --git a/adb/commandline.cpp b/adb/commandline.cpp index 6e4c4e8ff..abbc43d9c 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -62,7 +62,7 @@ static int uninstall_app(TransportType t, const char* serial, int argc, const ch static int install_app_legacy(TransportType t, const char* serial, int argc, const char** argv); static int uninstall_app_legacy(TransportType t, const char* serial, int argc, const char** argv); -static std::string gProductOutPath; +static auto& gProductOutPath = *new std::string(); extern int gListenAll; static std::string product_file(const char *extra) { diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index 06eb34dc2..46547b91f 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -70,8 +70,8 @@ struct PollNode { // All operations to fdevent should happen only in the main thread. // That's why we don't need a lock for fdevent. -static std::unordered_map g_poll_node_map; -static std::list g_pending_list; +static auto& g_poll_node_map = *new std::unordered_map(); +static auto& g_pending_list = *new std::list(); static bool main_thread_valid; static pthread_t main_thread; diff --git a/adb/remount_service.cpp b/adb/remount_service.cpp index 35ba0566e..8f1c9b015 100644 --- a/adb/remount_service.cpp +++ b/adb/remount_service.cpp @@ -35,8 +35,6 @@ #include "cutils/properties.h" #include "fs_mgr.h" -const std::string kFstab_Prefix = "/fstab."; - // Returns the device used to mount a directory in /proc/mounts. static std::string find_proc_mount(const char* dir) { std::unique_ptr fp(setmntent("/proc/mounts", "r"), endmntent); @@ -58,7 +56,7 @@ static std::string find_fstab_mount(const char* dir) { char propbuf[PROPERTY_VALUE_MAX]; property_get("ro.hardware", propbuf, ""); - std::string fstab_filename = kFstab_Prefix + propbuf; + std::string fstab_filename = std::string("/fstab.") + propbuf; struct fstab* fstab = fs_mgr_read_fstab(fstab_filename.c_str()); struct fstab_rec* rec = fs_mgr_get_entry_for_mount_point(fstab, dir); std::string dev = rec ? std::string(rec->blk_device) : ""; diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index beaca16a5..4d59adb20 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -2924,7 +2924,7 @@ size_t _escape_prefix(char* const buf, const size_t len) { } // Internal buffer to satisfy future _console_read() calls. -static std::vector g_console_input_buffer; +static auto& g_console_input_buffer = *new std::vector(); // Writes to buffer buf (of length len), returning number of bytes written or -1 on error. Never // returns zero on console closure because Win32 consoles are never 'closed' (as far as I can tell). @@ -3886,7 +3886,7 @@ extern "C" int wmain(int argc, wchar_t **argv) { // currently updated if putenv, setenv, unsetenv are called. Note that no // thread synchronization is done, but we're called early enough in // single-threaded startup that things work ok. -static std::unordered_map g_environ_utf8; +static auto& g_environ_utf8 = *new std::unordered_map(); // Make sure that shadow UTF-8 environment variables are setup. static void _ensure_env_setup() { diff --git a/adb/transport.cpp b/adb/transport.cpp index 406688999..2f18f2011 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -38,8 +38,8 @@ static void transport_unref(atransport *t); -static std::list transport_list; -static std::list pending_list; +static auto& transport_list = *new std::list(); +static auto& pending_list = *new std::list(); ADB_MUTEX_DEFINE( transport_lock ); diff --git a/adb/usb_linux.cpp b/adb/usb_linux.cpp index c633f7fb8..0358b6203 100644 --- a/adb/usb_linux.cpp +++ b/adb/usb_linux.cpp @@ -81,8 +81,8 @@ struct usb_handle { pthread_t reaper_thread = 0; }; -static std::mutex g_usb_handles_mutex; -static std::list g_usb_handles; +static auto& g_usb_handles_mutex = *new std::mutex(); +static auto& g_usb_handles = *new std::list(); static int is_known_device(const char* dev_name) { std::lock_guard lock(g_usb_handles_mutex); From 7df6b5fc79a0f4521d171280cd431640ac567d27 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 12 Nov 2015 11:54:47 -0800 Subject: [PATCH 2/2] libbase: remove exit-time destructors. Removed for the same reason as the adb exit-time destructors. Bug: http://b/23384853 Change-Id: Ic124ecb9df132b850a3855e207baffec926dde29 --- base/Android.mk | 10 +++++++++- base/logging.cpp | 8 ++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/base/Android.mk b/base/Android.mk index 613636b61..5a0ad05c3 100644 --- a/base/Android.mk +++ b/base/Android.mk @@ -36,6 +36,12 @@ libbase_cppflags := \ -Wextra \ -Werror \ +libbase_linux_cppflags := \ + -Wexit-time-destructors \ + +libbase_darwin_cppflags := \ + -Wexit-time-destructors \ + # Device # ------------------------------------------------------------------------------ include $(CLEAR_VARS) @@ -43,7 +49,7 @@ LOCAL_MODULE := libbase LOCAL_CLANG := true LOCAL_SRC_FILES := $(libbase_src_files) LOCAL_C_INCLUDES := $(LOCAL_PATH)/include -LOCAL_CPPFLAGS := $(libbase_cppflags) +LOCAL_CPPFLAGS := $(libbase_cppflags) $(libbase_linux_cppflags) LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include LOCAL_STATIC_LIBRARIES := libcutils LOCAL_MULTILIB := both @@ -66,6 +72,8 @@ LOCAL_MODULE := libbase LOCAL_SRC_FILES := $(libbase_src_files) LOCAL_C_INCLUDES := $(LOCAL_PATH)/include LOCAL_CPPFLAGS := $(libbase_cppflags) +LOCAL_CPPFLAGS_darwin := $(libbase_darwin_cppflags) +LOCAL_CPPFLAGS_linux := $(libbase_linux_cppflags) LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include LOCAL_STATIC_LIBRARIES := libcutils LOCAL_MULTILIB := both diff --git a/base/logging.cpp b/base/logging.cpp index 248cd0617..d88455fcb 100644 --- a/base/logging.cpp +++ b/base/logging.cpp @@ -127,17 +127,17 @@ class lock_guard { namespace android { namespace base { -static mutex logging_lock; +static auto& logging_lock = *new mutex(); #ifdef __ANDROID__ -static LogFunction gLogger = LogdLogger(); +static auto& gLogger = *new LogFunction(LogdLogger()); #else -static LogFunction gLogger = StderrLogger; +static auto& gLogger = *new LogFunction(StderrLogger); #endif static bool gInitialized = false; static LogSeverity gMinimumLogSeverity = INFO; -static std::unique_ptr gProgramInvocationName; +static auto& gProgramInvocationName = *new std::unique_ptr(); LogSeverity GetMinimumLogSeverity() { return gMinimumLogSeverity;