From 2744084981f29726ddccc64220a191002a772a32 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 14 May 2018 11:16:02 -0700 Subject: [PATCH 1/5] adb: fix uninitialized variable in AsyncServiceRef. Bug: none Test: treehugger Change-Id: I8ba0a70a772f88bfd701730a48d4eb32c6677b9e --- adb/client/transport_mdns.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index 3603f0974..01f140a98 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -87,7 +87,7 @@ class AsyncServiceRef { } private: - bool initialized_; + bool initialized_ = false; fdevent fde_; }; From 71f775a9448d483e031767f0a1afb5ef104a36d0 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 14 May 2018 11:14:33 -0700 Subject: [PATCH 2/5] adb: remove fdevent_install, fdevent_remove. Remove fdevent_install and fdevent_remove in favor of using fdevent_create and fdevent_destroy, so that we can put RAII types (i.e. unique_fd) into fdevent without worrying about -Wexit-time-destructors or structs that are freed instead of deleted. Bug: http://b/79786774 Test: python test_device.py Change-Id: I8471cc00574ed492fe1b196944976cdaae8b7cff --- adb/adb_listeners.cpp | 10 ++++----- adb/client/transport_mdns.cpp | 27 +++++++++++------------ adb/daemon/auth.cpp | 18 +++++++++------- adb/fdevent.cpp | 40 ++++++++++++++++------------------- adb/fdevent.h | 25 +++++++--------------- adb/fdevent_test.cpp | 30 +++++++++++++------------- adb/socket.h | 4 ++-- adb/sockets.cpp | 26 +++++++++++------------ adb/transport.cpp | 9 ++++---- 9 files changed, 87 insertions(+), 102 deletions(-) diff --git a/adb/adb_listeners.cpp b/adb/adb_listeners.cpp index fecf452c1..ea5a44e46 100644 --- a/adb/adb_listeners.cpp +++ b/adb/adb_listeners.cpp @@ -42,7 +42,7 @@ class alistener { alistener(const std::string& _local_name, const std::string& _connect_to); ~alistener(); - fdevent fde; + fdevent* fde = nullptr; int fd = -1; std::string local_name; @@ -60,7 +60,7 @@ alistener::alistener(const std::string& _local_name, const std::string& _connect alistener::~alistener() { // Closes the corresponding fd. - fdevent_remove(&fde); + fdevent_destroy(fde); if (transport) { transport->RemoveDisconnect(&disconnect); @@ -222,11 +222,11 @@ InstallStatus install_listener(const std::string& local_name, const char* connec close_on_exec(listener->fd); if (listener->connect_to == "*smartsocket*") { - fdevent_install(&listener->fde, listener->fd, ss_listener_event_func, listener.get()); + listener->fde = fdevent_create(listener->fd, ss_listener_event_func, listener.get()); } else { - fdevent_install(&listener->fde, listener->fd, listener_event_func, listener.get()); + listener->fde = fdevent_create(listener->fd, listener_event_func, listener.get()); } - fdevent_set(&listener->fde, FDE_READ); + fdevent_set(listener->fde, FDE_READ); listener->transport = transport; diff --git a/adb/client/transport_mdns.cpp b/adb/client/transport_mdns.cpp index 01f140a98..283fac554 100644 --- a/adb/client/transport_mdns.cpp +++ b/adb/client/transport_mdns.cpp @@ -35,7 +35,7 @@ #include "sysdeps.h" static DNSServiceRef service_ref; -static fdevent service_ref_fde; +static fdevent* service_ref_fde; // Use adb_DNSServiceRefSockFD() instead of calling DNSServiceRefSockFD() // directly so that the socket is put through the appropriate compatibility @@ -68,27 +68,26 @@ class AsyncServiceRef { } virtual ~AsyncServiceRef() { - if (! initialized_) { + if (!initialized_) { return; } DNSServiceRefDeallocate(sdRef_); - fdevent_remove(&fde_); + fdevent_destroy(fde_); } protected: DNSServiceRef sdRef_; void Initialize() { - fdevent_install(&fde_, adb_DNSServiceRefSockFD(sdRef_), - pump_service_ref, &sdRef_); - fdevent_set(&fde_, FDE_READ); + fde_ = fdevent_create(adb_DNSServiceRefSockFD(sdRef_), pump_service_ref, &sdRef_); + fdevent_set(fde_, FDE_READ); initialized_ = true; } private: bool initialized_ = false; - fdevent fde_; + fdevent* fde_; }; class ResolvedService : public AsyncServiceRef { @@ -252,14 +251,12 @@ static void DNSSD_API register_mdns_transport(DNSServiceRef sdRef, if (errorCode != kDNSServiceErr_NoError) { D("Got error %d during mDNS browse.", errorCode); DNSServiceRefDeallocate(sdRef); - fdevent_remove(&service_ref_fde); + fdevent_destroy(service_ref_fde); return; } - auto discovered = new DiscoveredService(interfaceIndex, serviceName, - regtype, domain); - - if (! discovered->Initialized()) { + auto discovered = new DiscoveredService(interfaceIndex, serviceName, regtype, domain); + if (!discovered->Initialized()) { delete discovered; } } @@ -274,9 +271,9 @@ void init_mdns_transport_discovery_thread(void) { } fdevent_run_on_main_thread([]() { - fdevent_install(&service_ref_fde, adb_DNSServiceRefSockFD(service_ref), pump_service_ref, - &service_ref); - fdevent_set(&service_ref_fde, FDE_READ); + service_ref_fde = + fdevent_create(adb_DNSServiceRefSockFD(service_ref), pump_service_ref, &service_ref); + fdevent_set(service_ref_fde, FDE_READ); }); } diff --git a/adb/daemon/auth.cpp b/adb/daemon/auth.cpp index 3fd2b3194..f0c36294b 100644 --- a/adb/daemon/auth.cpp +++ b/adb/daemon/auth.cpp @@ -35,8 +35,8 @@ #include #include -static fdevent listener_fde; -static fdevent framework_fde; +static fdevent* listener_fde = nullptr; +static fdevent* framework_fde = nullptr; static int framework_fd = -1; static void usb_disconnected(void* unused, atransport* t); @@ -106,8 +106,10 @@ static void usb_disconnected(void* unused, atransport* t) { static void framework_disconnected() { LOG(INFO) << "Framework disconnect"; - fdevent_remove(&framework_fde); - framework_fd = -1; + if (framework_fde) { + fdevent_destroy(framework_fde); + framework_fd = -1; + } } static void adbd_auth_event(int fd, unsigned events, void*) { @@ -168,8 +170,8 @@ static void adbd_auth_listener(int fd, unsigned events, void* data) { } framework_fd = s; - fdevent_install(&framework_fde, framework_fd, adbd_auth_event, nullptr); - fdevent_add(&framework_fde, FDE_READ); + framework_fde = fdevent_create(framework_fd, adbd_auth_event, nullptr); + fdevent_add(framework_fde, FDE_READ); if (needs_retry) { needs_retry = false; @@ -198,8 +200,8 @@ void adbd_auth_init(void) { return; } - fdevent_install(&listener_fde, fd, adbd_auth_listener, NULL); - fdevent_add(&listener_fde, FDE_READ); + listener_fde = fdevent_create(fd, adbd_auth_listener, NULL); + fdevent_add(listener_fde, FDE_READ); } void send_auth_request(atransport* t) { diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index cf441cf91..b3ff457ec 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -117,29 +117,17 @@ static std::string dump_fde(const fdevent* fde) { return android::base::StringPrintf("(fdevent %d %s)", fde->fd, state.c_str()); } -fdevent* fdevent_create(int fd, fd_func func, void* arg) { - check_main_thread(); - fdevent *fde = (fdevent*) malloc(sizeof(fdevent)); - if(fde == 0) return 0; - fdevent_install(fde, fd, func, arg); - fde->state |= FDE_CREATED; - return fde; -} - -void fdevent_destroy(fdevent* fde) { - check_main_thread(); - if(fde == 0) return; - if(!(fde->state & FDE_CREATED)) { - LOG(FATAL) << "destroying fde not created by fdevent_create(): " << dump_fde(fde); - } - fdevent_remove(fde); - free(fde); -} - void fdevent_install(fdevent* fde, int fd, fd_func func, void* arg) { check_main_thread(); CHECK_GE(fd, 0); memset(fde, 0, sizeof(fdevent)); +} + +fdevent* fdevent_create(int fd, fd_func func, void* arg) { + check_main_thread(); + CHECK_GE(fd, 0); + + fdevent* fde = new fdevent(); fde->state = FDE_ACTIVE; fde->fd = fd; fde->func = func; @@ -152,12 +140,18 @@ void fdevent_install(fdevent* fde, int fd, fd_func func, void* arg) { } auto pair = g_poll_node_map.emplace(fde->fd, PollNode(fde)); CHECK(pair.second) << "install existing fd " << fd; - D("fdevent_install %s", dump_fde(fde).c_str()); + + fde->state |= FDE_CREATED; + return fde; } -void fdevent_remove(fdevent* fde) { +void fdevent_destroy(fdevent* fde) { check_main_thread(); - D("fdevent_remove %s", dump_fde(fde).c_str()); + if (fde == 0) return; + if (!(fde->state & FDE_CREATED)) { + LOG(FATAL) << "destroying fde not created by fdevent_create(): " << dump_fde(fde); + } + if (fde->state & FDE_ACTIVE) { g_poll_node_map.erase(fde->fd); if (fde->state & FDE_PENDING) { @@ -170,6 +164,8 @@ void fdevent_remove(fdevent* fde) { fde->state = 0; fde->events = 0; } + + delete fde; } static void fdevent_update(fdevent* fde, unsigned events) { diff --git a/adb/fdevent.h b/adb/fdevent.h index 896400ad5..2d8efd8db 100644 --- a/adb/fdevent.h +++ b/adb/fdevent.h @@ -33,17 +33,17 @@ typedef void (*fd_func)(int fd, unsigned events, void *userdata); struct fdevent { - fdevent *next; - fdevent *prev; + fdevent* next = nullptr; + fdevent* prev = nullptr; - int fd; - int force_eof; + int fd = -1; + int force_eof = 0; - uint16_t state; - uint16_t events; + uint16_t state = 0; + uint16_t events = 0; - fd_func func; - void *arg; + fd_func func = nullptr; + void* arg = nullptr; }; /* Allocate and initialize a new fdevent object @@ -57,15 +57,6 @@ fdevent *fdevent_create(int fd, fd_func func, void *arg); */ void fdevent_destroy(fdevent *fde); -/* Initialize an fdevent object that was externally allocated -*/ -void fdevent_install(fdevent *fde, int fd, fd_func func, void *arg); - -/* Uninitialize an fdevent object that was initialized by -** fdevent_install() -*/ -void fdevent_remove(fdevent *item); - /* Change which events should cause notifications */ void fdevent_set(fdevent *fde, unsigned events); diff --git a/adb/fdevent_test.cpp b/adb/fdevent_test.cpp index 2f0ff1805..0cb24390a 100644 --- a/adb/fdevent_test.cpp +++ b/adb/fdevent_test.cpp @@ -31,14 +31,14 @@ class FdHandler { public: FdHandler(int read_fd, int write_fd) : read_fd_(read_fd), write_fd_(write_fd) { - fdevent_install(&read_fde_, read_fd_, FdEventCallback, this); - fdevent_add(&read_fde_, FDE_READ); - fdevent_install(&write_fde_, write_fd_, FdEventCallback, this); + read_fde_ = fdevent_create(read_fd_, FdEventCallback, this); + fdevent_add(read_fde_, FDE_READ); + write_fde_ = fdevent_create(write_fd_, FdEventCallback, this); } ~FdHandler() { - fdevent_remove(&read_fde_); - fdevent_remove(&write_fde_); + fdevent_destroy(read_fde_); + fdevent_destroy(write_fde_); } private: @@ -50,7 +50,7 @@ class FdHandler { char c; ASSERT_EQ(1, adb_read(fd, &c, 1)); handler->queue_.push(c); - fdevent_add(&handler->write_fde_, FDE_WRITE); + fdevent_add(handler->write_fde_, FDE_WRITE); } if (events & FDE_WRITE) { ASSERT_EQ(fd, handler->write_fd_); @@ -59,7 +59,7 @@ class FdHandler { handler->queue_.pop(); ASSERT_EQ(1, adb_write(fd, &c, 1)); if (handler->queue_.empty()) { - fdevent_del(&handler->write_fde_, FDE_WRITE); + fdevent_del(handler->write_fde_, FDE_WRITE); } } } @@ -67,8 +67,8 @@ class FdHandler { private: const int read_fd_; const int write_fd_; - fdevent read_fde_; - fdevent write_fde_; + fdevent* read_fde_; + fdevent* write_fde_; std::queue queue_; }; @@ -137,7 +137,7 @@ TEST_F(FdeventTest, smoke) { } struct InvalidFdArg { - fdevent fde; + fdevent* fde; unsigned expected_events; size_t* happened_event_count; }; @@ -145,7 +145,7 @@ struct InvalidFdArg { static void InvalidFdEventCallback(int, unsigned events, void* userdata) { InvalidFdArg* arg = reinterpret_cast(userdata); ASSERT_EQ(arg->expected_events, events); - fdevent_remove(&arg->fde); + fdevent_destroy(arg->fde); if (++*(arg->happened_event_count) == 2) { fdevent_terminate_loop(); } @@ -157,15 +157,15 @@ static void InvalidFdThreadFunc() { InvalidFdArg read_arg; read_arg.expected_events = FDE_READ | FDE_ERROR; read_arg.happened_event_count = &happened_event_count; - fdevent_install(&read_arg.fde, INVALID_READ_FD, InvalidFdEventCallback, &read_arg); - fdevent_add(&read_arg.fde, FDE_READ); + read_arg.fde = fdevent_create(INVALID_READ_FD, InvalidFdEventCallback, &read_arg); + fdevent_add(read_arg.fde, FDE_READ); const int INVALID_WRITE_FD = std::numeric_limits::max(); InvalidFdArg write_arg; write_arg.expected_events = FDE_READ | FDE_ERROR; write_arg.happened_event_count = &happened_event_count; - fdevent_install(&write_arg.fde, INVALID_WRITE_FD, InvalidFdEventCallback, &write_arg); - fdevent_add(&write_arg.fde, FDE_WRITE); + write_arg.fde = fdevent_create(INVALID_WRITE_FD, InvalidFdEventCallback, &write_arg); + fdevent_add(write_arg.fde, FDE_WRITE); fdevent_loop(); } diff --git a/adb/socket.h b/adb/socket.h index e0fd87f19..27e5b0534 100644 --- a/adb/socket.h +++ b/adb/socket.h @@ -58,8 +58,8 @@ struct asocket { * us to our fd event system. For remote asockets * these fields are not used. */ - fdevent fde = {}; - int fd = 0; + fdevent* fde = nullptr; + int fd = -1; // queue of data waiting to be written std::deque packet_queue; diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 7bc0165b0..4f4fbfbaa 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -121,10 +121,10 @@ static SocketFlushResult local_socket_flush_incoming(asocket* s) { s->packet_queue.pop_front(); } else if (rc > 0) { r.drop_front(rc); - fdevent_add(&s->fde, FDE_WRITE); + fdevent_add(s->fde, FDE_WRITE); return SocketFlushResult::TryAgain; } else if (rc == -1 && errno == EAGAIN) { - fdevent_add(&s->fde, FDE_WRITE); + fdevent_add(s->fde, FDE_WRITE); return SocketFlushResult::TryAgain; } else { // We failed to write, but it's possible that we can still read from the socket. @@ -140,7 +140,7 @@ static SocketFlushResult local_socket_flush_incoming(asocket* s) { return SocketFlushResult::Destroyed; } - fdevent_del(&s->fde, FDE_WRITE); + fdevent_del(s->fde, FDE_WRITE); return SocketFlushResult::Completed; } @@ -173,7 +173,7 @@ static bool local_socket_flush_outgoing(asocket* s) { break; } D("LS(%d): fd=%d post avail loop. r=%d is_eof=%d forced_eof=%d", s->id, s->fd, r, is_eof, - s->fde.force_eof); + s->fde->force_eof); if (avail != max_payload && s->peer) { data.resize(max_payload - avail); @@ -200,13 +200,13 @@ static bool local_socket_flush_outgoing(asocket* s) { ** we disable notification of READs. They'll ** be enabled again when we get a call to ready() */ - fdevent_del(&s->fde, FDE_READ); + fdevent_del(s->fde, FDE_READ); } } // Don't allow a forced eof if data is still there. - if ((s->fde.force_eof && !r) || is_eof) { - D(" closing because is_eof=%d r=%d s->fde.force_eof=%d", is_eof, r, s->fde.force_eof); + if ((s->fde->force_eof && !r) || is_eof) { + D(" closing because is_eof=%d r=%d s->fde.force_eof=%d", is_eof, r, s->fde->force_eof); s->close(s); return false; } @@ -236,19 +236,19 @@ static int local_socket_enqueue(asocket* s, apacket::payload_type data) { static void local_socket_ready(asocket* s) { /* far side is ready for data, pay attention to readable events */ - fdevent_add(&s->fde, FDE_READ); + fdevent_add(s->fde, FDE_READ); } // be sure to hold the socket list lock when calling this static void local_socket_destroy(asocket* s) { int exit_on_close = s->exit_on_close; - D("LS(%d): destroying fde.fd=%d", s->id, s->fde.fd); + D("LS(%d): destroying fde.fd=%d", s->id, s->fd); /* IMPORTANT: the remove closes the fd ** that belongs to this socket */ - fdevent_remove(&s->fde); + fdevent_destroy(s->fde); remove_socket(s); delete s; @@ -290,11 +290,11 @@ static void local_socket_close(asocket* s) { */ D("LS(%d): closing", s->id); s->closing = 1; - fdevent_del(&s->fde, FDE_READ); + fdevent_del(s->fde, FDE_READ); remove_socket(s); D("LS(%d): put on socket_closing_list fd=%d", s->id, s->fd); local_socket_closing_list.push_back(s); - CHECK_EQ(FDE_WRITE, s->fde.state & FDE_WRITE); + CHECK_EQ(FDE_WRITE, s->fde->state & FDE_WRITE); } static void local_socket_event_func(int fd, unsigned ev, void* _s) { @@ -343,7 +343,7 @@ asocket* create_local_socket(int fd) { s->close = local_socket_close; install_local_socket(s); - fdevent_install(&s->fde, fd, local_socket_event_func, s); + s->fde = fdevent_create(fd, local_socket_event_func, s); D("LS(%d): created (fd=%d)", s->id, s->fd); return s; } diff --git a/adb/transport.cpp b/adb/transport.cpp index fa7cc8c69..4c9c90c8d 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -300,7 +300,7 @@ void kick_transport(atransport* t) { static int transport_registration_send = -1; static int transport_registration_recv = -1; -static fdevent transport_registration_fde; +static fdevent* transport_registration_fde; #if ADB_HOST @@ -565,10 +565,9 @@ void init_transport_registration(void) { transport_registration_send = s[0]; transport_registration_recv = s[1]; - fdevent_install(&transport_registration_fde, transport_registration_recv, - transport_registration_func, 0); - - fdevent_set(&transport_registration_fde, FDE_READ); + transport_registration_fde = + fdevent_create(transport_registration_recv, transport_registration_func, 0); + fdevent_set(transport_registration_fde, FDE_READ); } void kick_all_transports() { From e5353021baba3aff13e2dea6601aa8f31c5a42dc Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 14 May 2018 13:17:56 -0700 Subject: [PATCH 3/5] adb: delete FDEVENT_DONTCLOSE. The only existing usage of this doesn't actually need it. Bug: http://b/79786774 Test: mma Change-Id: If5e665705393e938cfdbf1526beb5496a8b99a9b --- adb/daemon/jdwp_service.cpp | 3 --- adb/fdevent.cpp | 9 ++------- adb/fdevent.h | 3 --- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/adb/daemon/jdwp_service.cpp b/adb/daemon/jdwp_service.cpp index 89577cb7c..211d8f345 100644 --- a/adb/daemon/jdwp_service.cpp +++ b/adb/daemon/jdwp_service.cpp @@ -139,8 +139,6 @@ struct JdwpProcess { fatal("could not create fdevent for new JDWP process"); } - this->fde->state |= FDE_DONT_CLOSE; - /* start by waiting for the PID */ fdevent_add(this->fde, FDE_READ); } @@ -148,7 +146,6 @@ struct JdwpProcess { ~JdwpProcess() { if (this->socket >= 0) { adb_shutdown(this->socket); - adb_close(this->socket); this->socket = -1; } diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index b3ff457ec..f9e262c51 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -111,9 +111,6 @@ static std::string dump_fde(const fdevent* fde) { if (fde->state & FDE_ERROR) { state += "E"; } - if (fde->state & FDE_DONT_CLOSE) { - state += "D"; - } return android::base::StringPrintf("(fdevent %d %s)", fde->fd, state.c_str()); } @@ -157,10 +154,8 @@ void fdevent_destroy(fdevent* fde) { if (fde->state & FDE_PENDING) { g_pending_list.remove(fde); } - if (!(fde->state & FDE_DONT_CLOSE)) { - adb_close(fde->fd); - fde->fd = -1; - } + adb_close(fde->fd); + fde->fd = -1; fde->state = 0; fde->events = 0; } diff --git a/adb/fdevent.h b/adb/fdevent.h index 2d8efd8db..9c6e06999 100644 --- a/adb/fdevent.h +++ b/adb/fdevent.h @@ -27,9 +27,6 @@ #define FDE_WRITE 0x0002 #define FDE_ERROR 0x0004 -/* features that may be set (via the events set/add/del interface) */ -#define FDE_DONT_CLOSE 0x0080 - typedef void (*fd_func)(int fd, unsigned events, void *userdata); struct fdevent { From ae9c1dc44a7cc6e6eb217ee3284f53fcf5332140 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 11 May 2018 12:55:56 -0700 Subject: [PATCH 4/5] adb: move towards using unique_fd. Bug: http://b/79786774 Test: treehugger Change-Id: Ib5a684bba88e87e1aad9da452dcdd5edd11f18f4 --- adb/adb.cpp | 16 +++++++--------- adb/adb_unique_fd.h | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 3bf281c6d..7fc4cc2cd 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -885,9 +885,8 @@ int launch_server(const std::string& socket_spec) { } #else /* !defined(_WIN32) */ // set up a pipe so the child can tell us when it is ready. - // fd[0] will be parent's end, and the child will write on fd[1] - int fd[2]; - if (pipe(fd)) { + unique_fd pipe_read, pipe_write; + if (!Pipe(&pipe_read, &pipe_write)) { fprintf(stderr, "pipe failed in launch_server, errno: %d\n", errno); return -1; } @@ -899,11 +898,10 @@ int launch_server(const std::string& socket_spec) { if (pid == 0) { // child side of the fork - - adb_close(fd[0]); + pipe_read.reset(); char reply_fd[30]; - snprintf(reply_fd, sizeof(reply_fd), "%d", fd[1]); + snprintf(reply_fd, sizeof(reply_fd), "%d", pipe_write.get()); // child process int result = execl(path.c_str(), "adb", "-L", socket_spec.c_str(), "fork-server", "server", "--reply-fd", reply_fd, NULL); @@ -913,10 +911,10 @@ int launch_server(const std::string& socket_spec) { // parent side of the fork char temp[3] = {}; // wait for the "OK\n" message - adb_close(fd[1]); - int ret = adb_read(fd[0], temp, 3); + pipe_write.reset(); + int ret = adb_read(pipe_read.get(), temp, 3); int saved_errno = errno; - adb_close(fd[0]); + pipe_read.reset(); if (ret < 0) { fprintf(stderr, "could not read ok from ADB Server, errno = %d\n", saved_errno); return -1; diff --git a/adb/adb_unique_fd.h b/adb/adb_unique_fd.h index 34c1bbcfa..9c02cbeec 100644 --- a/adb/adb_unique_fd.h +++ b/adb/adb_unique_fd.h @@ -16,6 +16,8 @@ #pragma once +#include + #include // Helper to automatically close an FD when it goes out of scope. @@ -24,3 +26,15 @@ struct AdbCloser { }; using unique_fd = android::base::unique_fd_impl; + +#if !defined(_WIN32) +inline bool Pipe(unique_fd* read, unique_fd* write) { + int pipefd[2]; + if (pipe(pipefd) != 0) { + return false; + } + read->reset(pipefd[0]); + write->reset(pipefd[1]); + return true; +} +#endif From 3b37fa256f00abc6a6414984cabbb3dd9e0a0ad1 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 14 May 2018 13:42:49 -0700 Subject: [PATCH 5/5] adb: convert fdevent over to unique_fd. This adds fdsan deallocation sanitization to all fds monitored by fdevent, which is most of the ones in adb. Bug: http://b/79786774 Test: python test_device.py Change-Id: I465804fdb0fd0ac019445900a30ba3403f5bf711 --- adb/fdevent.cpp | 19 +++++++++---------- adb/fdevent.h | 4 +++- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp index f9e262c51..1b7758c57 100644 --- a/adb/fdevent.cpp +++ b/adb/fdevent.cpp @@ -57,7 +57,7 @@ struct PollNode { explicit PollNode(fdevent* fde) : fde(fde) { memset(&pollfd, 0, sizeof(pollfd)); - pollfd.fd = fde->fd; + pollfd.fd = fde->fd.get(); #if defined(__linux__) // Always enable POLLRDHUP, so the host server can take action when some clients disconnect. @@ -111,7 +111,7 @@ static std::string dump_fde(const fdevent* fde) { if (fde->state & FDE_ERROR) { state += "E"; } - return android::base::StringPrintf("(fdevent %d %s)", fde->fd, state.c_str()); + return android::base::StringPrintf("(fdevent %d %s)", fde->fd.get(), state.c_str()); } void fdevent_install(fdevent* fde, int fd, fd_func func, void* arg) { @@ -126,7 +126,7 @@ fdevent* fdevent_create(int fd, fd_func func, void* arg) { fdevent* fde = new fdevent(); fde->state = FDE_ACTIVE; - fde->fd = fd; + fde->fd.reset(fd); fde->func = func; fde->arg = arg; if (!set_file_block_mode(fd, false)) { @@ -135,7 +135,7 @@ fdevent* fdevent_create(int fd, fd_func func, void* arg) { // to handle it. LOG(ERROR) << "failed to set non-blocking mode for fd " << fd; } - auto pair = g_poll_node_map.emplace(fde->fd, PollNode(fde)); + auto pair = g_poll_node_map.emplace(fde->fd.get(), PollNode(fde)); CHECK(pair.second) << "install existing fd " << fd; fde->state |= FDE_CREATED; @@ -150,12 +150,11 @@ void fdevent_destroy(fdevent* fde) { } if (fde->state & FDE_ACTIVE) { - g_poll_node_map.erase(fde->fd); + g_poll_node_map.erase(fde->fd.get()); if (fde->state & FDE_PENDING) { g_pending_list.remove(fde); } - adb_close(fde->fd); - fde->fd = -1; + fde->fd.reset(); fde->state = 0; fde->events = 0; } @@ -164,7 +163,7 @@ void fdevent_destroy(fdevent* fde) { } static void fdevent_update(fdevent* fde, unsigned events) { - auto it = g_poll_node_map.find(fde->fd); + auto it = g_poll_node_map.find(fde->fd.get()); CHECK(it != g_poll_node_map.end()); PollNode& node = it->second; if (events & FDE_READ) { @@ -263,7 +262,7 @@ static void fdevent_process() { auto it = g_poll_node_map.find(pollfd.fd); CHECK(it != g_poll_node_map.end()); fdevent* fde = it->second.fde; - CHECK_EQ(fde->fd, pollfd.fd); + CHECK_EQ(fde->fd.get(), pollfd.fd); fde->events |= events; D("%s got events %x", dump_fde(fde).c_str(), events); fde->state |= FDE_PENDING; @@ -278,7 +277,7 @@ static void fdevent_call_fdfunc(fdevent* fde) { CHECK(fde->state & FDE_PENDING); fde->state &= (~FDE_PENDING); D("fdevent_call_fdfunc %s", dump_fde(fde).c_str()); - fde->func(fde->fd, events, fde->arg); + fde->func(fde->fd.get(), events, fde->arg); } static void fdevent_run_flush() EXCLUDES(run_queue_mutex) { diff --git a/adb/fdevent.h b/adb/fdevent.h index 9c6e06999..69c40721b 100644 --- a/adb/fdevent.h +++ b/adb/fdevent.h @@ -22,6 +22,8 @@ #include +#include "adb_unique_fd.h" + /* events that may be observed */ #define FDE_READ 0x0001 #define FDE_WRITE 0x0002 @@ -33,7 +35,7 @@ struct fdevent { fdevent* next = nullptr; fdevent* prev = nullptr; - int fd = -1; + unique_fd fd; int force_eof = 0; uint16_t state = 0;