[testrunner] Prevent erroneous overriding of signal handlers

mi-ac · Commit Bot · commit d64a48dbdf33 · 2020-01-16T14:44:14.000Z
When an overall timeout is reached, swarming sends a SIGTERM to terminate the test runner. The test runner has a signal handler on the main process to terminate all workers gracefully. Additionally, every worker process installs a signal handler for terminating ongoing tests wrapped by command.Command. Also, command.Command is used on the main process to list tests for cctest and gtest executables, which led to overriding the test runner's main signal handler. This CL disables using signal handlers in commands by default and only explicitly enables it in safe source locations. Bug: v8:8292 Change-Id: Ifceadaff75bdd2b77e761498bccbe00b6a3e265c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2002528 Reviewed-by: Tamer Tas <tmrts@chromium.org> Commit-Queue: Michael Achenbach <machenbach@chromium.org> Cr-Commit-Position: refs/heads/master@{#65815}
diff --git a/tools/predictable_wrapper.py b/tools/predictable_wrapper.py
@@ -37,7 +37,8 @@ def allocation_str(stdout):
         return line
     return None
 
-  cmd = command.Command(args[0], args[1:], timeout=TIMEOUT)
+  cmd = command.Command(
+      args[0], args[1:], timeout=TIMEOUT, handle_sigterm=True)
 
   previous_allocations = None
   for run in range(1, MAX_TRIES + 1):
diff --git a/tools/run_perf.py b/tools/run_perf.py
@@ -479,7 +479,8 @@ def GetCommand(self, cmd_prefix, shell_dir, extra_flags=None):
         cmd_prefix=cmd_prefix,
         shell=os.path.join(shell_dir, self.binary),
         args=self.GetCommandFlags(extra_flags=extra_flags),
-        timeout=self.timeout or 60)
+        timeout=self.timeout or 60,
+        handle_sigterm=True)
 
   def ProcessOutput(self, output, result_tracker, count):
     """Processes test run output and updates result tracker.
diff --git a/tools/testrunner/local/command.py b/tools/testrunner/local/command.py
@@ -41,7 +41,7 @@ class AbortException(Exception):
 
 class BaseCommand(object):
   def __init__(self, shell, args=None, cmd_prefix=None, timeout=60, env=None,
-               verbose=False, resources_func=None):
+               verbose=False, resources_func=None, handle_sigterm=False):
     """Initialize the command.
 
     Args:
@@ -52,6 +52,9 @@ def __init__(self, shell, args=None, cmd_prefix=None, timeout=60, env=None,
       env: Environment dict for execution.
       verbose: Print additional output.
       resources_func: Callable, returning all test files needed by this command.
+      handle_sigterm: Flag indicating if SIGTERM will be used to terminate the
+          underlying process. Should not be used from the main thread, e.g. when
+          using a command to list tests.
     """
     assert(timeout > 0)
 
@@ -61,6 +64,7 @@ def __init__(self, shell, args=None, cmd_prefix=None, timeout=60, env=None,
     self.timeout = timeout
     self.env = env or {}
     self.verbose = verbose
+    self.handle_sigterm = handle_sigterm
 
   def execute(self):
     if self.verbose:
@@ -72,7 +76,9 @@ def execute(self):
     abort_occured = [False]
     def handler(signum, frame):
       self._abort(process, abort_occured)
-    signal.signal(signal.SIGTERM, handler)
+
+    if self.handle_sigterm:
+      signal.signal(signal.SIGTERM, handler)
 
     # Variable to communicate with the timer.
     timeout_occured = [False]
diff --git a/tools/testrunner/objects/testcase.py b/tools/testrunner/objects/testcase.py
@@ -271,6 +271,7 @@ def _create_cmd(self, shell, params, env, timeout):
       timeout=timeout,
       verbose=self._test_config.verbose,
       resources_func=self._get_resources,
+      handle_sigterm=True,
     )
 
   def _parse_source_flags(self, source=None):
diff --git a/tools/unittests/run_perf_test.py b/tools/unittests/run_perf_test.py
@@ -191,7 +191,8 @@ def _VerifyMock(self, binary, *args, **kwargs):
         cmd_prefix=[],
         shell=shell,
         args=list(args),
-        timeout=kwargs.get('timeout', 60))
+        timeout=kwargs.get('timeout', 60),
+        handle_sigterm=True)
 
   def _VerifyMockMultiple(self, *args, **kwargs):
     self.assertEqual(len(args), len(command.Command.call_args_list))
@@ -200,7 +201,8 @@ def _VerifyMockMultiple(self, *args, **kwargs):
         'cmd_prefix': [],
         'shell': os.path.join(os.path.dirname(BASE_DIR), arg[0]),
         'args': list(arg[1:]),
-        'timeout': kwargs.get('timeout', 60)
+        'timeout': kwargs.get('timeout', 60),
+        'handle_sigterm': True,
       }
       self.assertTupleEqual((expected, ), actual)
 

Original file line number	Diff line number	Diff line change
`@@ -271,6 +271,7 @@ def _create_cmd(self, shell, params, env, timeout):`
`271`	`271`	`timeout=timeout,`
`272`	`272`	`verbose=self._test_config.verbose,`
`273`	`273`	`resources_func=self._get_resources,`
	`274`	`+ handle_sigterm=True,`
`274`	`275`	`)`
`275`	`276`
`276`	`277`	`def _parse_source_flags(self, source=None):`