diff options
author | Josh Gao <jmgao@google.com> | 2016-05-17 19:23:39 -0700 |
---|---|---|
committer | gitbuildkicker <android-build@google.com> | 2016-08-16 15:52:46 -0700 |
commit | 11440395e379f35069e8e39a6e3ccfa7a7bfc72f (patch) | |
tree | 881f5726ca9e9eb73c37f84e9aac52a4f0b01b31 | |
parent | f2059d3fd959ce6f0d8a106ad4483c733aab3f66 (diff) | |
download | system_core-11440395e379f35069e8e39a6e3ccfa7a7bfc72f.zip system_core-11440395e379f35069e8e39a6e3ccfa7a7bfc72f.tar.gz system_core-11440395e379f35069e8e39a6e3ccfa7a7bfc72f.tar.bz2 |
adb: switch the socket list mutex to a recursive_mutex.
sockets.cpp was branching on whether a socket close function was
local_socket_close in order to avoid a potential deadlock if the socket
list lock was held while closing a peer socket.
Bug: http://b/28347842
Change-Id: I5e56f17fa54275284787f0f1dc150d1960256ab3
(functionally a cherrypick of 903b749f + 9b587dec, with windows disabled)
-rw-r--r-- | adb/Android.mk | 3 | ||||
-rw-r--r-- | adb/mutex_list.h | 1 | ||||
-rw-r--r-- | adb/sockets.cpp | 78 | ||||
-rw-r--r-- | adb/sysdeps/mutex.h | 143 |
4 files changed, 180 insertions, 45 deletions
diff --git a/adb/Android.mk b/adb/Android.mk index 425bf9b..d4adbfb 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -200,7 +200,10 @@ endif # will violate ODR LOCAL_SHARED_LIBRARIES := +# Don't build the host adb on Windows (this branch should only be used for security updates.) +ifneq ($(HOST_OS),windows) include $(BUILD_HOST_EXECUTABLE) +endif $(call dist-for-goals,dist_files sdk,$(LOCAL_BUILT_MODULE)) diff --git a/adb/mutex_list.h b/adb/mutex_list.h index ff72751..15e383c 100644 --- a/adb/mutex_list.h +++ b/adb/mutex_list.h @@ -6,7 +6,6 @@ #ifndef ADB_MUTEX #error ADB_MUTEX not defined when including this file #endif -ADB_MUTEX(socket_list_lock) ADB_MUTEX(transport_lock) #if ADB_HOST ADB_MUTEX(local_transports_lock) diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 32ca17d..31b5443 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -25,18 +25,27 @@ #include <string.h> #include <unistd.h> +#include <algorithm> +#include <mutex> +#include <string> +#include <vector> + #if !ADB_HOST #include "cutils/properties.h" #endif #include "adb.h" #include "adb_io.h" +#include "sysdeps/mutex.h" #include "transport.h" -ADB_MUTEX_DEFINE( socket_list_lock ); +#if !defined(__BIONIC__) +using std::recursive_mutex; +#endif -static void local_socket_close_locked(asocket *s); +static void local_socket_close(asocket* s); +static recursive_mutex& local_socket_list_lock = *new recursive_mutex(); static unsigned local_socket_next_id = 1; static asocket local_socket_list = { @@ -61,7 +70,7 @@ asocket *find_local_socket(unsigned local_id, unsigned peer_id) asocket *s; asocket *result = NULL; - adb_mutex_lock(&socket_list_lock); + std::lock_guard<recursive_mutex> lock(local_socket_list_lock); for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { if (s->id != local_id) continue; @@ -70,7 +79,6 @@ asocket *find_local_socket(unsigned local_id, unsigned peer_id) } break; } - adb_mutex_unlock(&socket_list_lock); return result; } @@ -84,20 +92,17 @@ insert_local_socket(asocket* s, asocket* list) s->next->prev = s; } - -void install_local_socket(asocket *s) -{ - adb_mutex_lock(&socket_list_lock); +void install_local_socket(asocket* s) { + std::lock_guard<recursive_mutex> lock(local_socket_list_lock); s->id = local_socket_next_id++; // Socket ids should never be 0. - if (local_socket_next_id == 0) - local_socket_next_id = 1; + if (local_socket_next_id == 0) { + fatal("local socket id overflow"); + } insert_local_socket(s, &local_socket_list); - - adb_mutex_unlock(&socket_list_lock); } void remove_socket(asocket *s) @@ -116,19 +121,17 @@ void remove_socket(asocket *s) void close_all_sockets(atransport *t) { asocket *s; - - /* this is a little gross, but since s->close() *will* modify - ** the list out from under you, your options are limited. - */ - adb_mutex_lock(&socket_list_lock); + /* this is a little gross, but since s->close() *will* modify + ** the list out from under you, your options are limited. + */ + std::lock_guard<recursive_mutex> lock(local_socket_list_lock); restart: - for(s = local_socket_list.next; s != &local_socket_list; s = s->next){ - if(s->transport == t || (s->peer && s->peer->transport == t)) { - local_socket_close_locked(s); + for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { + if (s->transport == t || (s->peer && s->peer->transport == t)) { + local_socket_close(s); goto restart; } } - adb_mutex_unlock(&socket_list_lock); } static int local_socket_enqueue(asocket *s, apacket *p) @@ -191,13 +194,6 @@ static void local_socket_ready(asocket *s) fdevent_add(&s->fde, FDE_READ); } -static void local_socket_close(asocket *s) -{ - adb_mutex_lock(&socket_list_lock); - local_socket_close_locked(s); - adb_mutex_unlock(&socket_list_lock); -} - // be sure to hold the socket list lock when calling this static void local_socket_destroy(asocket *s) { @@ -226,27 +222,21 @@ static void local_socket_destroy(asocket *s) } } - -static void local_socket_close_locked(asocket *s) -{ - D("entered local_socket_close_locked. LS(%d) fd=%d\n", s->id, s->fd); - if(s->peer) { - D("LS(%d): closing peer. peer->id=%d peer->fd=%d\n", - s->id, s->peer->id, s->peer->fd); +static void local_socket_close(asocket* s) { + D("entered local_socket_close. LS(%d) fd=%d", s->id, s->fd); + std::lock_guard<recursive_mutex> lock(local_socket_list_lock); + if (s->peer) { + D("LS(%d): closing peer. peer->id=%d peer->fd=%d", s->id, s->peer->id, s->peer->fd); /* Note: it's important to call shutdown before disconnecting from * the peer, this ensures that remote sockets can still get the id * of the local socket they're connected to, to send a CLOSE() * protocol event. */ - if (s->peer->shutdown) - s->peer->shutdown(s->peer); - s->peer->peer = 0; - // tweak to avoid deadlock - if (s->peer->close == local_socket_close) { - local_socket_close_locked(s->peer); - } else { - s->peer->close(s->peer); + if (s->peer->shutdown) { + s->peer->shutdown(s->peer); } - s->peer = 0; + s->peer->peer = nullptr; + s->peer->close(s->peer); + s->peer = nullptr; } /* If we are already closing, or if there are no diff --git a/adb/sysdeps/mutex.h b/adb/sysdeps/mutex.h new file mode 100644 index 0000000..ef5d9b1 --- /dev/null +++ b/adb/sysdeps/mutex.h @@ -0,0 +1,143 @@ +#pragma once + +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if defined(_WIN32) + +#include <windows.h> + +#include <android-base/macros.h> + +#include "adb.h" + +// The prebuilt version of mingw we use doesn't support mutex or recursive_mutex. +// Therefore, implement our own using the Windows primitives. +// Put them directly into the std namespace, so that when they're actually available, the build +// breaks until they're removed. + +#include <mutex> +namespace std { + +// CRITICAL_SECTION is recursive, so just wrap it in a Mutex-compatible class. +class recursive_mutex { + public: + recursive_mutex() { + InitializeCriticalSection(&mutex_); + } + + ~recursive_mutex() { + DeleteCriticalSection(&mutex_); + } + + void lock() { + EnterCriticalSection(&mutex_); + } + + bool try_lock() { + return TryEnterCriticalSection(&mutex_); + } + + void unlock() { + LeaveCriticalSection(&mutex_); + } + + private: + CRITICAL_SECTION mutex_; + + DISALLOW_COPY_AND_ASSIGN(recursive_mutex); +}; + +class mutex { + public: + mutex() { + } + + ~mutex() { + } + + void lock() { + mutex_.lock(); + if (++lock_count_ != 1) { + fatal("non-recursive mutex locked reentrantly"); + } + } + + void unlock() { + if (--lock_count_ != 0) { + fatal("non-recursive mutex unlock resulted in unexpected lock count: %d", lock_count_); + } + mutex_.unlock(); + } + + bool try_lock() { + if (!mutex_.try_lock()) { + return false; + } + + if (lock_count_ != 0) { + mutex_.unlock(); + return false; + } + + ++lock_count_; + return true; + } + + private: + recursive_mutex mutex_; + size_t lock_count_ = 0; +}; + +} + +#elif defined(__BIONIC__) + +// On M, the recovery image uses parts of adb that depends on recursive_mutex, and uses libstdc++, +// which lacks it. + +#include <pthread.h> +#include <mutex> + +#include <base/macros.h> + +class recursive_mutex { + public: + recursive_mutex() { + } + + ~recursive_mutex() { + } + + void lock() { + pthread_mutex_lock(&mutex_); + } + + bool try_lock() { + return pthread_mutex_trylock(&mutex_); + } + + void unlock() { + pthread_mutex_unlock(&mutex_); + } + + private: + pthread_mutex_t mutex_ = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; + + DISALLOW_COPY_AND_ASSIGN(recursive_mutex); +}; + +#endif |