am 98714882: Merge "adb start-server: Use a separate fd for sending initial OK"

* commit '9871488223b5bd8f08f4a22920057297f942f94a':
  adb start-server: Use a separate fd for sending initial OK

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