From 6f1a7f79115a949c8954ccd76bca1baa66e686f6 Mon Sep 17 00:00:00 2001 From: Jonathan Metzman Date: Tue, 28 Apr 2020 13:36:21 -0700 Subject: [CIFuzz] Support ALLOWED_BROKEN_TARGETS_PERCENTAGE --- infra/cifuzz/cifuzz.py | 9 +++++ infra/cifuzz/cifuzz_test.py | 93 +++++++++++++++++++++++++-------------------- infra/presubmit.py | 3 ++ 3 files changed, 64 insertions(+), 41 deletions(-) diff --git a/infra/cifuzz/cifuzz.py b/infra/cifuzz/cifuzz.py index 660075bf..319a254b 100644 --- a/infra/cifuzz/cifuzz.py +++ b/infra/cifuzz/cifuzz.py @@ -255,6 +255,7 @@ def check_fuzzer_build(out_dir, sanitizer='address'): Returns: True if fuzzers are correct. """ + if not os.path.exists(out_dir): logging.error('Invalid out directory: %s.', out_dir) return False @@ -266,6 +267,14 @@ def check_fuzzer_build(out_dir, sanitizer='address'): '--cap-add', 'SYS_PTRACE', '-e', 'FUZZING_ENGINE=' + DEFAULT_ENGINE, '-e', 'SANITIZER=' + sanitizer, '-e', 'ARCHITECTURE=' + DEFAULT_ARCHITECTURE ] + + allowed_broken_targets_percentage = os.getenv( + 'ALLOWED_BROKEN_TARGETS_PERCENTAGE') + if allowed_broken_targets_percentage is not None: + set_env_var_arg = ('ALLOWED_BROKEN_TARGETS_PERCENTAGE=' + + allowed_broken_targets_percentage) + command += ['-e', set_env_var_arg] + container = utils.get_container_name() if container: command += ['-e', 'OUT=' + out_dir, '--volumes-from', container] diff --git a/infra/cifuzz/cifuzz_test.py b/infra/cifuzz/cifuzz_test.py index 2da8b5ff..99fdecc6 100644 --- a/infra/cifuzz/cifuzz_test.py +++ b/infra/cifuzz/cifuzz_test.py @@ -22,7 +22,7 @@ import shutil import sys import tempfile import unittest -import unittest.mock +from unittest import mock # pylint: disable=wrong-import-position sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -56,6 +56,8 @@ MEMORY_FUZZER = 'curl_fuzzer_memory' UNDEFINED_FUZZER_DIR = os.path.join(TEST_FILES_PATH, 'out', 'undefined') UNDEFINED_FUZZER = 'curl_fuzzer_undefined' +# pylint: disable=no-self-use + class BuildFuzzersIntegrationTest(unittest.TestCase): """Test build_fuzzers function in the utils module.""" @@ -213,12 +215,12 @@ class RunAddressFuzzersIntegrationTest(unittest.TestCase): # Set the first return value to True, then the second to False to # emulate a bug existing in the current PR but not on the downloaded # OSS-Fuzz build. - with unittest.mock.patch.object( - fuzz_target.FuzzTarget, 'is_reproducible', - side_effect=[True, - False]), unittest.mock.patch.object(cifuzz, - 'is_project_sanitizer', - return_value=True): + with mock.patch.object(fuzz_target.FuzzTarget, + 'is_reproducible', + side_effect=[True, False]), mock.patch.object( + cifuzz, + 'is_project_sanitizer', + return_value=True): run_success, bug_found = cifuzz.run_fuzzers(10, TEST_FILES_PATH, EXAMPLE_PROJECT) build_dir = os.path.join(TEST_FILES_PATH, 'out', 'oss_fuzz_latest') @@ -229,12 +231,12 @@ class RunAddressFuzzersIntegrationTest(unittest.TestCase): def test_old_bug_found(self): """Test run_fuzzers with a bug found in OSS-Fuzz before.""" - with unittest.mock.patch.object( - fuzz_target.FuzzTarget, 'is_reproducible', - side_effect=[True, - True]), unittest.mock.patch.object(cifuzz, - 'is_project_sanitizer', - return_value=True): + with mock.patch.object(fuzz_target.FuzzTarget, + 'is_reproducible', + side_effect=[True, True]), mock.patch.object( + cifuzz, + 'is_project_sanitizer', + return_value=True): run_success, bug_found = cifuzz.run_fuzzers(10, TEST_FILES_PATH, EXAMPLE_PROJECT) build_dir = os.path.join(TEST_FILES_PATH, 'out', 'oss_fuzz_latest') @@ -245,7 +247,7 @@ class RunAddressFuzzersIntegrationTest(unittest.TestCase): def test_invalid_build(self): """Test run_fuzzers with an invalid build.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'is_project_sanitizer', return_value=True): out_path = os.path.join(tmp_dir, 'out') os.mkdir(out_path) @@ -255,7 +257,7 @@ class RunAddressFuzzersIntegrationTest(unittest.TestCase): def test_invalid_fuzz_seconds(self): """Tests run_fuzzers with an invalid fuzz seconds.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'is_project_sanitizer', return_value=True): out_path = os.path.join(tmp_dir, 'out') os.mkdir(out_path) @@ -315,6 +317,19 @@ class CheckFuzzerBuildUnitTest(unittest.TestCase): """Checks a directory that exists but does not have fuzzers is False.""" self.assertFalse(cifuzz.check_fuzzer_build(TEST_FILES_PATH)) + # TODO(metzman): Make it easier to patch environ without thinking about it. + @mock.patch.dict(os.environ, {'ALLOWED_BROKEN_TARGETS_PERCENTAGE': '0'}) + @mock.patch('helper.docker_run') + def test_allow_broken_fuzz_targets_percentage(self, mocked_docker_run): + """Tests that ALLOWED_BROKEN_TARGETS_PERCENTAGE is set when running + docker if it is set in the environment.""" + test_fuzzer_dir = os.path.join(TEST_FILES_PATH, 'out') + mocked_docker_run.return_value = 0 + cifuzz.check_fuzzer_build(test_fuzzer_dir) + self.assertIn( + '-e ALLOWED_BROKEN_TARGETS_PERCENTAGE=0', + ' '.join(mocked_docker_run.call_args[0][0])) + class GetFilesCoveredByTargetUnitTest(unittest.TestCase): """Test to get the files covered by a fuzz target in the cifuzz module.""" @@ -335,10 +350,9 @@ class GetFilesCoveredByTargetUnitTest(unittest.TestCase): def test_valid_target(self): """Tests that covered files can be retrieved from a coverage report.""" - with unittest.mock.patch.object( - cifuzz, - 'get_target_coverage_report', - return_value=self.fuzzer_cov_report_example): + with mock.patch.object(cifuzz, + 'get_target_coverage_report', + return_value=self.fuzzer_cov_report_example): file_list = cifuzz.get_files_covered_by_target( self.proj_cov_report_example, self.example_fuzzer, '/src/curl') @@ -379,9 +393,8 @@ class GetTargetCoverageReporUnitTest(unittest.TestCase): def test_valid_target(self): """Test a target's coverage report can be downloaded and parsed.""" - with unittest.mock.patch.object(cifuzz, - 'get_json_from_url', - return_value='{}') as mock_get_json: + with mock.patch.object(cifuzz, 'get_json_from_url', + return_value='{}') as mock_get_json: cifuzz.get_target_coverage_report(self.cov_exmp, self.example_fuzzer) (url,), _ = mock_get_json.call_args self.assertEqual( @@ -413,9 +426,8 @@ class GetLatestCoverageReportUnitTest(unittest.TestCase): NOTE: This test relies on the test_project repo's coverage report. Example was not used because it has no coverage reports. """ - with unittest.mock.patch.object(cifuzz, - 'get_json_from_url', - return_value='{}') as mock_fun: + with mock.patch.object(cifuzz, 'get_json_from_url', + return_value='{}') as mock_fun: cifuzz.get_latest_cov_report_info(self.test_project) (url,), _ = mock_fun.call_args @@ -438,52 +450,51 @@ class KeepAffectedFuzzersUnitTest(unittest.TestCase): def test_keeping_fuzzer_w_no_coverage(self): """Tests that a specific fuzzer is kept if it is deemed affected.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'get_latest_cov_report_info', return_value=1): shutil.copy(self.test_fuzzer_1, tmp_dir) shutil.copy(self.test_fuzzer_2, tmp_dir) - with unittest.mock.patch.object(cifuzz, - 'get_files_covered_by_target', - side_effect=[[self.example_file_changed], - None]): + with mock.patch.object(cifuzz, + 'get_files_covered_by_target', + side_effect=[[self.example_file_changed], None]): cifuzz.remove_unaffected_fuzzers(EXAMPLE_PROJECT, tmp_dir, [self.example_file_changed], '') self.assertEqual(2, len(os.listdir(tmp_dir))) def test_keeping_specific_fuzzer(self): """Tests that a specific fuzzer is kept if it is deemed affected.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'get_latest_cov_report_info', return_value=1): shutil.copy(self.test_fuzzer_1, tmp_dir) shutil.copy(self.test_fuzzer_2, tmp_dir) - with unittest.mock.patch.object(cifuzz, - 'get_files_covered_by_target', - side_effect=[[self.example_file_changed], - ['not/a/real/file']]): + with mock.patch.object(cifuzz, + 'get_files_covered_by_target', + side_effect=[[self.example_file_changed], + ['not/a/real/file']]): cifuzz.remove_unaffected_fuzzers(EXAMPLE_PROJECT, tmp_dir, [self.example_file_changed], '') self.assertEqual(1, len(os.listdir(tmp_dir))) def test_no_fuzzers_kept_fuzzer(self): """Tests that if there is no affected then all fuzzers are kept.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'get_latest_cov_report_info', return_value=1): shutil.copy(self.test_fuzzer_1, tmp_dir) shutil.copy(self.test_fuzzer_2, tmp_dir) - with unittest.mock.patch.object(cifuzz, - 'get_files_covered_by_target', - side_effect=[None, None]): + with mock.patch.object(cifuzz, + 'get_files_covered_by_target', + side_effect=[None, None]): cifuzz.remove_unaffected_fuzzers(EXAMPLE_PROJECT, tmp_dir, [self.example_file_changed], '') self.assertEqual(2, len(os.listdir(tmp_dir))) def test_both_fuzzers_kept_fuzzer(self): """Tests that if both fuzzers are affected then both fuzzers are kept.""" - with tempfile.TemporaryDirectory() as tmp_dir, unittest.mock.patch.object( + with tempfile.TemporaryDirectory() as tmp_dir, mock.patch.object( cifuzz, 'get_latest_cov_report_info', return_value=1): shutil.copy(self.test_fuzzer_1, tmp_dir) shutil.copy(self.test_fuzzer_2, tmp_dir) - with unittest.mock.patch.object( + with mock.patch.object( cifuzz, 'get_files_covered_by_target', side_effect=[self.example_file_changed, self.example_file_changed]): diff --git a/infra/presubmit.py b/infra/presubmit.py index cbcbc0bc..f8cf0110 100755 --- a/infra/presubmit.py +++ b/infra/presubmit.py @@ -334,6 +334,9 @@ def run_tests(): for file in get_changed_files(): changed_dirs.add(os.path.dirname(file)) + # TODO(metzman): This approach for running tests is probably flawed since + # tests can fail even if their directory isn't changed. Figure out if it is + # needed (to save time) and remove it if it isn't. suite_list = [] for change_dir in changed_dirs: suite_list.append(unittest.TestLoader().discover(change_dir, -- cgit v1.2.3