[CIFuzz] End fuzzing docker processes properly (#5473)

They only right way to do this properly seems to be using docker's container id file with docker stop.
Fixes #5423

infra/cifuzz/docker.py

@@ -12,8 +12,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Module for dealing with docker."""
+import logging
 import os
+import subprocess
 import sys
+import tempfile
+
+import process_utils
 
 # pylint: disable=wrong-import-position,import-error
 sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
@@ -36,3 +41,59 @@ def delete_images(images):
   command = ['docker', 'rmi', '-f'] + images
   utils.execute(command)
   utils.execute(['docker', 'builder', 'prune', '-f'])
+
+
+def stop_docker_container(container_id, wait_time=1):
+  """Stops the docker container, |container_id|. Returns True on success."""
+  result = subprocess.run(
+      ['docker', 'stop', container_id, '-t',
+       str(wait_time)], check=False)
+  return result.returncode == 0
+
+
+def _handle_timed_out_container_process(process, cid_filename):
+  """Stops the docker container |process| (and child processes) that has a
+  container id in |cid_filename|. Returns stdout and stderr of |process|. This
+  function is a helper for run_container_command and should only be invoked by
+  it. Returns None for each if we can't get stdout and stderr."""
+  # Be cautious here. We probably aren't doing anything essential for CIFuzz to
+  # function. So try extra hard not to throw uncaught exceptions.
+  try:
+    with open(cid_filename, 'r') as cid_file_handle:
+      container_id = cid_file_handle.read()
+  except FileNotFoundError:
+    logging.error('cid_file not found.')
+    return None, None
+
+  if not stop_docker_container(container_id):
+    logging.error('Failed to stop docker container: %s', container_id)
+    return None, None
+
+  # Use a timeout so we don't wait forever.
+  return process.communicate(timeout=1)
+
+
+def run_container_command(command_arguments, timeout=None):
+  """Runs |command_arguments| as a "docker run" command. Returns ProcessResult.
+  Stops the command if timeout is reached."""
+  command = ['docker', 'run', '--rm', '--privileged']
+  timed_out = False
+  with tempfile.TemporaryDirectory() as temp_dir:
+    # Use temp dir instead of file because docker complains if file exists
+    # already.
+    cid_file_path = os.path.join(temp_dir, 'cidfile')
+    command.extend(['--cidfile', cid_file_path])
+    command.extend(command_arguments)
+    logging.info('Running command: %s', ' '.join(command))
+    process = subprocess.Popen(command,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE)
+    try:
+      stdout, stderr = process.communicate(timeout=timeout)
+    except subprocess.TimeoutExpired:
+      logging.warning('Command timed out: %s', ' '.join(command))
+      stdout, stderr = _handle_timed_out_container_process(
+          process, cid_file_path)
+      timed_out = True
+
+    return process_utils.ProcessResult(process, stdout, stderr, timed_out)

infra/cifuzz/docker_test.py

@@ -0,0 +1,151 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Tests the functionality of the fuzz_target module."""
+
+import subprocess
+import unittest
+from unittest.mock import call
+from unittest import mock
+
+from pyfakefs import fake_filesystem_unittest
+
+import docker
+
+# pylint: disable=no-self-use,too-few-public-methods,protected-access
+
+
+class TestGetProjectImageName(unittest.TestCase):
+  """Tests for get_project_image_name."""
+
+  def test_get_project_image_name(self):
+    """Tests that get_project_image_name works as intended."""
+    project_name = 'myproject'
+    result = docker.get_project_image_name(project_name)
+    self.assertEqual(result, 'gcr.io/oss-fuzz/myproject')
+
+
+class TestDeleteImages(unittest.TestCase):
+  """Tests for get_project_image_name."""
+
+  @mock.patch('utils.execute')
+  def test_delete_images(self, mocked_execute):
+    """Tests thart delete_images deletes images."""
+    images = ['myimage1', 'myimage2']
+    docker.delete_images(images)
+    mocked_execute.assert_has_calls([
+        call(['docker', 'rmi', '-f'] + images),
+        call(['docker', 'builder', 'prune', '-f'])
+    ])
+
+
+class TestStopDockerContainer(unittest.TestCase):
+  """Tests for stop_docker_container."""
+
+  @mock.patch('subprocess.run', return_value=mock.MagicMock(returncode=0))
+  def test_stop_docker_container(self, mocked_run):
+    """Tests that stop_docker_container works as intended."""
+    container_id = 'container-id'
+    wait_time = 100
+    result = docker.stop_docker_container(container_id, wait_time)
+    mocked_run.assert_called_with(
+        ['docker', 'stop', container_id, '-t',
+         str(wait_time)], check=False)
+    self.assertTrue(result)
+
+
+class TestHandleTimedOutContainerProcess(fake_filesystem_unittest.TestCase):
+  """Tests for _handle_timed_out_container_process."""
+  ERROR_EXPECTED_RESULT = (None, None)
+  CONTAINER_ID = 'container-id'
+  CID_FILENAME = '/cid-file'
+
+  def setUp(self):
+    self.setUpPyfakefs()
+    self.fs.create_file(self.CID_FILENAME, contents=self.CONTAINER_ID)
+
+  @mock.patch('logging.error')
+  def test_unreadable_file(self, mocked_error):
+    """Tests that _handle_timed_out_container_process doesn't exception when the
+    cidfile doesn't exist."""
+    fake_cid_file = '/tmp/my-fake/cid-file'
+    result = docker._handle_timed_out_container_process(mock.MagicMock(),
+                                                        fake_cid_file)
+    self.assertEqual(result, self.ERROR_EXPECTED_RESULT)
+    mocked_error.assert_called_with('cid_file not found.')
+
+  @mock.patch('logging.error')
+  @mock.patch('docker.stop_docker_container')
+  def test_stop_docker_container_failed(self, mocked_stop_docker_container,
+                                        mocked_error):
+    """Tests that _handle_timed_out_container_process behaves properly when it
+    fails to stop the docker container."""
+    mocked_stop_docker_container.return_value = False
+
+    result = docker._handle_timed_out_container_process(mock.MagicMock(),
+                                                        self.CID_FILENAME)
+
+    mocked_stop_docker_container.assert_called_with(self.CONTAINER_ID)
+    self.assertEqual(result, self.ERROR_EXPECTED_RESULT)
+    mocked_error.assert_called_with('Failed to stop docker container: %s',
+                                    self.CONTAINER_ID)
+
+  @mock.patch('logging.error')
+  @mock.patch('docker.stop_docker_container')
+  def test_handle_timed_out_container_process(self,
+                                              mocked_stop_docker_container,
+                                              mocked_error):
+    """Tests that test_handle_timed_out_container_process works as intended."""
+    mocked_stop_docker_container.return_value = True
+    process = mock.MagicMock()
+    process.communicate = lambda *args, **kwargs: None
+    result = docker._handle_timed_out_container_process(process,
+                                                        self.CID_FILENAME)
+
+    # communicate returns None because of the way we mocked Popen.
+    self.assertIsNone(result)
+
+    mocked_error.assert_not_called()
+
+
+class TestRunContainerCommand(unittest.TestCase):
+  """Tests for run_container_command."""
+  ARGUMENTS = ['argument']
+
+  @mock.patch('docker._handle_timed_out_container_process',
+              return_value=(None, None))
+  @mock.patch('logging.warning')
+  @mock.patch('subprocess.Popen')
+  def test_timeout(self, mocked_popen, mocked_warning, _):
+    """Tests run_container_command behaves as expected when the command times
+    out."""
+    popen_magic_mock = mock.MagicMock()
+    mocked_popen.return_value = popen_magic_mock
+    popen_magic_mock.communicate.side_effect = subprocess.TimeoutExpired(
+        ['cmd'], '1')
+    result = docker.run_container_command(self.ARGUMENTS)
+    self.assertEqual(mocked_warning.call_count, 1)
+    self.assertTrue(result.timed_out)
+
+  @mock.patch('docker._handle_timed_out_container_process')
+  @mock.patch('subprocess.Popen')
+  def test_run_container_command(self, mocked_popen,
+                                 mocked_handle_timed_out_container_process):
+    """Tests run_container_command behaves as expected."""
+    popen_magic_mock = mock.MagicMock()
+    mocked_popen.return_value = popen_magic_mock
+    popen_magic_mock.communicate.return_value = (None, None)
+    mocked_handle_timed_out_container_process.return_value = (None, None)
+    result = docker.run_container_command(self.ARGUMENTS)
+    mocked_handle_timed_out_container_process.assert_not_called()
+    self.assertFalse(result.timed_out)

infra/cifuzz/fuzz_target.py

@@ -18,7 +18,6 @@ import os
 import re
 import shutil
 import stat
-import subprocess
 import sys
 
 import docker
@@ -93,15 +92,15 @@ class FuzzTarget:
     """
     logging.info('Fuzzer %s, started.', self.target_name)
     docker_container = utils.get_container_name()
-    command = ['docker', 'run', '--rm', '--privileged']
+    command_arguments = []
     if docker_container:
-      command += [
+      command_arguments += [
           '--volumes-from', docker_container, '-e', 'OUT=' + self.out_dir
       ]
     else:
-      command += ['-v', '%s:%s' % (self.out_dir, '/out')]
+      command_arguments += ['-v', '%s:%s' % (self.out_dir, '/out')]
 
-    command += [
+    command_arguments += [
         '-e', 'FUZZING_ENGINE=libfuzzer', '-e',
         'SANITIZER=' + self.config.sanitizer, '-e', 'CIFUZZ=True', '-e',
         'RUN_FUZZER_MODE=interactive', docker.BASE_RUNNER_TAG, 'bash', '-c'
@@ -116,30 +115,30 @@ class FuzzTarget:
         self.target_name, self.out_dir)
     if self.latest_corpus_path:
       run_fuzzer_command = run_fuzzer_command + ' ' + self.latest_corpus_path
-    command.append(run_fuzzer_command)
+    command_arguments.append(run_fuzzer_command)
+    result = docker.run_container_command(command_arguments,
+                                          timeout=self.duration + BUFFER_TIME)
 
-    logging.info('Running command: %s', ' '.join(command))
+    if result.timed_out:
-    process = subprocess.Popen(command,
+      logging.info('Stopped docker container before timeout.')
-                               stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE)
 
-    try:
+    if not result.retcode:
-      _, stderr = process.communicate(timeout=self.duration + BUFFER_TIME)
-    except subprocess.TimeoutExpired:
-      logging.error('Fuzzer %s timed out, ending fuzzing.', self.target_name)
-      return FuzzResult(None, None)
-
-    # Libfuzzer timeout was reached.
-    if not process.returncode:
       logging.info('Fuzzer %s finished with no crashes discovered.',
                    self.target_name)
       return FuzzResult(None, None)
 
+    if result.stderr is None:
+      return FuzzResult(None, None)
+
     # Crash was discovered.
     logging.info('Fuzzer %s, ended before timeout.', self.target_name)
-    testcase = self.get_testcase(stderr)
+
+    # TODO(metzman): Replace this with artifact_prefix so we don't have to
+    # parse.
+    testcase = self.get_testcase(result.stderr)
+
     if not testcase:
-      logging.error(b'No testcase found in stacktrace: %s.', stderr)
+      logging.error(b'No testcase found in stacktrace: %s.', result.stderr)
       return FuzzResult(None, None)
 
     utils.binary_print(b'Fuzzer: %s. Detected bug:\n%s' %

infra/cifuzz/process_utils.py

@@ -0,0 +1,24 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Module for dealing with processes."""
+
+
+class ProcessResult:  # pylint: disable=too-few-public-methods
+  """Class that represents the result of a finished processs."""
+
+  def __init__(self, process, stdout, stderr, timed_out=False):
+    self.retcode = process.returncode
+    self.stdout = stdout
+    self.stderr = stderr
+    self.timed_out = timed_out