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.

Also includes squashed patches from AOSP master that allow for use of
unique_fd inside adb.

Bug: http://b/29254462
Change-Id: Id9cf0b7e7a7293bee7176919edc758597691c636
(cherry picked from commit c0e6e40cc9)
(cherry picked from commit 54c72aaccc)
(cherry picked from commit 2c5d1d7cd9)
(cherry picked from commit 2a7b86337f)
(cherry picked from commit 13ea01db45)
(cherry picked from commit 344778da41)

adb/adb_utils.h

@@ -20,6 +20,7 @@
 #include <string>
 
 #include <android-base/macros.h>
+#include <android-base/unique_fd.h>
 
 void close_stdin();
 
@@ -50,6 +51,14 @@ bool set_file_block_mode(int fd, bool block);
 extern int adb_close(int fd);
 
 // Helper to automatically close an FD when it goes out of scope.
+struct AdbCloser {
+    static void Close(int fd) {
+        adb_close(fd);
+    }
+};
+
+using unique_fd = android::base::unique_fd_impl<AdbCloser>;
+
 class ScopedFd {
   public:
     ScopedFd() {

adb/shell_service.cpp

@@ -160,9 +160,14 @@ class Subprocess {
     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, ScopedFd* error_sfd);
@@ -391,14 +396,19 @@ bool Subprocess::ForkAndExec(std::string* error) {
         }
     }
 
-    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;
 }
 
@@ -442,6 +452,7 @@ void Subprocess::ThreadHandler(void* userdata) {
     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());
