From d51c6df1ef98f2b57916ddefc80afda5f1eecb59 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 23 Feb 2018 14:00:24 -0800 Subject: [PATCH 1/2] adb: stop using quick_exit. We don't actually need to use quick_exit to avoid calling static destructors, since we have -Wexit-time-destructors to guarantee we don't actually have any, and this precludes the use of asan's exit time leak checking, so switch back to atexit/exit. Test: ASAN_OPTIONS=detect_leaks=1:leak_check_at_exit=1 adb server nodaemon with a manually inserted leak Change-Id: Id8178913f64cb02c820c5073351369a9e4d8c74d --- adb/adb.cpp | 3 +-- adb/client/main.cpp | 7 +++---- adb/client/usb_libusb.cpp | 2 +- adb/transport.cpp | 1 - 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index ae8020e81..c4df5c4b9 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -42,7 +42,6 @@ #include #include #include -#include #include #include @@ -1059,7 +1058,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser SendOkay(reply_fd); // Rely on process exit to close the socket for us. - android::base::quick_exit(0); + exit(0); } // "transport:" is used for switching transport with a specified serial number diff --git a/adb/client/main.cpp b/adb/client/main.cpp index f0d0ce799..f28302b61 100644 --- a/adb/client/main.cpp +++ b/adb/client/main.cpp @@ -28,7 +28,6 @@ #include #include #include -#include #include #include "adb.h" @@ -61,7 +60,7 @@ static void setup_daemon_logging() { static BOOL WINAPI ctrlc_handler(DWORD type) { // TODO: Consider trying to kill a starting up adb server (if we're in // launch_server) by calling GenerateConsoleCtrlEvent(). - android::base::quick_exit(STATUS_CONTROL_C_EXIT); + exit(STATUS_CONTROL_C_EXIT); return TRUE; } #endif @@ -95,7 +94,7 @@ int adb_server_main(int is_daemon, const std::string& socket_spec, int ack_reply SetConsoleCtrlHandler(ctrlc_handler, TRUE); #else signal(SIGINT, [](int) { - fdevent_run_on_main_thread([]() { android::base::quick_exit(0); }); + fdevent_run_on_main_thread([]() { exit(0); }); }); #endif @@ -104,7 +103,7 @@ int adb_server_main(int is_daemon, const std::string& socket_spec, int ack_reply setup_daemon_logging(); } - android::base::at_quick_exit(adb_server_cleanup); + atexit(adb_server_cleanup); init_transport_registration(); init_mdns_transport_discovery(); diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp index 18f585d88..46c3f58ec 100644 --- a/adb/client/usb_libusb.cpp +++ b/adb/client/usb_libusb.cpp @@ -19,6 +19,7 @@ #include "sysdeps.h" #include +#include #include #include @@ -33,7 +34,6 @@ #include #include -#include #include #include diff --git a/adb/transport.cpp b/adb/transport.cpp index 14888ab70..6b1a00bf1 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -34,7 +34,6 @@ #include #include -#include #include #include #include From e39ccd3cbd67833ce8571710d39fe9fb375880c4 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 23 Feb 2018 14:37:07 -0800 Subject: [PATCH 2/2] adb: allow reentrant calls to fdevent_run_on_main_thread. Previously, reentrant calls to fdevent_run_on_main_thread would deadlock. Test: adb_test on host Change-Id: I0783be0558dcaf61ddbe76d13ac6917fc2de0be0 --- adb/fdevent.cpp | 44 ++++++++++++++++++++++++++++---------------- adb/fdevent_test.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index b28de4b52..d2855619b 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -81,7 +82,7 @@ static unsigned long main_thread_id; static auto& run_queue_notify_fd = *new unique_fd(); static auto& run_queue_mutex = *new std::mutex(); -static auto& run_queue GUARDED_BY(run_queue_mutex) = *new std::vector>(); +static auto& run_queue GUARDED_BY(run_queue_mutex) = *new std::deque>(); void check_main_thread() { if (main_thread_valid) { @@ -359,11 +360,21 @@ static void fdevent_subproc_setup() { } #endif // !ADB_HOST -static void fdevent_run_flush() REQUIRES(run_queue_mutex) { - for (auto& f : run_queue) { - f(); +static void fdevent_run_flush() EXCLUDES(run_queue_mutex) { + // We need to be careful around reentrancy here, since a function we call can queue up another + // function. + while (true) { + std::function fn; + { + std::lock_guard lock(run_queue_mutex); + if (run_queue.empty()) { + break; + } + fn = run_queue.front(); + run_queue.pop_front(); + } + fn(); } - run_queue.clear(); } static void fdevent_run_func(int fd, unsigned ev, void* /* userdata */) { @@ -377,22 +388,23 @@ static void fdevent_run_func(int fd, unsigned ev, void* /* userdata */) { PLOG(FATAL) << "failed to empty run queue notify fd"; } - std::lock_guard lock(run_queue_mutex); fdevent_run_flush(); } static void fdevent_run_setup() { - std::lock_guard lock(run_queue_mutex); - CHECK(run_queue_notify_fd.get() == -1); - int s[2]; - if (adb_socketpair(s) != 0) { - PLOG(FATAL) << "failed to create run queue notify socketpair"; - } + { + std::lock_guard lock(run_queue_mutex); + CHECK(run_queue_notify_fd.get() == -1); + int s[2]; + if (adb_socketpair(s) != 0) { + PLOG(FATAL) << "failed to create run queue notify socketpair"; + } - run_queue_notify_fd.reset(s[0]); - fdevent* fde = fdevent_create(s[1], fdevent_run_func, nullptr); - CHECK(fde != nullptr); - fdevent_add(fde, FDE_READ); + run_queue_notify_fd.reset(s[0]); + fdevent* fde = fdevent_create(s[1], fdevent_run_func, nullptr); + CHECK(fde != nullptr); + fdevent_add(fde, FDE_READ); + } fdevent_run_flush(); } diff --git a/adb/fdevent_test.cpp b/adb/fdevent_test.cpp index 86e020957..63cc4d176 100644 --- a/adb/fdevent_test.cpp +++ b/adb/fdevent_test.cpp @@ -194,3 +194,31 @@ TEST_F(FdeventTest, run_on_main_thread) { ASSERT_EQ(i, vec[i]); } } + +static std::function make_appender(std::vector* vec, int value) { + return [vec, value]() { + check_main_thread(); + if (value == 100) { + return; + } + + vec->push_back(value); + fdevent_run_on_main_thread(make_appender(vec, value + 1)); + }; +} + +TEST_F(FdeventTest, run_on_main_thread_reentrant) { + std::vector vec; + + PrepareThread(); + std::thread thread(fdevent_loop); + + fdevent_run_on_main_thread(make_appender(&vec, 0)); + + TerminateThread(thread); + + ASSERT_EQ(100u, vec.size()); + for (int i = 0; i < 100; ++i) { + ASSERT_EQ(i, vec[i]); + } +}