Merge "adb: Fix PTY logic for non-interactive shells."

adb/device.py

@@ -101,6 +101,19 @@ def get_device(serial=None, product=None):
 
 
 class AndroidDevice(object):
+    # Delimiter string to indicate the start of the exit code.
+    _RETURN_CODE_DELIMITER = 'x'
+
+    # Follow any shell command with this string to get the exit
+    # status of a program since this isn't propagated by adb.
+    #
+    # The delimiter is needed because `printf 1; echo $?` would print
+    # "10", and we wouldn't be able to distinguish the exit code.
+    _RETURN_CODE_PROBE_STRING = 'echo "{0}$?"'.format(_RETURN_CODE_DELIMITER)
+
+    # Maximum search distance from the output end to find the delimiter.
+    _RETURN_CODE_SEARCH_LENGTH = len('{0}255\n'.format(_RETURN_CODE_DELIMITER))
+
     def __init__(self, serial, product=None):
         self.serial = serial
         self.product = product
@@ -110,40 +123,44 @@ class AndroidDevice(object):
         if self.product is not None:
             self.adb_cmd.extend(['-p', product])
         self._linesep = None
-        self._shell_result_pattern = None
 
     @property
     def linesep(self):
         if self._linesep is None:
-            self._linesep = subprocess.check_output(['adb', 'shell', 'echo'])
+            self._linesep = subprocess.check_output(self.adb_cmd +
+                                                    ['shell', 'echo'])
         return self._linesep
 
     def _make_shell_cmd(self, user_cmd):
-        # Follow any shell command with `; echo; echo $?` to get the exit
+        return (self.adb_cmd + ['shell'] + user_cmd +
-        # status of a program since this isn't propagated by adb.
+                ['; ' + self._RETURN_CODE_PROBE_STRING])
-        #
-        # The leading newline is needed because `printf 1; echo $?` would print
-        # "10", and we wouldn't be able to distinguish the exit code.
-        rc_probe = '; echo "\n$?"'
-        return self.adb_cmd + ['shell'] + user_cmd + [rc_probe]
 
-    def _parse_shell_output(self, out):  # pylint: disable=no-self-use
+    def _parse_shell_output(self, out):
+        """Finds the exit code string from shell output.
+
+        Args:
+            out: Shell output string.
+
+        Returns:
+            An (exit_code, output_string) tuple. The output string is
+            cleaned of any additional stuff we appended to find the
+            exit code.
+
+        Raises:
+            RuntimeError: Could not find the exit code in |out|.
+        """
         search_text = out
-        max_result_len = len('{0}255{0}'.format(self.linesep))
+        if len(search_text) > self._RETURN_CODE_SEARCH_LENGTH:
-        if len(search_text) > max_result_len:
+            # We don't want to search over massive amounts of data when we know
-            # We don't want to regex match over massive amounts of data when we
+            # the part we want is right at the end.
-            # know the part we want is right at the end.
+            search_text = search_text[-self._RETURN_CODE_SEARCH_LENGTH:]
-            search_text = search_text[-max_result_len:]
+        partition = search_text.rpartition(self._RETURN_CODE_DELIMITER)
-        if self._shell_result_pattern is None:
+        if partition[1] == '':
-            self._shell_result_pattern = re.compile(
-                r'({0}\d+{0})$'.format(self.linesep), re.MULTILINE)
-        m = self._shell_result_pattern.search(search_text)
-        if m is None:
             raise RuntimeError('Could not find exit status in shell output.')
-
+        result = int(partition[2])
-        result_text = m.group(1)
+        # partition[0] won't contain the full text if search_text was truncated,
-        result = int(result_text.strip())
+        # pull from the original string instead.
-        out = out[:-len(result_text)]  # Trim the result text from the output.
+        out = out[:-len(partition[1]) - len(partition[2])]
         return result, out
 
     def _simple_call(self, cmd):

adb/services.cpp

@@ -59,6 +59,10 @@ struct stinfo {
     void *cookie;
 };
 
+enum class SubprocessType {
+    kPty,
+    kRaw,
+};
 
 void *service_bootstrap_func(void *x)
 {
@@ -389,17 +393,27 @@ static void subproc_waiter_service(int fd, void *cookie)
     }
 }
 
