cr_cronet.py: Replace most os.system uses with subprocess.call

os.system passes the command line to a shell to execute, which requires
careful quoting/escaping (which the script was not doing).
subprocess.call executes the target directly and arguments are not
processed by the shell.

iossim has one argument that is itself a list of arguments to pass to
its child process, so extra care is taken in escaping that argument.

Change-Id: I6785c83d732dc1fe6a874598f13cb4e47eaa35e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1577877Reviewed-by: Misha Efimov <mef@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653335}

components/cronet/tools/cr_cronet.py

@@ -9,60 +9,74 @@ cr_cronet.py - cr - like helper tool for cronet developers
 
 import argparse
 import os
+import pipes
+import subprocess
 import sys
 
-def run(command, extra_options=''):
+
+def quoted_args(args):
+  return ' '.join([pipes.quote(arg) for arg in args])
+
+
+def run(command, **kwargs):
+  print command, kwargs
+  return subprocess.call(command, **kwargs)
+
+
+def run_shell(command, extra_options=''):
   command = command + ' ' + extra_options
   print command
   return os.system(command)
 
 
-def gn(out_dir, gn_args, gn_extra=''):
-  return run ('gn gen %s --args=\'%s\' %s' % (out_dir, gn_args, gn_extra))
+def gn(out_dir, gn_args, gn_extra=[]):
+  return run(['gn', 'gen', out_dir, "--args=%s" % gn_args] + gn_extra)
 
 
-def build(out_dir, build_target, extra_options=''):
-  return run('ninja -C ' + out_dir + ' ' + build_target,
-             get_ninja_jobs_option() + extra_options)
+def build(out_dir, build_target, extra_options=[]):
+  return run(['ninja', '-C', out_dir, build_target] +
+             get_ninja_jobs_options() + extra_options)
 
 
 def install(out_dir):
-  cmd = 'BUILDTYPE=' + out_dir[4:] + ' build/android/adb_install_apk.py {0}'
-  return run(cmd.format('CronetTestInstrumentation.apk')) or \
-    run(cmd.format('ChromiumNetTestSupport.apk'))
+  cmd = ['build/android/adb_install_apk.py']
+  env = {'BUILDTYPE': out_dir[4:]}
+  return run(cmd + ['CronetTestInstrumentation.apk'], env=env) or \
+      run(cmd + ['ChromiumNetTestSupport.apk'], env=env)
 
 
 def test(out_dir, extra_options):
-  return run(out_dir + '/bin/run_cronet_test_instrumentation_apk ' + \
+  return run([out_dir + '/bin/run_cronet_test_instrumentation_apk'] +
              extra_options)
 
 
 def unittest(out_dir, extra_options):
-  return run(out_dir + '/bin/run_cronet_unittests_android ' + \
+  return run([out_dir + '/bin/run_cronet_unittests_android'] +
              extra_options)
 
 
 def test_ios(out_dir, extra_options):
-  return run(out_dir + '/iossim -c "' + extra_options + '" ' + \
-             out_dir + '/cronet_test.app')
+  return run([out_dir + '/iossim', '-c', quoted_args(extra_options),
+             out_dir + '/cronet_test.app'])
 
 
 def unittest_ios(out_dir, extra_options):
-  return run(out_dir + '/iossim -c "' + extra_options + '" ' + \
-             out_dir + '/cronet_unittests_ios.app')
+  return run([out_dir + '/iossim', '-c', quoted_args(extra_options),
+             out_dir + '/cronet_unittests_ios.app'])
 
 
 def debug(extra_options):
-  return run('build/android/adb_gdb --start ' + \
-             '--activity=.CronetTestActivity ' + \
-             '--program-name=CronetTest ' + \
-             '--package-name=org.chromium.net',
+  return run(['build/android/adb_gdb', '--start',
+             '--activity=.CronetTestActivity',
+             '--program-name=CronetTest',
+             '--package-name=org.chromium.net'] +
              extra_options)
 
 
 def stack(out_dir):
-  return run('adb logcat -d | CHROMIUM_OUTPUT_DIR=' + out_dir +
-          ' third_party/android_platform/development/scripts/stack')
+  return run_shell(
+      'adb logcat -d | CHROMIUM_OUTPUT_DIR=' + pipes.quote(out_dir) +
+      ' third_party/android_platform/development/scripts/stack')
 
 
 def use_goma():
@@ -73,10 +87,10 @@ def use_goma():
   return ''
 
 
-def get_ninja_jobs_option():
+def get_ninja_jobs_options():
   if use_goma():
-    return " -j1000 "
-  return ""
+    return ["-j1000"]
+  return []
 
 
 def get_default_gn_args(target_os, is_release):
@@ -138,15 +152,15 @@ def main():
   parser.add_argument('-a', '--asan', action='store_true',
                       help='use address sanitizer')
 
-  options, extra_options_list = parser.parse_known_args()
+  options, extra_options = parser.parse_known_args()
   print options
-  print extra_options_list
+  print extra_options
 
   is_ios = (sys.platform == 'darwin')
   if is_ios:
     test_target = 'cronet_test'
     unit_target = 'cronet_unittests_ios'
-    gn_extra = '--ide=xcode'
+    gn_extra = ['--ide=xcode']
     if options.iphoneos:
       gn_args = get_ios_gn_args(options.release, 'arm64')
       out_dir_suffix = '-iphoneos'
@@ -162,7 +176,7 @@ def main():
     gn_args = get_mobile_gn_args('android', options.release) + \
               'use_errorprone_java_compiler=true enable_reporting=true ' + \
               'use_hashed_jni_names=true '
-    gn_extra = ''
+    gn_extra = []
     out_dir_suffix = ''
     if options.x86:
       gn_args += 'target_cpu="x86" '
@@ -175,8 +189,6 @@ def main():
       gn_args += 'is_asan=true is_clang=true is_debug=false '
       out_dir_suffix += '-asan'
 
-  extra_options = ' '.join(extra_options_list)
-
   if options.release:
     out_dir = 'out/Release' + out_dir_suffix
   else:
@@ -188,14 +200,14 @@ def main():
   if (options.command=='gn'):
     return gn(out_dir, gn_args, gn_extra)
   if (options.command=='sync'):
-    return run('git pull --rebase && gclient sync')
+    return run(['git', 'pull', '--rebase']) or run(['gclient', 'sync'])
   if (options.command=='build'):
     return build(out_dir, test_target, extra_options)
   if (not is_ios):
     if (options.command=='install'):
       return install(out_dir)
     if (options.command=='proguard'):
-      return build (out_dir, 'cronet_sample_proguard_apk')
+      return build(out_dir, 'cronet_sample_proguard_apk')
     if (options.command=='test'):
       return install(out_dir) or test(out_dir, extra_options)
     if (options.command=='build-test'):