Reland of 'New run shell implementation for DeviceUtils'

This is a reland of https://codereview.chromium.org/659533002/

Previous CL broke telemetry unittests and perf unittests because it
didn't handle properly shell commands which may end with a newline
character when check_rc is not None.

This revised patch strips any whitespace at the end of shell commands
to prevent this.

BUG=267773

Review URL: https://codereview.chromium.org/669573003

Cr-Commit-Position: refs/heads/master@{#300322}

build/android/pylib/cmd_helper.py

@@ -9,6 +9,7 @@ import os
 import pipes
 import select
 import signal
+import string
 import StringIO
 import subprocess
 import time
@@ -19,6 +20,52 @@ try:
 except ImportError:
   fcntl = None
 
+_SafeShellChars = frozenset(string.ascii_letters + string.digits + '@%_-+=:,./')
+
+def SingleQuote(s):
+  """Return an shell-escaped version of the string using single quotes.
+
+  Reliably quote a string which may contain unsafe characters (e.g. space,
+  quote, or other special characters such as '$').
+
+  The returned value can be used in a shell command line as one token that gets
+  to be interpreted literally.
+
+  Args:
+    s: The string to quote.
+
+  Return:
+    The string quoted using single quotes.
+  """
+  return pipes.quote(s)
+
+def DoubleQuote(s):
+  """Return an shell-escaped version of the string using double quotes.
+
+  Reliably quote a string which may contain unsafe characters (e.g. space
+  or quote characters), while retaining some shell features such as variable
+  interpolation.
+
+  The returned value can be used in a shell command line as one token that gets
+  to be further interpreted by the shell.
+
+  The set of characters that retain their special meaning may depend on the
+  shell implementation. This set usually includes: '$', '`', '\', '!', '*',
+  and '@'.
+
+  Args:
+    s: The string to quote.
+
+  Return:
+    The string quoted using double quotes.
+  """
+  if not s:
+    return '""'
+  elif all(c in _SafeShellChars for c in s):
+    return s
+  else:
+    return '"' + s.replace('"', '\\"') + '"'
+
 
 def Popen(args, stdout=None, stderr=None, shell=None, cwd=None, env=None):
   return subprocess.Popen(
@@ -88,7 +135,7 @@ def GetCmdStatusAndOutput(args, cwd=None, shell=False):
   elif shell:
     raise Exception('array args must be run with shell=False')
   else:
-    args_repr = ' '.join(map(pipes.quote, args))
+    args_repr = ' '.join(map(SingleQuote, args))
 
   s = '[host]'
   if cwd:

build/android/pylib/cmd_helper_test.py

+# Copyright 2013 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Tests for the cmd_helper module."""
+
+import unittest
+
+from pylib import cmd_helper
+
+# TODO(jbudorick) Make these tests run on the bots.
+
+
+class CmdHelperSingleQuoteTest(unittest.TestCase):
+
+  def testSingleQuote_basic(self):
+    self.assertEquals('hello',
+                      cmd_helper.SingleQuote('hello'))
+
+  def testSingleQuote_withSpaces(self):
+    self.assertEquals("'hello world'",
+                      cmd_helper.SingleQuote('hello world'))
+
+  def testSingleQuote_withUnsafeChars(self):
+    self.assertEquals("""'hello'"'"'; rm -rf /'""",
+                      cmd_helper.SingleQuote("hello'; rm -rf /"))
+
+  def testSingleQuote_dontExpand(self):
+    test_string = 'hello $TEST_VAR'
+    cmd = 'TEST_VAR=world; echo %s' % cmd_helper.SingleQuote(test_string)
+    self.assertEquals(test_string,
+                      cmd_helper.GetCmdOutput(cmd, shell=True).rstrip())
+
+
+class CmdHelperDoubleQuoteTest(unittest.TestCase):
+
+  def testDoubleQuote_basic(self):
+    self.assertEquals('hello',
+                      cmd_helper.DoubleQuote('hello'))
+
+  def testDoubleQuote_withSpaces(self):
+    self.assertEquals('"hello world"',
+                      cmd_helper.DoubleQuote('hello world'))
+
+  def testDoubleQuote_withUnsafeChars(self):
+    self.assertEquals('''"hello\\"; rm -rf /"''',
+                      cmd_helper.DoubleQuote('hello"; rm -rf /'))
+
+  def testSingleQuote_doExpand(self):
+    test_string = 'hello $TEST_VAR'
+    cmd = 'TEST_VAR=world; echo %s' % cmd_helper.DoubleQuote(test_string)
+    self.assertEquals('hello world',
+                      cmd_helper.GetCmdOutput(cmd, shell=True).rstrip())

build/android/pylib/device/adb_wrapper.py

@@ -171,18 +171,27 @@ class AdbWrapper(object):
     if expect_rc is None:
       actual_command = command
     else:
-      actual_command = '%s; echo $?;' % command
+      actual_command = '%s; echo %%$?;' % command.rstrip()
     output = self._DeviceAdbCmd(
         ['shell', actual_command], timeout, retries, check_error=False)
     if expect_rc is not None:
-      output_end = output.rstrip().rfind('\n') + 1
-      rc = output[output_end:].strip()
-      output = output[:output_end]
-      if int(rc) != expect_rc:
+      output_end = output.rfind('%')
+      if output_end < 0:
+        # causes the string for rc to become empty and also raise a ValueError
+        output_end = len(output)
+
+      try:
+        rc = int(output[output_end+1:])
+      except ValueError:
         raise device_errors.AdbCommandFailedError(
-            ['shell', command],
-            'shell command exited with code: %s' % rc,
+            ['shell'], 'command %r on device produced output %r where no'
+            ' valid return code was found' % (actual_command, output),
             self._device_serial)
+
+      output = output[:output_end]
+      if rc != expect_rc:
+        raise device_errors.AdbShellCommandFailedError(
+            command, rc, output, self._device_serial)
     return output
 
   def Logcat(self, filter_spec=None, timeout=_DEFAULT_TIMEOUT,

build/android/pylib/device/adb_wrapper_test.py

@@ -41,8 +41,8 @@ class TestAdbWrapper(unittest.TestCase):
     self.assertEqual(output.strip(), 'test')
     output = self._adb.Shell('echo test')
     self.assertEqual(output.strip(), 'test')
-    self.assertRaises(device_errors.AdbCommandFailedError, self._adb.Shell,
-        'echo test', expect_rc=1)
+    self.assertRaises(device_errors.AdbShellCommandFailedError,
+        self._adb.Shell, 'echo test', expect_rc=1)
 
   def testPushPull(self):
     path = self._MakeTempFile('foo')

build/android/pylib/device/device_errors.py

@@ -24,10 +24,23 @@ class AdbCommandFailedError(CommandFailedError):
 
   def __init__(self, cmd, msg, device=None):
     super(AdbCommandFailedError, self).__init__(
-        'adb command \'%s\' failed with message: \'%s\'' % (' '.join(cmd), msg),
+        'adb command %r failed with message: %s' % (' '.join(cmd), msg),
         device=device)
 
 
+class AdbShellCommandFailedError(AdbCommandFailedError):
+  """Exception for adb shell command failing with non-zero return code."""
+
+  def __init__(self, cmd, return_code, output, device=None):
+    super(AdbShellCommandFailedError, self).__init__(
+        ['shell'],
+        'command %r on device failed with return code %d and output %r'
+        % (cmd, return_code, output),
+        device=device)
+    self.return_code = return_code
+    self.output = output
+
+
 class CommandTimeoutError(BaseError):
   """Exception for command timeouts."""
   pass

build/android/pylib/instrumentation/test_runner.py

@@ -358,8 +358,8 @@ class TestRunner(base_test_runner.BaseTestRunner):
 
     cmd = ['am', 'instrument', '-r']
     for k, v in self._GetInstrumentationArgs().iteritems():
-      cmd.extend(['-e', k, "'%s'" % v])
-    cmd.extend(['-e', 'class', "'%s'" % test])
+      cmd.extend(['-e', k, v])
+    cmd.extend(['-e', 'class', test])
     cmd.extend(['-w', instrumentation_path])
     return self.device.RunShellCommand(cmd, timeout=timeout, retries=0)
 

build/android/pylib/instrumentation/test_runner_test.py

@@ -261,8 +261,8 @@ class InstrumentationTestRunnerTest(unittest.TestCase):
     self.instance._RunTest('test.package.TestClass#testMethod', 100)
     self.instance.device.RunShellCommand.assert_called_with(
         ['am', 'instrument', '-r',
-         '-e', 'test_arg_key', "'test_arg_value'",
-         '-e', 'class', "'test.package.TestClass#testMethod'",
+         '-e', 'test_arg_key', 'test_arg_value',
+         '-e', 'class', 'test.package.TestClass#testMethod',
          '-w', 'test.package/MyTestRunner'],
         timeout=100, retries=0)