commit 34c6ca01ff7f4750c5865923e2d4d0a2fd33a15a
author Josh Gao <jmgao@google.com> Mon Jun 20 20:47:17 2016 +0000
committer android-build-merger <android-build-merger@google.com> Mon Jun 20 20:47:17 2016 +0000
tree 11717c0f7605d4cbbc2f5e79705e3de8b2ff5dfb
parent 9f6b80c9c8073e1a8ee1b7ed124e5321cb4052df
parent b23e5729a8df729f4994be39a1d5db6a256dc419

Merge \"adb: fix fd double close, Subprocess lifetime issue.\"
am: b23e5729a8

Change-Id: I96db140e4316b767fb632307a435bc3e22cbfb36

adb/shell_service.cpp

diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index b038fda..10b2502 100644
--- a/adb/shell_service.cpp
+++ b/adb/shell_service.cpp
@@ -160,9 +160,14 @@
     pid_t pid() const { return pid_; }
 
     // Sets up FDs, forks a subprocess, starts the subprocess manager thread,
-    // and exec's the child. Returns false on failure.
+    // and exec's the child. Returns false and sets error on failure.
     bool ForkAndExec(std::string* _Nonnull error);
 
+    // Start the subprocess manager thread. Consumes the subprocess, regardless of success.
+    // Returns false and sets error on failure.
+    static bool StartThread(std::unique_ptr<Subprocess> subprocess,
+                            std::string* _Nonnull error);
+
   private:
     // Opens the file at |pts_name|.
     int OpenPtyChildFd(const char* pts_name, unique_fd* error_sfd);
@@ -390,14 +395,19 @@
         }
     }
 
-    if (!adb_thread_create(ThreadHandler, this)) {
+    D("subprocess parent: completed");
+    return true;
+}
+
+bool Subprocess::StartThread(std::unique_ptr<Subprocess> subprocess, std::string* error) {
+    Subprocess* raw = subprocess.release();
+    if (!adb_thread_create(ThreadHandler, raw)) {
         *error =
             android::base::StringPrintf("failed to create subprocess thread: %s", strerror(errno));
-        kill(pid_, SIGKILL);
+        kill(raw->pid_, SIGKILL);
         return false;
     }
 
-    D("subprocess parent: completed");
     return true;
 }
 
@@ -441,6 +451,7 @@
     adb_thread_setname(android::base::StringPrintf(
             "shell srvc %d", subprocess->local_socket_fd()));
 
+    D("passing data streams for PID %d", subprocess->pid());
     subprocess->PassDataStreams();
 
     D("deleting Subprocess for PID %d", subprocess->pid());
@@ -733,7 +744,7 @@
       protocol == SubprocessProtocol::kNone ? "none" : "shell",
       terminal_type, name);
 
-    Subprocess* subprocess = new Subprocess(name, terminal_type, type, protocol);
+    auto subprocess = std::make_unique<Subprocess>(name, terminal_type, type, protocol);
     if (!subprocess) {
         LOG(ERROR) << "failed to allocate new subprocess";
         return ReportError(protocol, "failed to allocate new subprocess");
@@ -742,11 +753,17 @@
     std::string error;
     if (!subprocess->ForkAndExec(&error)) {
         LOG(ERROR) << "failed to start subprocess: " << error;
-        delete subprocess;
         return ReportError(protocol, error);
     }
 
-    D("subprocess creation successful: local_socket_fd=%d, pid=%d",
-      subprocess->local_socket_fd(), subprocess->pid());
-    return subprocess->local_socket_fd();
+    unique_fd local_socket(dup(subprocess->local_socket_fd()));
+    D("subprocess creation successful: local_socket_fd=%d, pid=%d", local_socket.get(),
+      subprocess->pid());
+
+    if (!Subprocess::StartThread(std::move(subprocess), &error)) {
+        LOG(ERROR) << "failed to start subprocess management thread: " << error;
+        return ReportError(protocol, error);
+    }
+
+    return local_socket.release();
 }