commit 344778da411ebb47966961f3a70ca0848425194f
author Josh Gao <jmgao@google.com> Fri Jun 17 14:53:57 2016 -0700
committer Josh Gao <jmgao@google.com> Mon Jun 20 11:40:16 2016 -0700
tree 0f07f3dc9c7c2180e856a6f7d69928758d26d88d
parent 2d690a920f1b2f67e267160083aab711b35ab3fc

adb: fix fd double close, Subprocess lifetime issue.

This commit fixes two somewhat related issues in shell_service.

  - The fd returned by StartSubprocess is owned by a unique_fd
    contained in the Subprocess object, but also gets closed by the
    caller. Resolve this by duping the returned file descriptor.

  - A Subprocess object can be destroyed immediately after its initial
    construction in StartSubprocess if we're sufficiently unlucky.
    Split up the fork/exec and "start management thread" steps, so that
    we can safely do everything we need to do on the Subprocess before
    handing it over to the thread that'll eventually destroy it.

Bug: http://b/29254462
Change-Id: Id9cf0b7e7a7293bee7176919edc758597691c636

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