From 9f2d1a9cfc04e1d5970823da1878097288a9a9cd Mon Sep 17 00:00:00 2001 From: Siva Velusamy Date: Fri, 7 Aug 2015 10:10:29 -0700 Subject: [PATCH] adb start-server: Use a separate fd for sending initial OK When "adb start-server" is issued, and a server needs to be launched, adb client forks itself and the child process runs the server routine. Once the server initializes its various components, it sends an "OK\n" back to the client via its stderror (or stdout on Windows). This sequence breaks down if before sending the "OK\n", the server happens to log something on its stderr. In order to avoid this, the client now expects the ack to come on a different fd rather than one of the standard streams. Bug: https://code.google.com/p/android/issues/detail?id=182150 Change-Id: I9d58a08068d71eb3b77e8a7377e934631c016466 --- adb/adb.cpp | 12 +++++------- adb/adb.h | 2 +- adb/client/main.cpp | 28 +++++++++------------------- adb/commandline.cpp | 26 ++++++++++++++++++++++++-- 4 files changed, 39 insertions(+), 29 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index fd46deac1..dd6c55511 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -698,7 +698,7 @@ int launch_server(int server_port) int fd[2]; // set up a pipe so the child can tell us when it is ready. - // fd[0] will be parent's end, and fd[1] will get mapped to stderr in the child. + // fd[0] will be parent's end, and the child will write on fd[1] if (pipe(fd)) { fprintf(stderr, "pipe failed in launch_server, errno: %d\n", errno); return -1; @@ -710,16 +710,14 @@ int launch_server(int server_port) if (pid == 0) { // child side of the fork - // redirect stderr to the pipe - // we use stderr instead of stdout due to stdout's buffering behavior. adb_close(fd[0]); - dup2(fd[1], STDERR_FILENO); - adb_close(fd[1]); char str_port[30]; - snprintf(str_port, sizeof(str_port), "%d", server_port); + snprintf(str_port, sizeof(str_port), "%d", server_port); + char reply_fd[30]; + snprintf(reply_fd, sizeof(reply_fd), "%d", fd[1]); // child process - int result = execl(path, "adb", "-P", str_port, "fork-server", "server", NULL); + int result = execl(path, "adb", "-P", str_port, "fork-server", "server", "--reply-fd", reply_fd, NULL); // this should not return fprintf(stderr, "OOPS! execl returned %d, errno: %d\n", result, errno); } else { diff --git a/adb/adb.h b/adb/adb.h index 309b0e9bc..b0e53f026 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -299,7 +299,7 @@ void handle_packet(apacket *p, atransport *t); void get_my_path(char *s, size_t maxLen); int launch_server(int server_port); -int adb_main(int is_daemon, int server_port); +int adb_main(int is_daemon, int server_port, int ack_reply_fd); /* initialize a transport object's func pointers and state */ #if ADB_HOST diff --git a/adb/client/main.cpp b/adb/client/main.cpp index 6b4862138..af3a6a108 100644 --- a/adb/client/main.cpp +++ b/adb/client/main.cpp @@ -132,7 +132,7 @@ static void setup_daemon_logging(void) { fprintf(stderr, "--- adb starting (pid %d) ---\n", getpid()); } -int adb_main(int is_daemon, int server_port) { +int adb_main(int is_daemon, int server_port, int ack_reply_fd) { HOST = 1; #if defined(_WIN32) @@ -157,29 +157,20 @@ int adb_main(int is_daemon, int server_port) { LOG(FATAL) << "Could not install *smartsocket* listener: " << error; } + // Inform our parent that we are up and running. if (is_daemon) { - // Inform our parent that we are up and running. - // TODO(danalbert): Can't use SendOkay because we're sending "OK\n", not - // "OKAY". - // TODO(danalbert): Why do we use stdout for Windows? There is a - // comment in launch_server() that suggests that non-Windows uses - // stderr because it is non-buffered. So perhaps the history is that - // stdout was preferred for all platforms, but it was discovered that - // non-Windows needed a non-buffered fd, so stderr was used there. - // Note that using stderr on unix means that if you do - // `ADB_TRACE=all adb start-server`, it will say "ADB server didn't ACK" - // and "* failed to start daemon *" because the adb server will write - // logging to stderr, obscuring the OK\n output that is sent to stderr. #if defined(_WIN32) - int reply_fd = STDOUT_FILENO; // Change stdout mode to binary so \n => \r\n translation does not // occur. In a moment stdout will be reopened to the daemon log file // anyway. - _setmode(reply_fd, _O_BINARY); -#else - int reply_fd = STDERR_FILENO; + _setmode(ack_reply_fd, _O_BINARY); +#endif + // TODO(danalbert): Can't use SendOkay because we're sending "OK\n", not + // "OKAY". + android::base::WriteStringToFd("OK\n", ack_reply_fd); +#if !defined(_WIN32) + adb_close(ack_reply_fd); #endif - android::base::WriteStringToFd("OK\n", reply_fd); close_stdin(); setup_daemon_logging(); } @@ -204,7 +195,6 @@ int main(int argc, char** argv) { adb_sysdeps_init(); adb_trace_init(argv); - D("Handling commandline()\n"); return adb_commandline(argc - 1, const_cast(argv + 1)); } diff --git a/adb/commandline.cpp b/adb/commandline.cpp index d7a0c8d5b..2e182e898 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -954,6 +954,14 @@ int adb_commandline(int argc, const char **argv) { int r; TransportType transport_type = kTransportAny; +#if defined(_WIN32) + // TODO(compareandswap): Windows should use a separate reply fd too. + int ack_reply_fd = STDOUT_FILENO; +#else + int ack_reply_fd = -1; +#endif + + // If defined, this should be an absolute path to // the directory containing all of the various system images // for a particular product. If not defined, and the adb @@ -971,7 +979,7 @@ int adb_commandline(int argc, const char **argv) { const char* server_port_str = getenv("ANDROID_ADB_SERVER_PORT"); int server_port = DEFAULT_ADB_PORT; if (server_port_str && strlen(server_port_str) > 0) { - server_port = (int) strtol(server_port_str, NULL, 0); + server_port = strtol(server_port_str, nullptr, 0); if (server_port <= 0 || server_port > 65535) { fprintf(stderr, "adb: Env var ANDROID_ADB_SERVER_PORT must be a positive number less than 65535. Got \"%s\"\n", @@ -989,6 +997,16 @@ int adb_commandline(int argc, const char **argv) { } else if (!strcmp(argv[0], "fork-server")) { /* this is a special flag used only when the ADB client launches the ADB Server */ is_daemon = 1; + } else if (!strcmp(argv[0], "--reply-fd")) { + if (argc < 2) return usage(); + const char* reply_fd_str = argv[1]; + argc--; + argv++; + ack_reply_fd = strtol(reply_fd_str, nullptr, 10); + if (ack_reply_fd <= 2) { // Disallow stdin, stdout, and stderr. + fprintf(stderr, "adb: invalid reply fd \"%s\"\n", reply_fd_str); + return usage(); + } } else if (!strncmp(argv[0], "-p", 2)) { const char* product = nullptr; if (argv[0][2] == '\0') { @@ -1066,7 +1084,11 @@ int adb_commandline(int argc, const char **argv) { if (is_server) { if (no_daemon || is_daemon) { - r = adb_main(is_daemon, server_port); + if (ack_reply_fd == -1) { + fprintf(stderr, "reply fd for adb server to client communication not specified.\n"); + return usage(); + } + r = adb_main(is_daemon, server_port, ack_reply_fd); } else { r = launch_server(server_port); }