From de0cec2514e1be93fb453a812219de64063eeb7b Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Fri, 18 Jun 2021 10:26:36 -0700 Subject: [PATCH] [CIFuzz] Improve fuzz_target.py (#5929) * [CIFuzz] Clean up fuzz_target.py 1. Use CORPUS_DIR env var to set corpus so that corpus can be saved. 2. Clean up is_crash_novel. * fix * consistency * improve logging messages, remove over-cautious check * fix tests * fix tests * Make sure corpus path is mapped --- infra/cifuzz/docker.py | 6 ++++ infra/cifuzz/fuzz_target.py | 54 +++++++++++++++++++------------- infra/cifuzz/fuzz_target_test.py | 11 +++---- 3 files changed, 43 insertions(+), 28 deletions(-) diff --git a/infra/cifuzz/docker.py b/infra/cifuzz/docker.py index eb993e28d..1179d7832 100644 --- a/infra/cifuzz/docker.py +++ b/infra/cifuzz/docker.py @@ -36,3 +36,9 @@ def delete_images(images): command = ['docker', 'rmi', '-f'] + images utils.execute(command) utils.execute(['docker', 'builder', 'prune', '-f']) + + +def get_args_mapping_host_path_to_container(path): + """Get arguments to docker run that will map |path| a path on the host to the + same location in the container.""" + return ['-v', f'{path}:{path}'] diff --git a/infra/cifuzz/fuzz_target.py b/infra/cifuzz/fuzz_target.py index ecb8d970f..f0a764b20 100644 --- a/infra/cifuzz/fuzz_target.py +++ b/infra/cifuzz/fuzz_target.py @@ -42,10 +42,9 @@ REPRODUCE_ATTEMPTS = 10 BUFFER_TIME = 10 # Log message if we can't check if crash reproduces on an recent build. -COULD_NOT_TEST_ON_RECENT_MESSAGE = ( - 'Crash is reproducible. Could not run recent build of ' - 'target to determine if this code change (pr/commit) introduced crash. ' - 'Assuming this code change introduced crash.') +COULD_NOT_TEST_ON_CLUSTERFUZZ_MESSAGE = ( + 'Could not run previous build of target to determine if this code change ' + '(pr/commit) introduced crash. Assuming crash was newly introduced.') FuzzResult = collections.namedtuple('FuzzResult', ['testcase', 'stacktrace', 'corpus_path']) @@ -102,6 +101,16 @@ class FuzzTarget: else: command += ['-v', f'{self.out_dir}:/out'] + # TODO(metzman): Stop using /out for artifacts and corpus. Use another + # directory. + # If corpus can be downloaded use it for fuzzing. + self.latest_corpus_path = self.clusterfuzz_deployment.download_corpus( + self.target_name, self.out_dir) + if self.latest_corpus_path: + command += docker.get_args_mapping_host_path_to_container( + self.latest_corpus_path) + command += ['-e', 'CORPUS_DIR=' + self.latest_corpus_path] + command += [ '-e', 'FUZZING_ENGINE=libfuzzer', '-e', 'SANITIZER=' + self.config.sanitizer, '-e', 'CIFUZZ=True', '-e', @@ -111,11 +120,6 @@ class FuzzTarget: options = LIBFUZZER_OPTIONS + ' -max_total_time=' + str(self.duration) run_fuzzer_command = f'run_fuzzer {self.target_name} {options}' - # If corpus can be downloaded use it for fuzzing. - self.latest_corpus_path = self.clusterfuzz_deployment.download_corpus( - 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) logging.info('Running command: %s', ' '.join(command)) @@ -246,48 +250,52 @@ class FuzzTarget: reproducible_on_code_change = self.is_reproducible( testcase, self.target_path) except ReproduceError as error: - logging.error('Could not run target when checking for reproducibility.' + logging.error('Could not check for crash reproducibility.' 'Please file an issue:' 'https://github.com/google/oss-fuzz/issues/new.') raise error if not reproducible_on_code_change: - logging.info('Failed to reproduce the crash using the obtained testcase.') + # TODO(metzman): Allow users to specify if unreproducible crashes should + # be reported. + logging.info('Crash is not reproducible.') return False + logging.info('Crash is reproducible.') return self.is_crash_novel(testcase) def is_crash_novel(self, testcase): """Returns whether or not the crash is new. A crash is considered new if it can't be reproduced on an older ClusterFuzz build of the target.""" + if not os.path.exists(testcase): + raise ReproduceError('Testcase %s not found.' % testcase) clusterfuzz_build_dir = self.clusterfuzz_deployment.download_latest_build( self.out_dir) if not clusterfuzz_build_dir: # Crash is reproducible on PR build and we can't test on a recent # ClusterFuzz/OSS-Fuzz build. - logging.info(COULD_NOT_TEST_ON_RECENT_MESSAGE) + logging.info(COULD_NOT_TEST_ON_CLUSTERFUZZ_MESSAGE) return True clusterfuzz_target_path = os.path.join(clusterfuzz_build_dir, self.target_name) + try: reproducible_on_clusterfuzz_build = self.is_reproducible( testcase, clusterfuzz_target_path) except ReproduceError: # This happens if the project has ClusterFuzz builds, but the fuzz target # is not in it (e.g. because the fuzz target is new). - logging.info(COULD_NOT_TEST_ON_RECENT_MESSAGE) + logging.info(COULD_NOT_TEST_ON_CLUSTERFUZZ_MESSAGE) return True - if not reproducible_on_clusterfuzz_build: - logging.info('The crash is reproducible. The crash doesn\'t reproduce ' - 'on old builds. This code change probably introduced the ' - 'crash.') - return True - - logging.info('The crash is reproducible on old builds ' - '(without the current code change).') - return False + if reproducible_on_clusterfuzz_build: + logging.info('The crash is reproducible on previous build. ' + 'Code change (pr/commit) did not introduce crash.') + return False + logging.info('The crash doesn\'t reproduce on previous build. ' + 'Code change (pr/commit) introduced crash.') + return True def get_testcase(self, error_bytes): """Gets the file from a fuzzer run stacktrace. @@ -298,6 +306,8 @@ class FuzzTarget: Returns: The path to the testcase or None if not found. """ + # TODO(metzman): Stop parsing and use libFuzzers' artifact_prefix option + # instead. match = re.search(rb'\bTest unit written to \.\/([^\s]+)', error_bytes) if match: return os.path.join(self.out_dir, match.group(1).decode('utf-8')) diff --git a/infra/cifuzz/fuzz_target_test.py b/infra/cifuzz/fuzz_target_test.py index 1ec3aed74..21e04c44a 100644 --- a/infra/cifuzz/fuzz_target_test.py +++ b/infra/cifuzz/fuzz_target_test.py @@ -190,9 +190,8 @@ class IsCrashReportableTest(fake_filesystem_unittest.TestCase): self.test_target.out_dir = tmp_dir self.assertTrue(self.test_target.is_crash_reportable(self.testcase_path)) mocked_info.assert_called_with( - 'The crash is reproducible. The crash doesn\'t reproduce ' - 'on old builds. This code change probably introduced the ' - 'crash.') + 'The crash doesn\'t reproduce on previous build. ' + 'Code change (pr/commit) introduced crash.') # yapf: disable @parameterized.parameterized.expand([ @@ -239,9 +238,9 @@ class IsCrashReportableTest(fake_filesystem_unittest.TestCase): mocked_is_reproducible.assert_any_call(self.testcase_path, self.oss_fuzz_target_path) mocked_info.assert_called_with( - 'Crash is reproducible. Could not run recent build of ' - 'target to determine if this code change (pr/commit) introduced crash. ' - 'Assuming this code change introduced crash.') + 'Could not run previous build of target to determine if this code ' + 'change (pr/commit) introduced crash. Assuming crash was newly ' + 'introduced.') if __name__ == '__main__':