@@ -738,7 +749,7 @@ int StartSubprocess(const char* name, const char* terminal_type,
       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");
@@ -747,11 +758,17 @@ int StartSubprocess(const char* name, const char* terminal_type,
     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();
 }

adb/sysdeps.h

@@ -29,8 +29,9 @@
 #include <string>
 #include <vector>
 
-// Include this before open/unlink are defined as macros below.
+// Include this before open/close/unlink are defined as macros below.
 #include <android-base/errors.h>
+#include <android-base/unique_fd.h>
 #include <android-base/utf8.h>
 
 /*

base/include/android-base/errors.h

@@ -27,8 +27,8 @@
 // special handling to get the error string. Refer to Microsoft documentation
 // to determine which error code to check for each function.
 
-#ifndef BASE_ERRORS_H
-#define BASE_ERRORS_H
+#ifndef ANDROID_BASE_ERRORS_H
+#define ANDROID_BASE_ERRORS_H
 
 #include <string>
 
@@ -43,4 +43,4 @@ std::string SystemErrorCodeToString(int error_code);
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_ERRORS_H
+#endif  // ANDROID_BASE_ERRORS_H

base/include/android-base/file.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_FILE_H
-#define BASE_FILE_H
+#ifndef ANDROID_BASE_FILE_H
+#define ANDROID_BASE_FILE_H
 
 #include <sys/stat.h>
 #include <string>
@@ -46,4 +46,4 @@ bool RemoveFileIfExists(const std::string& path, std::string* err = nullptr);
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_FILE_H
+#endif // ANDROID_BASE_FILE_H

base/include/android-base/logging.h

@@ -13,8 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#ifndef BASE_LOGGING_H
-#define BASE_LOGGING_H
+
+#ifndef ANDROID_BASE_LOGGING_H
+#define ANDROID_BASE_LOGGING_H
 
 // NOTE: For Windows, you must include logging.h after windows.h to allow the
 // following code to suppress the evil ERROR macro:
@@ -334,4 +335,4 @@ class ScopedLogSeverity {
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_LOGGING_H
+#endif  // ANDROID_BASE_LOGGING_H

base/include/android-base/macros.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef UTILS_MACROS_H
-#define UTILS_MACROS_H
+#ifndef ANDROID_BASE_MACROS_H
+#define ANDROID_BASE_MACROS_H
 
 #include <stddef.h>  // for size_t
 #include <unistd.h>  // for TEMP_FAILURE_RETRY
@@ -185,4 +185,4 @@ void UNUSED(const T&...) {
   } while (0)
 #endif
 
-#endif  // UTILS_MACROS_H
+#endif  // ANDROID_BASE_MACROS_H

base/include/android-base/memory.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_MEMORY_H
-#define BASE_MEMORY_H
+#ifndef ANDROID_BASE_MEMORY_H
+#define ANDROID_BASE_MEMORY_H
 
 namespace android {
 namespace base {
@@ -44,4 +44,4 @@ static inline void put_unaligned(T* address, T v) {
 } // namespace base
 } // namespace android
 
-#endif // BASE_MEMORY_H
+#endif  // ANDROID_BASE_MEMORY_H

base/include/android-base/parseint.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_PARSEINT_H
-#define BASE_PARSEINT_H
+#ifndef ANDROID_BASE_PARSEINT_H
+#define ANDROID_BASE_PARSEINT_H
 
 #include <errno.h>
 #include <stdlib.h>
@@ -70,4 +70,4 @@ bool ParseInt(const char* s, T* out,
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_PARSEINT_H
+#endif  // ANDROID_BASE_PARSEINT_H

base/include/android-base/parsenetaddress.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_PARSENETADDRESS_H
-#define BASE_PARSENETADDRESS_H
+#ifndef ANDROID_BASE_PARSENETADDRESS_H
+#define ANDROID_BASE_PARSENETADDRESS_H
 
 #include <string>
 
@@ -35,4 +35,4 @@ bool ParseNetAddress(const std::string& address, std::string* host, int* port,
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_PARSENETADDRESS_H
+#endif  // ANDROID_BASE_PARSENETADDRESS_H

base/include/android-base/stringprintf.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_STRINGPRINTF_H
-#define BASE_STRINGPRINTF_H
+#ifndef ANDROID_BASE_STRINGPRINTF_H
+#define ANDROID_BASE_STRINGPRINTF_H
 
 #include <stdarg.h>
 #include <string>
@@ -53,4 +53,4 @@ void StringAppendV(std::string* dst, const char* format, va_list ap)
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_STRINGPRINTF_H
+#endif  // ANDROID_BASE_STRINGPRINTF_H

base/include/android-base/strings.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_STRINGS_H
-#define BASE_STRINGS_H
+#ifndef ANDROID_BASE_STRINGS_H
+#define ANDROID_BASE_STRINGS_H
 
 #include <sstream>
 #include <string>
@@ -65,4 +65,4 @@ bool EndsWith(const std::string& s, const char* suffix);
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_STRINGS_H
+#endif  // ANDROID_BASE_STRINGS_H

base/include/android-base/test_utils.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef TEST_UTILS_H
-#define TEST_UTILS_H
+#ifndef ANDROID_BASE_TEST_UTILS_H
+#define ANDROID_BASE_TEST_UTILS_H
 
 #include <string>
 
@@ -48,4 +48,4 @@ class TemporaryDir {
   DISALLOW_COPY_AND_ASSIGN(TemporaryDir);
 };
 
-#endif // TEST_UTILS_H
+#endif  // ANDROID_BASE_TEST_UTILS_H

base/include/android-base/thread_annotations.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef UTILS_THREAD_ANNOTATIONS_H
-#define UTILS_THREAD_ANNOTATIONS_H
+#ifndef ANDROID_BASE_THREAD_ANNOTATIONS_H
+#define ANDROID_BASE_THREAD_ANNOTATIONS_H
 
 #if defined(__SUPPORT_TS_ANNOTATION__) || defined(__clang__)
 #define THREAD_ANNOTATION_ATTRIBUTE__(x)   __attribute__((x))
@@ -80,4 +80,4 @@
 #define NO_THREAD_SAFETY_ANALYSIS \
       THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)
 
-#endif  // UTILS_THREAD_ANNOTATIONS_H
+#endif  // ANDROID_BASE_THREAD_ANNOTATIONS_H

base/include/android-base/unique_fd.h

@@ -19,39 +19,54 @@
 
 #include <unistd.h>
 
-#include <android-base/macros.h>
+// DO NOT INCLUDE OTHER LIBBASE HEADERS!
+// This file gets used in libbinder, and libbinder is used everywhere.
+// Including other headers from libbase frequently results in inclusion of
+// android-base/macros.h, which causes macro collisions.
 
-/* Container for a file descriptor that automatically closes the descriptor as
- * it goes out of scope.
- *
- *      unique_fd ufd(open("/some/path", "r"));
- *
- *      if (ufd.get() < 0) // invalid descriptor
- *          return error;
- *
- *      // Do something useful
- *
- *      return 0; // descriptor is closed here
- */
+// Container for a file descriptor that automatically closes the descriptor as
+// it goes out of scope.
+//
+//      unique_fd ufd(open("/some/path", "r"));
+//      if (ufd.get() == -1) return error;
+//
+//      // Do something useful, possibly including 'return'.
+//
+//      return 0; // Descriptor is closed for you.
+//
+// unique_fd is also known as ScopedFd/ScopedFD/scoped_fd; mentioned here to help
+// you find this class if you're searching for one of those names.
 namespace android {
 namespace base {
 
-class unique_fd final {
+struct DefaultCloser {
+  static void Close(int fd) {
+    // Even if close(2) fails with EINTR, the fd will have been closed.
+    // Using TEMP_FAILURE_RETRY will either lead to EBADF or closing someone
+    // else's fd.
+    // http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
+    ::close(fd);
+  }
+};
+
+template <typename Closer>
+class unique_fd_impl final {
  public:
-  unique_fd() : value_(-1) {}
+  unique_fd_impl() : value_(-1) {}
 
-  explicit unique_fd(int value) : value_(value) {}
-  ~unique_fd() { clear(); }
+  explicit unique_fd_impl(int value) : value_(value) {}
+  ~unique_fd_impl() { clear(); }
 
-  unique_fd(unique_fd&& other) : value_(other.release()) {}
-  unique_fd& operator = (unique_fd&& s) {
+  unique_fd_impl(unique_fd_impl&& other) : value_(other.release()) {}
+  unique_fd_impl& operator=(unique_fd_impl&& s) {
     reset(s.release());
     return *this;
   }
 
   void reset(int new_value) {
-    if (value_ >= 0)
-      close(value_);
+    if (value_ != -1) {
+      Closer::Close(value_);
+    }
     value_ = new_value;
   }
 
@@ -60,8 +75,9 @@ class unique_fd final {
   }
 
   int get() const { return value_; }
+  operator int() const { return get(); }
 
-  int release() {
+  int release() __attribute__((warn_unused_result)) {
     int ret = value_;
     value_ = -1;
     return ret;
@@ -70,10 +86,13 @@ class unique_fd final {
  private:
   int value_;
 
-  DISALLOW_COPY_AND_ASSIGN(unique_fd);
+  unique_fd_impl(const unique_fd_impl&);
+  void operator=(const unique_fd_impl&);
 };
 
+using unique_fd = unique_fd_impl<DefaultCloser>;
+
 }  // namespace base
 }  // namespace android
 
-#endif // ANDROID_BASE_UNIQUE_FD_H
+#endif  // ANDROID_BASE_UNIQUE_FD_H

base/include/android-base/utf8.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef BASE_UTF8_H
-#define BASE_UTF8_H
+#ifndef ANDROID_BASE_UTF8_H
+#define ANDROID_BASE_UTF8_H
 
 #ifdef _WIN32
 #include <string>
@@ -84,4 +84,4 @@ using ::unlink;
 }  // namespace base
 }  // namespace android
 
-#endif  // BASE_UTF8_H
+#endif  // ANDROID_BASE_UTF8_H

bootstat/boot_event_record_store_test.cpp

@@ -41,7 +41,7 @@ namespace {
 // the value of the record.
 bool CreateEmptyBootEventRecord(const std::string& record_path, int32_t value) {
   android::base::unique_fd record_fd(creat(record_path.c_str(), S_IRUSR | S_IWUSR));
-  if (record_fd.get() == -1) {
+  if (record_fd == -1) {
     return false;
   }
 
@@ -49,7 +49,7 @@ bool CreateEmptyBootEventRecord(const std::string& record_path, int32_t value) {
   // ensure the validity of the file mtime value, i.e., to check that the record
   // file mtime values are not changed once set.
   // TODO(jhawkins): Remove this block.
-  if (!android::base::WriteStringToFd(std::to_string(value), record_fd.get())) {
+  if (!android::base::WriteStringToFd(std::to_string(value), record_fd)) {
     return false;
   }
 

libmemunreachable/ProcessMappings.cpp

@@ -30,11 +30,10 @@
 bool ProcessMappings(pid_t pid, allocator::vector<Mapping>& mappings) {
   char map_buffer[1024];
   snprintf(map_buffer, sizeof(map_buffer), "/proc/%d/maps", pid);
-  int fd = open(map_buffer, O_RDONLY);
-  if (fd < 0) {
+  android::base::unique_fd fd(open(map_buffer, O_RDONLY));
+  if (fd == -1) {
     return false;
   }
-  android::base::unique_fd fd_guard{fd};
 
   LineBuffer line_buf(fd, map_buffer, sizeof(map_buffer));
   char* line;

libmemunreachable/ThreadCapture.cpp

@@ -108,12 +108,11 @@ bool ThreadCaptureImpl::ListThreads(TidList& tids) {
   strlcat(path, pid_str, sizeof(path));
   strlcat(path, "/task", sizeof(path));
 
-  int fd = open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY);
-  if (fd < 0) {
+  android::base::unique_fd fd(open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+  if (fd == -1) {
     ALOGE("failed to open %s: %s", path, strerror(errno));
     return false;
   }
-  android::base::unique_fd fd_guard{fd};
 
   struct linux_dirent64 {
     uint64_t  d_ino;
@@ -125,7 +124,7 @@ bool ThreadCaptureImpl::ListThreads(TidList& tids) {
   char dirent_buf[4096];
   ssize_t nread;
   do {
-    nread = syscall(SYS_getdents64, fd, dirent_buf, sizeof(dirent_buf));
+    nread = syscall(SYS_getdents64, fd.get(), dirent_buf, sizeof(dirent_buf));
     if (nread < 0) {
       ALOGE("failed to get directory entries from %s: %s", path, strerror(errno));
       return false;

libmemunreachable/tests/ThreadCapture_test.cpp

@@ -28,8 +28,6 @@
 
 #include <gtest/gtest.h>
 
-#include <android-base/unique_fd.h>
-
 #include "Allocator.h"
 #include "ScopedDisableMalloc.h"
 #include "ScopedPipe.h"