ACPR/android_system_core
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

adb: switch the socket list mutex to a recursive_mutex.

Browse source 
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
This commit is contained in:
Josh Gao 2016-05-17 17:46:27 -07:00
parent 52bd8526aa
commit 9b587dec6d
2 changed files with 15 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
adb/mutex_list.h
View file

@ -8,7 +8,6 @@
#endif #endif
ADB_MUTEX(basename_lock) ADB_MUTEX(basename_lock)
ADB_MUTEX(dirname_lock) ADB_MUTEX(dirname_lock)
ADB_MUTEX(socket_list_lock)
ADB_MUTEX(transport_lock) ADB_MUTEX(transport_lock)
#if ADB_HOST #if ADB_HOST
ADB_MUTEX(local_transports_lock) ADB_MUTEX(local_transports_lock)

43
adb/sockets.cpp
View file

@ -26,6 +26,7 @@
#include <unistd.h> #include <unistd.h>
#include <algorithm> #include <algorithm>
#include <mutex>
#include <string> #include <string>
#include <vector> #include <vector>
@ -35,12 +36,12 @@
#include "adb.h" #include "adb.h"
#include "adb_io.h" #include "adb_io.h"
#include "sysdeps/mutex.h"
#include "transport.h" #include "transport.h"
ADB_MUTEX_DEFINE(socket_list_lock); static void local_socket_close(asocket* s);
static void local_socket_close_locked(asocket* s);
static std::recursive_mutex& local_socket_list_lock = *new std::recursive_mutex();
static unsigned local_socket_next_id = 1; static unsigned local_socket_next_id = 1;
static asocket local_socket_list = { static asocket local_socket_list = {
@ -62,7 +63,7 @@ asocket* find_local_socket(unsigned local_id, unsigned peer_id) {
asocket* s; asocket* s;
asocket* result = NULL; asocket* result = NULL;
adb_mutex_lock(&socket_list_lock); std::lock_guard<std::recursive_mutex> lock(local_socket_list_lock);
for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { for (s = local_socket_list.next; s != &local_socket_list; s = s->next) {
if (s->id != local_id) { if (s->id != local_id) {
continue; continue;
@ -72,7 +73,6 @@ asocket* find_local_socket(unsigned local_id, unsigned peer_id) {
} }
break; break;
} }
adb_mutex_unlock(&socket_list_lock);
return result; return result;
} }
@ -85,18 +85,16 @@ static void insert_local_socket(asocket* s, asocket* list) {
} }
void install_local_socket(asocket* s) { void install_local_socket(asocket* s) {
adb_mutex_lock(&socket_list_lock); std::lock_guard<std::recursive_mutex> lock(local_socket_list_lock);
s->id = local_socket_next_id++; s->id = local_socket_next_id++;
// Socket ids should never be 0. // Socket ids should never be 0.
if (local_socket_next_id == 0) { if (local_socket_next_id == 0) {
local_socket_next_id = 1; fatal("local socket id overflow");
} }
insert_local_socket(s, &local_socket_list); insert_local_socket(s, &local_socket_list);
adb_mutex_unlock(&socket_list_lock);
} }
void remove_socket(asocket* s) { void remove_socket(asocket* s) {
@ -116,15 +114,14 @@ void close_all_sockets(atransport* t) {
/* this is a little gross, but since s->close() *will* modify /* this is a little gross, but since s->close() *will* modify
** the list out from under you, your options are limited. ** the list out from under you, your options are limited.
*/ */
adb_mutex_lock(&socket_list_lock); std::lock_guard<std::recursive_mutex> lock(local_socket_list_lock);
restart: restart:
for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { for (s = local_socket_list.next; s != &local_socket_list; s = s->next) {
if (s->transport == t || (s->peer && s->peer->transport == t)) { if (s->transport == t || (s->peer && s->peer->transport == t)) {
local_socket_close_locked(s); local_socket_close(s);
goto restart; goto restart;
} }
} }
adb_mutex_unlock(&socket_list_lock);
} }
static int local_socket_enqueue(asocket* s, apacket* p) { static int local_socket_enqueue(asocket* s, apacket* p) {
@ -187,12 +184,6 @@ static void local_socket_ready(asocket* s) {
fdevent_add(&s->fde, FDE_READ); 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 // be sure to hold the socket list lock when calling this
static void local_socket_destroy(asocket* s) { static void local_socket_destroy(asocket* s) {
apacket *p, *n; apacket *p, *n;
@ -220,8 +211,9 @@ static void local_socket_destroy(asocket* s) {
} }
} }
static void local_socket_close_locked(asocket* s) { static void local_socket_close(asocket* s) {
D("entered local_socket_close_locked. LS(%d) fd=%d", s->id, s->fd); D("entered local_socket_close. LS(%d) fd=%d", s->id, s->fd);
std::lock_guard<std::recursive_mutex> lock(local_socket_list_lock);
if (s->peer) { if (s->peer) {
D("LS(%d): closing peer. peer->id=%d peer->fd=%d", s->id, s->peer->id, s->peer->fd); 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 /* Note: it's important to call shutdown before disconnecting from
@ -231,14 +223,9 @@ static void local_socket_close_locked(asocket* s) {
if (s->peer->shutdown) { if (s->peer->shutdown) {
s->peer->shutdown(s->peer); s->peer->shutdown(s->peer);
} }
s->peer->peer = 0; s->peer->peer = nullptr;
// tweak to avoid deadlock s->peer->close(s->peer);
if (s->peer->close == local_socket_close) { s->peer = nullptr;
local_socket_close_locked(s->peer);
} else {
s->peer->close(s->peer);
}
s->peer = 0;
} }
/* If we are already closing, or if there are no /* If we are already closing, or if there are no