diff options
author | Oliver Chang <oliverchang@users.noreply.github.com> | 2021-08-17 16:36:06 +1000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-08-17 06:36:06 +0000 |
commit | a4bc23909b873b60630123e3e42b7a7d1d5ee939 (patch) | |
tree | 57ed1681a4d9505977b4c625e28f72caab4f8d57 /infra/cifuzz | |
parent | 44addc5c71526c3ea0eed2b545d9271c35d62359 (diff) | |
download | oss-fuzz-a4bc23909b873b60630123e3e42b7a7d1d5ee939.tar.gz |
Don't upload builds in run_fuzzers. (#6151)
The current way adds a lot of ordering assumptions, and doesn't fit too
well with parallel batch fuzzing either. Add a "upload-build" boolean action
input that can be added to "build_fuzzers" to upload latest builds
instead.
Builds are now uploaded by commit hash, rather than a fixed "latest" name.
ClusterFuzzLite's download_latest_build will check the last 3 commits and download the
first available build by git hash.
Diffstat (limited to 'infra/cifuzz')
-rw-r--r-- | infra/cifuzz/build_fuzzers.py | 10 | ||||
-rw-r--r-- | infra/cifuzz/build_fuzzers_test.py | 34 | ||||
-rw-r--r-- | infra/cifuzz/clusterfuzz_deployment.py | 61 | ||||
-rw-r--r-- | infra/cifuzz/clusterfuzz_deployment_test.py | 34 | ||||
-rw-r--r-- | infra/cifuzz/config_utils.py | 1 | ||||
-rw-r--r-- | infra/cifuzz/continuous_integration.py | 32 | ||||
-rw-r--r-- | infra/cifuzz/external-actions/build_fuzzers/action.yml | 5 | ||||
-rw-r--r-- | infra/cifuzz/run_fuzzers.py | 14 | ||||
-rw-r--r-- | infra/cifuzz/run_fuzzers_test.py | 5 | ||||
-rw-r--r-- | infra/cifuzz/workspace_utils.py | 5 |
10 files changed, 135 insertions, 66 deletions
diff --git a/infra/cifuzz/build_fuzzers.py b/infra/cifuzz/build_fuzzers.py index 978b50fb6..94478fdc2 100644 --- a/infra/cifuzz/build_fuzzers.py +++ b/infra/cifuzz/build_fuzzers.py @@ -111,6 +111,14 @@ class Builder: # pylint: disable=too-many-instance-attributes self.handle_msan_postbuild(docker_container) return True + def upload_build(self): + """Upload build.""" + if self.config.upload_build: + self.clusterfuzz_deployment.upload_build( + self.repo_manager.get_current_commit()) + + return True + def handle_msan_postbuild(self, container): """Post-build step for MSAN builds. Patches the build to use MSAN libraries.""" @@ -133,7 +141,7 @@ class Builder: # pylint: disable=too-many-instance-attributes and then removes the unaffectted fuzzers. Returns True on success.""" methods = [ self.build_image_and_checkout_src, self.build_fuzzers, - self.remove_unaffected_fuzz_targets + self.upload_build, self.remove_unaffected_fuzz_targets ] for method in methods: if not method(): diff --git a/infra/cifuzz/build_fuzzers_test.py b/infra/cifuzz/build_fuzzers_test.py index 52e17aa89..19ac22c4a 100644 --- a/infra/cifuzz/build_fuzzers_test.py +++ b/infra/cifuzz/build_fuzzers_test.py @@ -53,7 +53,7 @@ EXAMPLE_NOCRASH_FUZZER = 'example_nocrash_fuzzer' # A fuzzer to be built in build_fuzzers integration tests. EXAMPLE_BUILD_FUZZER = 'do_stuff_fuzzer' -# pylint: disable=no-self-use,protected-access,too-few-public-methods +# pylint: disable=no-self-use,protected-access,too-few-public-methods,unused-argument class BuildFuzzersTest(unittest.TestCase): @@ -91,16 +91,15 @@ class BuildFuzzersTest(unittest.TestCase): class InternalGithubBuildTest(unittest.TestCase): """Tests for building OSS-Fuzz projects on GitHub actions.""" - PROJECT_NAME = 'myproject' PROJECT_REPO_NAME = 'myproject' SANITIZER = 'address' COMMIT_SHA = 'fake' PR_REF = 'fake' - def _create_builder(self, tmp_dir): + def _create_builder(self, tmp_dir, oss_fuzz_project_name='myproject'): """Creates an InternalGithubBuilder and returns it.""" config = test_helpers.create_build_config( - oss_fuzz_project_name=self.PROJECT_NAME, + oss_fuzz_project_name=oss_fuzz_project_name, project_repo_name=self.PROJECT_REPO_NAME, workspace=tmp_dir, sanitizer=self.SANITIZER, @@ -108,7 +107,9 @@ class InternalGithubBuildTest(unittest.TestCase): pr_ref=self.PR_REF, is_github=True) ci_system = continuous_integration.get_ci(config) - return build_fuzzers.Builder(config, ci_system) + builder = build_fuzzers.Builder(config, ci_system) + builder.repo_manager = repo_manager.RepoManager('/fake') + return builder @mock.patch('repo_manager._clone', side_effect=None) @mock.patch('continuous_integration.checkout_specified_commit', @@ -128,6 +129,29 @@ class InternalGithubBuildTest(unittest.TestCase): self.assertEqual(os.path.basename(builder.host_repo_path), os.path.basename(image_repo_path)) + @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_build', + return_value=True) + def test_upload_build_disabled(self, mock_upload_build): + """Test upload build (disabled).""" + with tempfile.TemporaryDirectory() as tmp_dir: + builder = self._create_builder(tmp_dir) + builder.upload_build() + + mock_upload_build.assert_not_called() + + @mock.patch('repo_manager.RepoManager.get_current_commit', + return_value='commit') + @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_build', + return_value=True) + def test_upload_build(self, mock_upload_build, mock_get_current_commit): + """Test upload build.""" + with tempfile.TemporaryDirectory() as tmp_dir: + builder = self._create_builder(tmp_dir, oss_fuzz_project_name='') + builder.config.upload_build = True + builder.upload_build() + + mock_upload_build.assert_called_with('commit') + @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'), 'INTEGRATION_TESTS=1 not set') diff --git a/infra/cifuzz/clusterfuzz_deployment.py b/infra/cifuzz/clusterfuzz_deployment.py index a2be1ff9f..169c83fb3 100644 --- a/infra/cifuzz/clusterfuzz_deployment.py +++ b/infra/cifuzz/clusterfuzz_deployment.py @@ -19,10 +19,12 @@ import urllib.error import urllib.request import config_utils +import continuous_integration import filestore import filestore_utils import http_utils import get_coverage +import repo_manager # pylint: disable=wrong-import-position,import-error sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -35,6 +37,7 @@ class BaseClusterFuzzDeployment: def __init__(self, config, workspace): self.config = config self.workspace = workspace + self.ci_system = continuous_integration.get_ci(config) def download_latest_build(self): """Downloads the latest build from ClusterFuzz. @@ -44,11 +47,8 @@ class BaseClusterFuzzDeployment: """ raise NotImplementedError('Child class must implement method.') - def upload_latest_build(self): - """Uploads the latest build to the filestore. - Returns: - True on success. - """ + def upload_build(self, commit): + """Uploads the build with the given commit sha to the filestore.""" raise NotImplementedError('Child class must implement method.') def download_corpus(self, target_name, corpus_dir): @@ -85,6 +85,7 @@ class ClusterFuzzLite(BaseClusterFuzzDeployment): """Class representing a deployment of ClusterFuzzLite.""" COVERAGE_NAME = 'latest' + LATEST_BUILD_WINDOW = 3 def __init__(self, config, workspace): super().__init__(config, workspace) @@ -99,17 +100,31 @@ class ClusterFuzzLite(BaseClusterFuzzDeployment): # called if multiple bugs are found. return self.workspace.clusterfuzz_build - _make_empty_dir_if_nonexistent(self.workspace.clusterfuzz_build) - build_name = self._get_build_name() + repo_dir = self.ci_system.repo_dir() + if not repo_dir: + raise RuntimeError('Repo checkout does not exist.') - try: - logging.info('Downloading latest build.') - if self.filestore.download_build(build_name, - self.workspace.clusterfuzz_build): - logging.info('Done downloading latest build.') - return self.workspace.clusterfuzz_build - except Exception as err: # pylint: disable=broad-except - logging.error('Could not download latest build because of: %s', err) + _make_empty_dir_if_nonexistent(self.workspace.clusterfuzz_build) + repo = repo_manager.RepoManager(repo_dir) + + # Builds are stored by commit, so try the latest |LATEST_BUILD_WINDOW| + # commits before the current. + # TODO(ochang): If API usage becomes an issue, this can be optimized by the + # filestore accepting a list of filenames to try. + for old_commit in repo.get_commit_list('HEAD^', + limit=self.LATEST_BUILD_WINDOW): + logging.info('Trying to downloading previous build %s.', old_commit) + build_name = self._get_build_name(old_commit) + try: + if self.filestore.download_build(build_name, + self.workspace.clusterfuzz_build): + logging.info('Done downloading previus build.') + return self.workspace.clusterfuzz_build + + logging.info('Build for %s does not exist.', old_commit) + except Exception as err: # pylint: disable=broad-except + logging.error('Could not download build for %s because of: %s', + old_commit, err) return None @@ -126,8 +141,8 @@ class ClusterFuzzLite(BaseClusterFuzzDeployment): target_name, str(err)) return corpus_dir - def _get_build_name(self): - return self.config.sanitizer + '-latest' + def _get_build_name(self, name): + return f'{self.config.sanitizer}-{name}' def _get_corpus_name(self, target_name): # pylint: disable=no-self-use """Returns the name of the corpus artifact.""" @@ -148,10 +163,10 @@ class ClusterFuzzLite(BaseClusterFuzzDeployment): logging.error('Failed to upload corpus for target: %s. Error: %s.', target_name, error) - def upload_latest_build(self): + def upload_build(self, commit): """Upload the build produced by CIFuzz as the latest build.""" logging.info('Uploading latest build in %s.', self.workspace.out) - build_name = self._get_build_name() + build_name = self._get_build_name(commit) try: result = self.filestore.upload_build(build_name, self.workspace.out) logging.info('Done uploading latest build.') @@ -253,8 +268,8 @@ class OSSFuzz(BaseClusterFuzzDeployment): return None - def upload_latest_build(self): # pylint: disable=no-self-use - """Noop Implementation of upload_latest_build.""" + def upload_build(self, commit): # pylint: disable=no-self-use + """Noop Implementation of upload_build.""" logging.info('Not uploading latest build because on OSS-Fuzz.') def upload_corpus(self, target_name, corpus_dir): # pylint: disable=no-self-use,unused-argument @@ -303,8 +318,8 @@ class NoClusterFuzzDeployment(BaseClusterFuzzDeployment): """ClusterFuzzDeployment implementation used when there is no deployment of ClusterFuzz to use.""" - def upload_latest_build(self): # pylint: disable=no-self-use - """Noop Implementation of upload_latest_build.""" + def upload_build(self, commit): # pylint: disable=no-self-use + """Noop Implementation of upload_build.""" logging.info('Not uploading latest build because no ClusterFuzz ' 'deployment.') diff --git a/infra/cifuzz/clusterfuzz_deployment_test.py b/infra/cifuzz/clusterfuzz_deployment_test.py index 29a864d5b..247678548 100644 --- a/infra/cifuzz/clusterfuzz_deployment_test.py +++ b/infra/cifuzz/clusterfuzz_deployment_test.py @@ -35,6 +35,8 @@ EXAMPLE_FUZZER = 'example_crash_fuzzer' WORKSPACE = '/workspace' EXPECTED_LATEST_BUILD_PATH = os.path.join(WORKSPACE, 'cifuzz-prev-build') +# pylint: disable=unused-argument + def _create_config(**kwargs): """Creates a config object and then sets every attribute that is a key in @@ -92,7 +94,7 @@ class OSSFuzzTest(fake_filesystem_unittest.TestCase): self.assertTrue('address' in latest_build_name) @parameterized.parameterized.expand([ - ('upload_latest_build', tuple(), + ('upload_build', ('commit',), 'Not uploading latest build because on OSS-Fuzz.'), ('upload_corpus', ('target', 'corpus-dir'), 'Not uploading corpus because on OSS-Fuzz.'), @@ -152,28 +154,38 @@ class ClusterFuzzLiteTest(fake_filesystem_unittest.TestCase): self.assertEqual(os.listdir(self.corpus_dir), []) @mock.patch('filestore.github_actions.GithubActionsFilestore.download_build', - return_value=True) - def test_download_latest_build(self, mock_download_build): + side_effect=[False, True]) + @mock.patch('repo_manager.RepoManager.get_commit_list', + return_value=['commit1', 'commit2']) + @mock.patch('continuous_integration.BaseCi.repo_dir', + return_value='/path/to/repo') + def test_download_latest_build(self, mock_repo_dir, mock_get_commit_list, + mock_download_build): """Tests that downloading the latest build works as intended under normal circumstances.""" self.assertEqual(self.deployment.download_latest_build(), EXPECTED_LATEST_BUILD_PATH) - expected_artifact_name = 'address-latest' + expected_artifact_name = 'address-commit2' mock_download_build.assert_called_with(expected_artifact_name, EXPECTED_LATEST_BUILD_PATH) @mock.patch('filestore.github_actions.GithubActionsFilestore.download_build', side_effect=Exception) - def test_download_latest_build_fail(self, _): + @mock.patch('repo_manager.RepoManager.get_commit_list', + return_value=['commit1', 'commit2']) + @mock.patch('continuous_integration.BaseCi.repo_dir', + return_value='/path/to/repo') + def test_download_latest_build_fail(self, mock_repo_dir, mock_get_commit_list, + _): """Tests that download_latest_build returns None when it fails to download a build.""" self.assertIsNone(self.deployment.download_latest_build()) - @mock.patch('filestore.github_actions.GithubActionsFilestore.' 'upload_build') - def test_upload_latest_build(self, mock_upload_build): - """Tests that upload_latest_build works as intended.""" - self.deployment.upload_latest_build() - mock_upload_build.assert_called_with('address-latest', + @mock.patch('filestore.github_actions.GithubActionsFilestore.upload_build') + def test_upload_build(self, mock_upload_build): + """Tests that upload_build works as intended.""" + self.deployment.upload_build('commit') + mock_upload_build.assert_called_with('address-commit', '/workspace/build-out') @@ -199,7 +211,7 @@ class NoClusterFuzzDeploymentTest(fake_filesystem_unittest.TestCase): self.assertTrue(os.path.exists(self.corpus_dir)) @parameterized.parameterized.expand([ - ('upload_latest_build', tuple(), + ('upload_build', ('commit',), 'Not uploading latest build because no ClusterFuzz deployment.'), ('upload_corpus', ('target', 'corpus-dir'), 'Not uploading corpus because no ClusterFuzz deployment.'), diff --git a/infra/cifuzz/config_utils.py b/infra/cifuzz/config_utils.py index b8029bf41..561837770 100644 --- a/infra/cifuzz/config_utils.py +++ b/infra/cifuzz/config_utils.py @@ -209,6 +209,7 @@ class BaseConfig: self.git_store_branch = os.environ.get('GIT_STORE_BRANCH') self.git_store_branch_coverage = os.environ.get('GIT_STORE_BRANCH_COVERAGE', self.git_store_branch) + self.upload_build = environment.get_bool('UPLOAD_BUILD', False) # TODO(metzman): Fix tests to create valid configurations and get rid of # CIFUZZ_TEST here and in presubmit.py. diff --git a/infra/cifuzz/continuous_integration.py b/infra/cifuzz/continuous_integration.py index 33a57e52f..47c4a7cbf 100644 --- a/infra/cifuzz/continuous_integration.py +++ b/infra/cifuzz/continuous_integration.py @@ -26,6 +26,7 @@ import helper import repo_manager import retry import utils +import workspace_utils # pylint: disable=too-few-public-methods @@ -51,6 +52,24 @@ class BaseCi: def __init__(self, config): self.config = config + self.workspace = workspace_utils.Workspace(config) + + def repo_dir(self): + """Returns the source repo path, if it has been checked out. None is + returned otherwise.""" + if not os.path.exists(self.workspace.repo_storage): + return None + + # Note: this assumes there is only one repo checked out here. + listing = os.listdir(self.workspace.repo_storage) + if len(listing) != 1: + raise RuntimeError('Invalid repo storage.') + + repo_path = os.path.join(self.workspace.repo_storage, listing[0]) + if not os.path.isdir(repo_path): + raise RuntimeError('Repo is not a directory.') + + return repo_path def prepare_for_fuzzer_build(self): """Builds the fuzzer builder image and gets the source code we need to @@ -168,16 +187,14 @@ class InternalGithub(GithubCiMixin, BaseCi): image_repo_path=None, repo_manager=None) - git_workspace = os.path.join(self.config.workspace, 'storage') - os.makedirs(git_workspace, exist_ok=True) + os.makedirs(self.workspace.repo_storage, exist_ok=True) # Use the same name used in the docker image so we can overwrite it. image_repo_name = os.path.basename(image_repo_path) # Checkout project's repo in the shared volume. - manager = repo_manager.clone_repo_and_get_manager(inferred_url, - git_workspace, - repo_name=image_repo_name) + manager = repo_manager.clone_repo_and_get_manager( + inferred_url, self.workspace.repo_storage, repo_name=image_repo_name) checkout_specified_commit(manager, self.config.pr_ref, self.config.commit_sha) @@ -278,14 +295,13 @@ class ExternalGithub(GithubCiMixin, BaseCi): projects are expected to bring their own source code to CIFuzz. Returns True on success.""" logging.info('Building external project.') - git_workspace = os.path.join(self.config.workspace, 'storage') - os.makedirs(git_workspace, exist_ok=True) + os.makedirs(self.workspace.repo_storage, exist_ok=True) # Checkout before building, so we don't need to rely on copying the source # into the image. # TODO(metzman): Figure out if we want second copy at all. manager = repo_manager.clone_repo_and_get_manager( self.config.git_url, - git_workspace, + self.workspace.repo_storage, repo_name=self.config.project_repo_name) checkout_specified_commit(manager, self.config.pr_ref, self.config.commit_sha) diff --git a/infra/cifuzz/external-actions/build_fuzzers/action.yml b/infra/cifuzz/external-actions/build_fuzzers/action.yml index d8dec002b..f45d02e20 100644 --- a/infra/cifuzz/external-actions/build_fuzzers/action.yml +++ b/infra/cifuzz/external-actions/build_fuzzers/action.yml @@ -35,6 +35,10 @@ inputs: description: | The branch of the git repo to use for storing coverage reports. required: false + upload-build: + description: | + If set, will upload the build. + default: false github-token: description: | Token for GitHub API. WARNING: THIS SHOULD NOT BE USED IN PRODUCTION YET @@ -56,3 +60,4 @@ runs: GITHUB_TOKEN: ${{ inputs.github-token }} LOW_DISK_SPACE: 'True' BAD_BUILD_CHECK: ${{ inputs.bad-build-check }} + UPLOAD_BUILD: ${{ inputs.upload-build }} diff --git a/infra/cifuzz/run_fuzzers.py b/infra/cifuzz/run_fuzzers.py index 79cf432cb..347615772 100644 --- a/infra/cifuzz/run_fuzzers.py +++ b/infra/cifuzz/run_fuzzers.py @@ -258,21 +258,7 @@ class BatchFuzzTargetRunner(BaseFuzzTargetRunner): def run_fuzz_targets(self): result = super().run_fuzz_targets() - self.clusterfuzz_deployment.upload_crashes() - - # We want to upload the build to the filestore after we do batch fuzzing. - # There are some is a problem with this. We don't want to upload the build - # before fuzzing, because if we download the latest build, we will consider - # the build we just uploaded to be the latest even though it shouldn't be - # (we really intend to download the build before the curent one. - # TODO(metzman): We should really be uploading latest build in build_fuzzers - # before we remove unaffected fuzzers. Otherwise, we can lose fuzzers. This - # is probably more of a theoretical concern since in batch fuzzing, there is - # no code change and thus no fuzzers that are removed, but it's inelegant to - # put this here. - - self.clusterfuzz_deployment.upload_latest_build() return result diff --git a/infra/cifuzz/run_fuzzers_test.py b/infra/cifuzz/run_fuzzers_test.py index 5e10046b9..21b10dcc5 100644 --- a/infra/cifuzz/run_fuzzers_test.py +++ b/infra/cifuzz/run_fuzzers_test.py @@ -288,7 +288,7 @@ class BatchFuzzTargetRunnerTest(fake_filesystem_unittest.TestCase): is_github=True) @mock.patch('utils.get_fuzz_targets', return_value=['target1', 'target2']) - @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_latest_build', + @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_build', return_value=True) @mock.patch('run_fuzzers.BatchFuzzTargetRunner.run_fuzz_target') @mock.patch('run_fuzzers.BatchFuzzTargetRunner.create_fuzz_target_obj') @@ -321,10 +321,8 @@ class BatchFuzzTargetRunnerTest(fake_filesystem_unittest.TestCase): @mock.patch('run_fuzzers.BaseFuzzTargetRunner.run_fuzz_targets', return_value=False) - @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_latest_build') @mock.patch('clusterfuzz_deployment.ClusterFuzzLite.upload_crashes') def test_run_fuzz_targets_upload_crashes_and_builds(self, mock_upload_crashes, - mock_upload_latest_build, _): """Tests that run_fuzz_targets uploads crashes and builds correctly.""" runner = run_fuzzers.BatchFuzzTargetRunner(self.config) @@ -333,7 +331,6 @@ class BatchFuzzTargetRunnerTest(fake_filesystem_unittest.TestCase): self.assertFalse(runner.run_fuzz_targets()) self.assertEqual(mock_upload_crashes.call_count, 1) - self.assertEqual(mock_upload_latest_build.call_count, 1) @unittest.skipIf(not os.getenv('INTEGRATION_TESTS'), diff --git a/infra/cifuzz/workspace_utils.py b/infra/cifuzz/workspace_utils.py index d3b9596d1..ed296bc2b 100644 --- a/infra/cifuzz/workspace_utils.py +++ b/infra/cifuzz/workspace_utils.py @@ -27,6 +27,11 @@ class Workspace: os.makedirs(directory, exist_ok=True) @property + def repo_storage(self): + """The parent directory for repo storage.""" + return os.path.join(self.workspace, 'storage') + + @property def out(self): """The out directory used for storing the fuzzer build built by build_fuzzers.""" |