remote_access: enforce shell=True on shell commands
Using shell strings instead of argv commands is fragile and error prone
and we've long discouraged it for local RunCommands. Update the remote
API with the same shell=True check and convert most existing shell users
over to the argv form.
BUG=None
TEST=precq passes
Change-Id: Ic2d598f33aa0f04bc73350717d1e130d1b58fa1a
Reviewed-on: https://chromium-review.googlesource.com/697065
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
diff --git a/scripts/deploy_chrome.py b/scripts/deploy_chrome.py
index d913e73..9219879 100644
--- a/scripts/deploy_chrome.py
+++ b/scripts/deploy_chrome.py
@@ -50,9 +50,9 @@
KILL_PROC_MAX_WAIT = 10
POST_KILL_WAIT = 2
-MOUNT_RW_COMMAND = 'mount -o remount,rw /'
-LSOF_COMMAND = 'lsof %s/chrome'
-DBUS_RELOAD_COMMAND = 'killall -HUP dbus-daemon'
+# Complicated commands that unittests want to mock out.
+# All others should be inlined.
+MOUNT_RW_COMMAND = ['mount', '-o', 'remount,rw', '/']
_ANDROID_DIR = '/system/chrome'
_ANDROID_DIR_EXTRACT_PATH = 'system/chrome/*'
@@ -60,13 +60,6 @@
_CHROME_DIR = '/opt/google/chrome'
_CHROME_DIR_MOUNT = '/mnt/stateful_partition/deploy_rootfs/opt/google/chrome'
-_UMOUNT_DIR_IF_MOUNTPOINT_CMD = (
- 'if mountpoint -q %(dir)s; then umount %(dir)s; fi')
-_BIND_TO_FINAL_DIR_CMD = 'mount --rbind %s %s'
-_SET_MOUNT_FLAGS_CMD = 'mount -o remount,exec,suid %s'
-
-DF_COMMAND = 'df -k %s'
-
def _UrlBaseName(url):
"""Return the last component of the URL."""
@@ -105,7 +98,7 @@
self.chrome_dir = _CHROME_DIR
def _GetRemoteMountFree(self, remote_dir):
- result = self.device.RunCommand(DF_COMMAND % remote_dir)
+ result = self.device.RunCommand(['df', '-k', remote_dir])
line = result.output.splitlines()[1]
value = line.split()[3]
multipliers = {
@@ -116,7 +109,7 @@
return int(value.rstrip('GMK')) * multipliers.get(value[-1], 1)
def _GetRemoteDirSize(self, remote_dir):
- result = self.device.RunCommand('du -ks %s' % remote_dir,
+ result = self.device.RunCommand(['du', '-ks', remote_dir],
capture_output=True)
return int(result.output.split()[0])
@@ -127,8 +120,9 @@
return int(result.output.split()[0])
def _ChromeFileInUse(self):
- result = self.device.RunCommand(LSOF_COMMAND % (self.options.target_dir,),
- error_code_ok=True, capture_output=True)
+ result = self.device.RunCommand(
+ ['lsof', os.path.join(self.options.target_dir, 'chrome')],
+ error_code_ok=True, capture_output=True)
return result.returncode == 0
def _Reboot(self):
@@ -148,17 +142,17 @@
# Since we stopped Chrome earlier, it's good form to start it up again.
if self.options.startui:
logging.info('Starting Chrome...')
- self.device.RunCommand('start ui')
+ self.device.RunCommand(['start', 'ui'])
raise DeployFailure('Need rootfs verification to be disabled. '
'Aborting.')
logging.info('Removing rootfs verification from %s', self.options.to)
# Running in VM's cause make_dev_ssd's firmware sanity checks to fail.
# Use --force to bypass the checks.
- cmd = ('/usr/share/vboot/bin/make_dev_ssd.sh --partitions %d '
- '--remove_rootfs_verification --force')
+ cmd = ['/usr/share/vboot/bin/make_dev_ssd.sh', '--force',
+ '--remove_rootfs_verification', '--partitions']
for partition in (KERNEL_A_PARTITION, KERNEL_B_PARTITION):
- self.device.RunCommand(cmd % partition, error_code_ok=True)
+ self.device.RunCommand(cmd + [str(partition)], error_code_ok=True)
self._Reboot()
@@ -173,7 +167,7 @@
# <job_name> <status> ['process' <pid>].
# <status> is in the format <goal>/<state>.
try:
- result = self.device.RunCommand('status ui', capture_output=True)
+ result = self.device.RunCommand(['status', 'ui'], capture_output=True)
except cros_build_lib.RunCommandError as e:
if 'Unknown job' in e.result.error:
return False
@@ -185,7 +179,7 @@
def _KillProcsIfNeeded(self):
if self._CheckUiJobStarted():
logging.info('Shutting down Chrome...')
- self.device.RunCommand('stop ui')
+ self.device.RunCommand(['stop', 'ui'])
# Developers sometimes run session_manager manually, in which case we'll
# need to help shut the chrome processes down.
@@ -195,7 +189,7 @@
logging.warning('The chrome binary on the device is in use.')
logging.warning('Killing chrome and session_manager processes...\n')
- self.device.RunCommand("pkill 'chrome|session_manager'",
+ self.device.RunCommand(['pkill', 'chrome|session_manager'],
error_code_ok=True)
# Wait for processes to actually terminate
time.sleep(POST_KILL_WAIT)
@@ -274,23 +268,26 @@
for p in self.copy_paths:
if p.mode:
# Set mode if necessary.
- self.device.RunCommand('chmod %o %s/%s' % (
- p.mode, self.options.target_dir, p.src if not p.dest else p.dest))
+ self.device.RunCommand(
+ ['chmod', '%o' % p.mode,
+ '%s/%s' % (self.options.target_dir,
+ p.src if not p.dest else p.dest)])
# Send SIGHUP to dbus-daemon to tell it to reload its configs. This won't
# pick up major changes (bus type, logging, etc.), but all we care about is
# getting the latest policy from /opt/google/chrome/dbus so that Chrome will
# be authorized to take ownership of its service names.
- self.device.RunCommand(DBUS_RELOAD_COMMAND, error_code_ok=True)
+ self.device.RunCommand(['killall', '-HUP', 'dbus-daemon'],
+ error_code_ok=True)
if self.options.startui:
logging.info('Starting UI...')
- self.device.RunCommand('start ui')
+ self.device.RunCommand(['start', 'ui'])
def _CheckConnection(self):
try:
logging.info('Testing connection to the device...')
- self.device.RunCommand('true')
+ self.device.RunCommand(['true'])
except cros_build_lib.RunCommandError as ex:
logging.error('Error connecting to the test device.')
raise DeployFailure(ex)
@@ -318,15 +315,16 @@
logging.info('Mounting Chrome...')
# Create directory if does not exist
- self.device.RunCommand('mkdir -p --mode 0775 %s' % (
- self.options.mount_dir,))
+ self.device.RunCommand(['mkdir', '-p', '--mode', '0775',
+ self.options.mount_dir])
# Umount the existing mount on mount_dir if present first
- self.device.RunCommand(_UMOUNT_DIR_IF_MOUNTPOINT_CMD %
- {'dir': self.options.mount_dir})
- self.device.RunCommand(_BIND_TO_FINAL_DIR_CMD % (self.options.target_dir,
- self.options.mount_dir))
+ cmd = 'if mountpoint -q %(dir)s; then umount %(dir)s; fi'
+ self.device.RunCommand(cmd % {'dir': self.options.mount_dir}, shell=True)
+ self.device.RunCommand(['mount', '--rbind', self.options.target_dir,
+ self.options.mount_dir])
# Chrome needs partition to have exec and suid flags set
- self.device.RunCommand(_SET_MOUNT_FLAGS_CMD % (self.options.mount_dir,))
+ self.device.RunCommand(['mount', '-o', 'remount,exec,suid',
+ self.options.mount_dir])
def Cleanup(self):
"""Clean up RemoteDevice."""