From 57dd5ae1e3004daec664263e24dc4dcf4475bb02 Mon Sep 17 00:00:00 2001
From: David Pursell <dpursell@google.com>
Date: Wed, 27 Jan 2016 16:07:52 -0800
Subject: [PATCH] adb: fix subprocess termination for legacy shell.

http://r.android.com/166419 changed `adb shell` behavior to not
allocate a remote PTY for non-interactive commands, but adbd relied on
having a PTY to properly terminate the subprocess.

One impact of this is that when using older versions of adb or passing
the -x flag, `adb screenrecord` wasn't properly terminating and closing
out the video file.

This CL restores the old behavior for legacy shell connections: always
use a PTY, but put it in raw mode if the client is doing local PTY
input/output processing itself.

Bug: http://b/26742824
Change-Id: I9ee630c0ff0d2d6a0db367387af7123deea79676
---
 adb/commandline.cpp        | 15 +++++++++++----
 adb/shell_service.cpp      | 31 +++++++++++++++++++++++++++++++
 adb/shell_service_test.cpp |  5 +++--
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/adb/commandline.cpp b/adb/commandline.cpp
index f88669808..1f5d29f12 100644
--- a/adb/commandline.cpp
+++ b/adb/commandline.cpp
@@ -742,10 +742,6 @@ static int adb_shell(int argc, const char** argv,
             argc -= 2;
             argv += 2;
         } else if (!strcmp(argv[0], "-T") || !strcmp(argv[0], "-t")) {
-            if (!CanUseFeature(features, kFeatureShell2)) {
-                fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
-                return 1;
-            }
             // Like ssh, -t arguments are cumulative so that multiple -t's
             // are needed to force a PTY.
             if (argv[0][1] == 't') {
@@ -769,6 +765,17 @@ static int adb_shell(int argc, const char** argv,
         }
     }
 
+    // Legacy shell protocol requires a remote PTY to close the subprocess properly which creates
+    // some weird interactions with -tT.
+    if (!use_shell_protocol && t_arg_count != 0) {
+        if (!CanUseFeature(features, kFeatureShell2)) {
+            fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
+        } else {
+            fprintf(stderr, "error: PTY args -Tt cannot be used with -x\n");
+        }
+        return 1;
+    }
+
     std::string shell_type_arg;
     if (CanUseFeature(features, kFeatureShell2)) {
         if (t_arg_count < 0) {
diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index e092dc48c..b9a22dc19 100644
--- a/adb/shell_service.cpp
+++ b/adb/shell_service.cpp
@@ -210,6 +210,7 @@ class Subprocess {
 
     const std::string command_;
     const std::string terminal_type_;
+    bool make_pty_raw_ = false;
     SubprocessType type_;
     SubprocessProtocol protocol_;
     pid_t pid_ = -1;
@@ -229,6 +230,18 @@ Subprocess::Subprocess(const std::string& command, const char* terminal_type,
       terminal_type_(terminal_type ? terminal_type : ""),
       type_(type),
       protocol_(protocol) {
+    // If we aren't using the shell protocol we must allocate a PTY to properly close the
+    // subprocess. PTYs automatically send SIGHUP to the slave-side process when the master side
+    // of the PTY closes, which we rely on. If we use a raw pipe, processes that don't read/write,
+    // e.g. screenrecord, will never notice the broken pipe and terminate.
+    // The shell protocol doesn't require a PTY because it's always monitoring the local socket FD
+    // with select() and will send SIGHUP manually to the child process.
+    if (protocol_ == SubprocessProtocol::kNone && type_ == SubprocessType::kRaw) {
+        // Disable PTY input/output processing since the client is expecting raw data.
+        D("Can't create raw subprocess without shell protocol, using PTY in raw mode instead");
+        type_ = SubprocessType::kPty;
+        make_pty_raw_ = true;
+    }
 }
 
 Subprocess::~Subprocess() {
@@ -408,6 +421,24 @@ int Subprocess::OpenPtyChildFd(const char* pts_name, ScopedFd* error_sfd) {
         exit(-1);
     }
 
+    if (make_pty_raw_) {
+        termios tattr;
+        if (tcgetattr(child_fd, &tattr) == -1) {
+            int saved_errno = errno;
+            WriteFdExactly(error_sfd->fd(), "tcgetattr failed: ");
+            WriteFdExactly(error_sfd->fd(), strerror(saved_errno));
+            exit(-1);
+        }
+
+        cfmakeraw(&tattr);
+        if (tcsetattr(child_fd, TCSADRAIN, &tattr) == -1) {
+            int saved_errno = errno;
+            WriteFdExactly(error_sfd->fd(), "tcsetattr failed: ");
+            WriteFdExactly(error_sfd->fd(), strerror(saved_errno));
+            exit(-1);
+        }
+    }
+
     return child_fd;
 }
 
diff --git a/adb/shell_service_test.cpp b/adb/shell_service_test.cpp
index c85232b4c..839284ed0 100644
--- a/adb/shell_service_test.cpp
+++ b/adb/shell_service_test.cpp
@@ -175,8 +175,9 @@ TEST_F(ShellServiceTest, RawNoProtocolSubprocess) {
             "echo foo; echo bar >&2; [ -t 0 ]; echo $?",
             SubprocessType::kRaw, SubprocessProtocol::kNone));
 
-    // [ -t 0 ] == 1 means no terminal (raw).
-    ExpectLinesEqual(ReadRaw(subprocess_fd_), {"foo", "bar", "1"});
+    // [ -t 0 ] == 0 means we have a terminal (PTY). Even when requesting a raw subprocess, without
+    // the shell protocol we should always force a PTY to ensure proper cleanup.
+    ExpectLinesEqual(ReadRaw(subprocess_fd_), {"foo", "bar", "0"});
 }
 
 // Tests a PTY subprocess with no protocol.