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
diff --git a/adb/adb.cpp b/adb/adb.cpp
index fd46dea..dd6c555 100644
--- a/adb/adb.cpp
+++ b/adb/adb.cpp
@@ -698,7 +698,7 @@
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 @@
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 309b0e9..b0e53f0 100644
--- a/adb/adb.h
+++ b/adb/adb.h
@@ -299,7 +299,7 @@
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 6b48621..af3a6a1 100644
--- a/adb/client/main.cpp
+++ b/adb/client/main.cpp
@@ -132,7 +132,7 @@
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 @@
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
- android::base::WriteStringToFd("OK\n", reply_fd);
+ // 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
close_stdin();
setup_daemon_logging();
}
@@ -204,7 +195,6 @@
adb_sysdeps_init();
adb_trace_init(argv);
- D("Handling commandline()\n");
return adb_commandline(argc - 1, const_cast<const char**>(argv + 1));
}
diff --git a/adb/commandline.cpp b/adb/commandline.cpp
index d7a0c8d..2e182e8 100644
--- a/adb/commandline.cpp
+++ b/adb/commandline.cpp
@@ -954,6 +954,14 @@
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 @@
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 @@
} 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 @@
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);
}