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

@@ -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  {

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

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<const char**>(argv + 1));
 }
 

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);
         }