From c9b3d057b0e66e4e239ee2a10bae9da4fdbfd4f6 Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Mon, 12 Apr 2021 08:04:35 -0700 Subject: [PATCH] [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 | 61 ++++++++++++++ infra/cifuzz/docker_test.py | 151 ++++++++++++++++++++++++++++++++++ infra/cifuzz/fuzz_target.py | 39 +++++---- infra/cifuzz/process_utils.py | 24 ++++++ 4 files changed, 255 insertions(+), 20 deletions(-) create mode 100644 infra/cifuzz/docker_test.py create mode 100644 infra/cifuzz/process_utils.py diff --git a/infra/cifuzz/docker.py b/infra/cifuzz/docker.py index eb993e28d..28ae18096 100644 --- a/infra/cifuzz/docker.py +++ b/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) diff --git a/infra/cifuzz/docker_test.py b/infra/cifuzz/docker_test.py new file mode 100644 index 000000000..9cb0c6778 --- /dev/null +++ b/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) diff --git a/infra/cifuzz/fuzz_target.py b/infra/cifuzz/fuzz_target.py index c623bf60d..e582eadfd 100644 --- a/infra/cifuzz/fuzz_target.py +++ b/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)) - process = subprocess.Popen(command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + if result.timed_out: + logging.info('Stopped docker container before timeout.') - try: - _, 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: + if not result.retcode: 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' % diff --git a/infra/cifuzz/process_utils.py b/infra/cifuzz/process_utils.py new file mode 100644 index 000000000..14788a3c5 --- /dev/null +++ b/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