-static int create_subproc_thread(const char *name, bool pty = false) {
+// Starts a subprocess and spawns a thread to wait for the subprocess to finish
+// and trigger the necessary cleanup.
+//
+// |name| is the command to execute in the subprocess; empty string will start
+//     an interactive session.
+// |type| selects between a PTY or raw subprocess.
+//
+// Returns an open file descriptor tied to the subprocess stdin/stdout/stderr.
+static int create_subproc_thread(const char *name, SubprocessType type) {
     const char *arg0, *arg1;
-    if (name == 0 || *name == 0) {
+    if (*name == '\0') {
-        arg0 = "-"; arg1 = 0;
+        arg0 = "-";
+        arg1 = nullptr;
     } else {
-        arg0 = "-c"; arg1 = name;
+        arg0 = "-c";
+        arg1 = name;
     }
 
     pid_t pid = -1;
     int ret_fd;
-    if (pty) {
+    if (type == SubprocessType::kPty) {
         ret_fd = create_subproc_pty(SHELL_COMMAND, arg0, arg1, &pid);
     } else {
         ret_fd = create_subproc_raw(SHELL_COMMAND, arg0, arg1, &pid);
@@ -466,9 +480,16 @@ int service_to_fd(const char *name)
     } else if (!strncmp(name, "jdwp:", 5)) {
         ret = create_jdwp_connection_fd(atoi(name+5));
     } else if(!strncmp(name, "shell:", 6)) {
-        ret = create_subproc_thread(name + 6, true);
+        const char* args = name + 6;
+        if (*args) {
+            // Non-interactive session uses a raw subprocess.
+            ret = create_subproc_thread(args, SubprocessType::kRaw);
+        } else {
+            // Interactive session uses a PTY subprocess.
+            ret = create_subproc_thread(args, SubprocessType::kPty);
+        }
     } else if(!strncmp(name, "exec:", 5)) {
-        ret = create_subproc_thread(name + 5);
+        ret = create_subproc_thread(name + 5, SubprocessType::kRaw);
     } else if(!strncmp(name, "sync:", 5)) {
         ret = create_service_thread(file_sync_service, NULL);
     } else if(!strncmp(name, "remount:", 8)) {
@@ -482,10 +503,13 @@ int service_to_fd(const char *name)
     } else if(!strncmp(name, "unroot:", 7)) {
         ret = create_service_thread(restart_unroot_service, NULL);
     } else if(!strncmp(name, "backup:", 7)) {
-        ret = create_subproc_thread(android::base::StringPrintf("/system/bin/bu backup %s",
+        ret = create_subproc_thread(
-                                                                (name + 7)).c_str());
+                android::base::StringPrintf("/system/bin/bu backup %s",
+                                            (name + 7)).c_str(),
+                SubprocessType::kRaw);
     } else if(!strncmp(name, "restore:", 8)) {
-        ret = create_subproc_thread("/system/bin/bu restore");
+        ret = create_subproc_thread("/system/bin/bu restore",
+                                    SubprocessType::kRaw);
     } else if(!strncmp(name, "tcpip:", 6)) {
         int port;
         if (sscanf(name + 6, "%d", &port) != 1) {

adb/test_device.py

@@ -155,6 +155,31 @@ class ShellTest(DeviceTest):
         output = self.device.shell(['uname'])
         self.assertEqual(output, 'Linux' + self.device.linesep)
 
+    def test_pty_logic(self):
+        """Verify PTY logic for shells.
+
+        Interactive shells should use a PTY, non-interactive should not.
+
+        Bug: http://b/21215503
+        """
+        proc = subprocess.Popen(
+                self.device.adb_cmd + ['shell'], stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        # [ -t 0 ] is used (rather than `tty`) to provide portability. This
+        # gives an exit code of 0 iff stdin is connected to a terminal.
+        #
+        # Closing host-side stdin doesn't currently trigger the interactive
+        # shell to exit so we need to explicitly add an exit command to
+        # close the session from the device side, and append \n to complete
+        # the interactive command.
+        result = proc.communicate('[ -t 0 ]; echo x$?; exit 0\n')[0]
+        partition = result.rpartition('x')
+        self.assertEqual(partition[1], 'x')
+        self.assertEqual(int(partition[2]), 0)
+
+        exit_code = self.device.shell_nocheck(['[ -t 0 ]'])[0]
+        self.assertEqual(exit_code, 1)
+
 
 class ArgumentEscapingTest(DeviceTest):
     def test_shell_escaping(self):