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.

11 files changed, 141 insertions, 68 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."""
diff --git a/infra/repo_manager.py b/infra/repo_manager.py
index a0b97b3ef..07880d81a 100644
--- a/infra/repo_manager.py
+++ b/infra/repo_manager.py
@@ -135,7 +135,7 @@ class RepoManager:
              check_result=True)
     self.git(['remote', 'update'], check_result=True)
 
-  def get_commit_list(self, newest_commit, oldest_commit=None):
+  def get_commit_list(self, newest_commit, oldest_commit=None, limit=None):
     """Gets the list of commits(inclusive) between the old and new commits.
 
     Args:
@@ -162,7 +162,11 @@ class RepoManager:
     else:
       commit_range = newest_commit
 
-    out, _, err_code = self.git(['rev-list', commit_range])
+    limit_args = []
+    if limit:
+      limit_args.append(f'--max-count={limit}')
+
+    out, _, err_code = self.git(['rev-list', commit_range] + limit_args)
     commits = out.split('\n')
     commits = [commit for commit in commits if commit]
     if err_code or not commits: