From 351ecd15b29f0ce528ffac119640b9c01874562b Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Wed, 14 Oct 2015 17:32:44 -0700 Subject: adb: fix adb client running out of sockets on Windows Background ========== On Windows, if you run "adb shell exit" in a loop in two windows, eventually the adb client will be unable to connect to the adb server. I think connect() is returning WSAEADDRINUSE: "Only one usage of each socket address (protocol/network address/port) is normally permitted. (10048)". The Windows System Event Log may also show Event 4227, Tcpip. Netstat output is filled with: # for the adb server TCP 127.0.0.1:5037 127.0.0.1:65523 TIME_WAIT # for the adb client TCP 127.0.0.1:65523 127.0.0.1:5037 TIME_WAIT The error probably means that the client is running out of free address:port pairs. The first netstat line is unavoidable, but the second line exists because the adb client is not waiting for orderly/graceful shutdown of the socket, and that is apparently required on Windows to get rid of the second line. For more info, see https://github.com/CompareAndSwap/SocketCloseTest . This is exacerbated by the fact that "adb shell exit" makes 4 socket connections to the adb server: 1) host:version, 2) host:features, 3) host:version (again), 4) shell:exit. Also exacerbating is the fact that the adb protocol is length-prefixed so the client typically does not have to 'read() until zero' which effectively waits for orderly/graceful shutdown. The Fix ======= Introduce a function, ReadOrderlyShutdown(), that should be called in the adb client to wait for the server to close its socket, before closing the client socket. I reviewed all code where the adb client makes a connection to the adb server and added ReadOrderlyShutdown() when it made sense. I wasn't able to add it to the following: * interactive_shell: this doesn't matter because this is interactive and thus can't be run fast enough to use up ports. * adb sideload: I couldn't get enough test coverage and I don't think this is being called frequently enough to be a problem. * send_shell_command, backup, adb_connect_command, adb shell, adb exec-out, install_multiple_app, adb_send_emulator_command: These already wait for server socket shutdown since they already call recv() until zero. * restore, adb exec-in: protocol design can't have the server close first. * adb start-server: no fd is actually returned * create_local_service_socket, local_connect_arbitrary_ports, connect_device: probably called rarely enough not to be a problem. Also in this change =================== * Clarify comments in when adb_shutdown() is called before exit(). * add some missing adb_close() in adb sideload. * Fixup error handling and comments in adb_send_emulator_command(). * Make SyncConnection::SendQuit return a success boolean. * Add unittest for adb emu kill command. This gets code coverage over this very careful piece of code. Change-Id: Iad0b1336f5b74186af2cd35f7ea827d0fa77a17c Signed-off-by: Spencer Low --- adb/adb_io.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) (limited to 'adb/adb_io.cpp') diff --git a/adb/adb_io.cpp b/adb/adb_io.cpp index 7788996ab..a37fbc086 100644 --- a/adb/adb_io.cpp +++ b/adb/adb_io.cpp @@ -137,3 +137,43 @@ bool WriteFdFmt(int fd, const char* fmt, ...) { return WriteFdExactly(fd, str); } + +bool ReadOrderlyShutdown(int fd) { + char buf[16]; + + // Only call this function if you're sure that the peer does + // orderly/graceful shutdown of the socket, closing the socket so that + // adb_read() will return 0. If the peer keeps the socket open, adb_read() + // will never return. + int result = adb_read(fd, buf, sizeof(buf)); + if (result == -1) { + // If errno is EAGAIN, that means this function was called on a + // nonblocking socket and it would have blocked (which would be bad + // because we'd probably block the main thread where nonblocking IO is + // done). Don't do that. If you have a nonblocking socket, use the + // fdevent APIs to get called on FDE_READ, and then call this function + // if you really need to, but it shouldn't be needed for server sockets. + CHECK_NE(errno, EAGAIN); + + // Note that on Windows, orderly shutdown sometimes causes + // recv() == SOCKET_ERROR && WSAGetLastError() == WSAECONNRESET. That + // can be ignored. + return false; + } else if (result == 0) { + // Peer has performed an orderly/graceful shutdown. + return true; + } else { + // Unexpectedly received data. This is essentially a protocol error + // because you should not call this function unless you expect no more + // data. We don't repeatedly call adb_read() until we get zero because + // we don't know how long that would take, but we do know that the + // caller wants to close the socket soon. + VLOG(RWX) << "ReadOrderlyShutdown(" << fd << ") unexpectedly read " + << dump_hex(buf, result); + // Shutdown the socket to prevent the caller from reading or writing to + // it which doesn't make sense if we just read and discarded some data. + adb_shutdown(fd); + errno = EINVAL; + return false; + } +} -- cgit v1.2.3