[cifuzz][NFC] Handle TODOs (#5104)

Handle some TODOs
1. Get rid of multiple return values and replace with a more sensible
return value.
2. Eliminate some useless TODOs.

6 files changed, 45 insertions, 42 deletions

diff --git a/infra/cifuzz/build_fuzzers.py b/infra/cifuzz/build_fuzzers.py
index c8d06fff0..a4342a413 100644
--- a/infra/cifuzz/build_fuzzers.py
+++ b/infra/cifuzz/build_fuzzers.py
@@ -30,7 +30,6 @@ import utils
 DEFAULT_ENGINE = 'libfuzzer'
 DEFAULT_ARCHITECTURE = 'x86_64'
 
-# TODO(metzman): Turn default logging to WARNING when CIFuzz is stable.
 logging.basicConfig(
     format='%(asctime)s - %(name)s - %(levelname)s - %(message)s',
     level=logging.DEBUG)
diff --git a/infra/cifuzz/clusterfuzz_deployment.py b/infra/cifuzz/clusterfuzz_deployment.py
index 60b1c31a0..8c46e9d4e 100644
--- a/infra/cifuzz/clusterfuzz_deployment.py
+++ b/infra/cifuzz/clusterfuzz_deployment.py
@@ -158,7 +158,7 @@ def download_url(url, filename, num_attempts=3):
   """
   sleep_time = 1
 
-  # TODO(metzman): Use retry.wrap here.
+  # Don't use retry wrapper since we don't want this to raise any exceptions.
   for _ in range(num_attempts):
     try:
       urllib.request.urlretrieve(url, filename)
diff --git a/infra/cifuzz/fuzz_target.py b/infra/cifuzz/fuzz_target.py
index 3f2d7f8d7..7bccfa4e1 100644
--- a/infra/cifuzz/fuzz_target.py
+++ b/infra/cifuzz/fuzz_target.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """A module to handle running a fuzz target for a specified amount of time."""
+import collections
 import logging
 import os
 import re
@@ -23,7 +24,6 @@ import sys
 sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 import utils
 
-# TODO: Turn default logging to WARNING when CIFuzz is stable.
 logging.basicConfig(
     format='%(asctime)s - %(name)s - %(levelname)s - %(message)s',
     level=logging.DEBUG)
@@ -42,6 +42,8 @@ COULD_NOT_TEST_ON_RECENT_MESSAGE = (
     'target to determine if this code change (pr/commit) introduced crash. '
     'Assuming this code change introduced crash.')
 
+FuzzResult = collections.namedtuple('FuzzResult', ['testcase', 'stacktrace'])
+
 
 class ReproduceError(Exception):
   """Error for when we can't attempt to reproduce a crash."""
@@ -81,10 +83,8 @@ class FuzzTarget:
     """Starts the fuzz target run for the length of time specified by duration.
 
     Returns:
-      (testcase, stacktrace, time in seconds) on crash or
-      (None, None, time in seconds) on timeout or error.
+      FuzzResult namedtuple with stacktrace and testcase if applicable.
     """
-    # TODO(metzman): Change return value to a FuzzResult object.
     logging.info('Fuzzer %s, started.', self.target_name)
     docker_container = utils.get_container_name()
     command = ['docker', 'run', '--rm', '--privileged']
@@ -122,23 +122,23 @@ class FuzzTarget:
       _, stderr = process.communicate(timeout=self.duration + BUFFER_TIME)
     except subprocess.TimeoutExpired:
       logging.error('Fuzzer %s timed out, ending fuzzing.', self.target_name)
-      return None, None
+      return FuzzResult(None, None)
 
     # Libfuzzer timeout was reached.
     if not process.returncode:
       logging.info('Fuzzer %s finished with no crashes discovered.',
                    self.target_name)
-      return None, None
+      return FuzzResult(None, None)
 
     # Crash was discovered.
     logging.info('Fuzzer %s, ended before timeout.', self.target_name)
     testcase = self.get_testcase(stderr)
     if not testcase:
       logging.error(b'No testcase found in stacktrace: %s.', stderr)
-      return None, None
+      return FuzzResult(None, None)
     if self.is_crash_reportable(testcase):
-      return testcase, stderr
-    return None, None
+      return FuzzResult(testcase, stderr)
+    return FuzzResult(None, None)
 
   def is_reproducible(self, testcase, target_path):
     """Checks if the testcase reproduces.
diff --git a/infra/cifuzz/run_fuzzers.py b/infra/cifuzz/run_fuzzers.py
index f3e4e5284..2a2a89e5f 100644
--- a/infra/cifuzz/run_fuzzers.py
+++ b/infra/cifuzz/run_fuzzers.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Module for running fuzzers."""
+import enum
 import logging
 import os
 import shutil
@@ -28,6 +29,13 @@ sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 import utils
 
 
+class RunFuzzersResult(enum.Enum):
+  """Enum result from running fuzzers."""
+  ERROR = 0
+  BUG_FOUND = 1
+  NO_BUG_FOUND = 2
+
+
 class BaseFuzzTargetRunner:
   """Base class for fuzzer runners."""
 
@@ -120,28 +128,29 @@ class BaseFuzzTargetRunner:
 
       target = self.create_fuzz_target_obj(target_path, run_seconds)
       start_time = time.time()
-      testcase, stacktrace = self.run_fuzz_target(target)
+      result = self.run_fuzz_target(target)
 
       # It's OK if this goes negative since we take max when determining
       # run_seconds.
       fuzz_seconds -= time.time() - start_time
 
       fuzzers_left_to_run -= 1
-      if not testcase or not stacktrace:
+      if not result.testcase or not result.stacktrace:
         logging.info('Fuzzer %s finished running without crashes.',
                      target.target_name)
         continue
 
       # We found a bug in the fuzz target.
       utils.binary_print(b'Fuzzer: %s. Detected bug:\n%s' %
-                         (target.target_name.encode(), stacktrace))
+                         (target.target_name.encode(), result.stacktrace))
 
       # TODO(metzman): Do this with filestore.
-      testcase_artifact = self.get_fuzz_target_artifact(target, 'testcase')
-      shutil.move(testcase, testcase_artifact)
-      bug_summary_artifact = self.get_fuzz_target_artifact(
+      testcase_artifact_path = self.get_fuzz_target_artifact(target, 'testcase')
+      shutil.move(result.testcase, testcase_artifact_path)
+      bug_summary_artifact_path = self.get_fuzz_target_artifact(
           target, 'bug-summary.txt')
-      stack_parser.parse_fuzzer_output(stacktrace, bug_summary_artifact)
+      stack_parser.parse_fuzzer_output(result.stacktrace,
+                                       bug_summary_artifact_path)
 
       bug_found = True
       if self.quit_on_bug_found:
@@ -183,19 +192,17 @@ def run_fuzzers(config):  # pylint: disable=too-many-locals
     config: A RunFuzzTargetsConfig.
 
   Returns:
-    (True if no (internal) errors fuzzing, True if bug found fuzzing).
+    A RunFuzzersResult enum value indicating what happened during fuzzing.
   """
   fuzz_target_runner = get_fuzz_target_runner(config)
-  # TODO(metzman): Multiple return bools is confusing. Change to one enum
-  # return value.
   if not fuzz_target_runner.initialize():
     # We didn't fuzz at all because of internal (CIFuzz) errors. And we didn't
     # find any bugs.
-    return False, False
+    return RunFuzzersResult.ERROR
 
   if not fuzz_target_runner.run_fuzz_targets():
     # We fuzzed successfully, but didn't find any bugs (in the fuzz target).
-    return True, False
+    return RunFuzzersResult.NO_BUG_FOUND
 
   # We fuzzed successfully and found bug(s) in the fuzz targets.
-  return True, True
+  return RunFuzzersResult.BUG_FOUND
diff --git a/infra/cifuzz/run_fuzzers_entrypoint.py b/infra/cifuzz/run_fuzzers_entrypoint.py
index 48d745fd8..f810e38f8 100644
--- a/infra/cifuzz/run_fuzzers_entrypoint.py
+++ b/infra/cifuzz/run_fuzzers_entrypoint.py
@@ -21,7 +21,6 @@ import run_fuzzers
 # pylint: disable=c-extension-no-member
 # pylint gets confused because of the relative import of cifuzz.
 
-# TODO: Turn default logging to INFO when CIFuzz is stable.
 logging.basicConfig(
     format='%(asctime)s - %(name)s - %(levelname)s - %(message)s',
     level=logging.DEBUG)
@@ -64,12 +63,12 @@ def main():
     return returncode
 
   # Run the specified project's fuzzers from the build.
-  run_status, bug_found = run_fuzzers.run_fuzzers(config)
-  if not run_status:
+  result = run_fuzzers.run_fuzzers(config)
+  if result == run_fuzzers.RunFuzzersResult.ERROR:
     logging.error('Error occurred while running in workspace %s.',
                   config.workspace)
     return returncode
-  if bug_found:
+  if result == run_fuzzers.RunFuzzersResult.BUG_FOUND:
     logging.info('Bug found.')
     if not config.dry_run:
       # Return 2 when a bug was found by a fuzzer causing the CI to fail.
diff --git a/infra/cifuzz/run_fuzzers_test.py b/infra/cifuzz/run_fuzzers_test.py
index c41bbe37a..847ddf399 100644
--- a/infra/cifuzz/run_fuzzers_test.py
+++ b/infra/cifuzz/run_fuzzers_test.py
@@ -23,6 +23,7 @@ import parameterized
 from pyfakefs import fake_filesystem_unittest
 
 import config_utils
+import fuzz_target
 import run_fuzzers
 
 # pylint: disable=wrong-import-position
@@ -79,9 +80,8 @@ class RunFuzzerIntegrationTestMixin:  # pylint: disable=too-few-public-methods,i
                               workspace=fuzzer_dir_copy,
                               project_name='curl',
                               sanitizer=sanitizer)
-      run_success, bug_found = run_fuzzers.run_fuzzers(config)
-    self.assertTrue(run_success)
-    self.assertFalse(bug_found)
+      result = run_fuzzers.run_fuzzers(config)
+    self.assertEqual(result, run_fuzzers.RunFuzzersResult.NO_BUG_FOUND)
 
 
 class RunMemoryFuzzerIntegrationTest(RunFuzzerIntegrationTestMixin,
@@ -257,7 +257,8 @@ class CiFuzzTargetRunnerTest(fake_filesystem_unittest.TestCase):
     testcase = os.path.join(workspace, 'testcase')
     self.fs.create_file(testcase)
     stacktrace = b'stacktrace'
-    mocked_run_fuzz_target.return_value = (testcase, stacktrace)
+    mocked_run_fuzz_target.return_value = fuzz_target.FuzzResult(
+        testcase, stacktrace)
     magic_mock = mock.MagicMock()
     magic_mock.target_name = 'target1'
     mocked_create_fuzz_target_obj.return_value = magic_mock
@@ -304,7 +305,7 @@ class BatchFuzzTargetRunnerTest(fake_filesystem_unittest.TestCase):
         testcase = testcase2
       assert call_count != 2
       call_count += 1
-      return testcase, stacktrace
+      return fuzz_target.FuzzResult(testcase, stacktrace)
 
     mocked_run_fuzz_target.side_effect = mock_run_fuzz_target
     magic_mock = mock.MagicMock()
@@ -336,9 +337,8 @@ class RunAddressFuzzersIntegrationTest(RunFuzzerIntegrationTestMixin,
         config = _create_config(fuzz_seconds=FUZZ_SECONDS,
                                 workspace=workspace,
                                 project_name=EXAMPLE_PROJECT)
-        run_success, bug_found = run_fuzzers.run_fuzzers(config)
-        self.assertTrue(run_success)
-        self.assertTrue(bug_found)
+        result = run_fuzzers.run_fuzzers(config)
+        self.assertEqual(result, run_fuzzers.RunFuzzersResult.BUG_FOUND)
         build_dir = os.path.join(workspace, 'out', self.BUILD_DIR_NAME)
         self.assertNotEqual(0, len(os.listdir(build_dir)))
 
@@ -357,12 +357,11 @@ class RunAddressFuzzersIntegrationTest(RunFuzzerIntegrationTestMixin,
       config = _create_config(fuzz_seconds=FUZZ_SECONDS,
                               workspace=TEST_FILES_PATH,
                               project_name=EXAMPLE_PROJECT)
-      run_success, bug_found = run_fuzzers.run_fuzzers(config)
+      result = run_fuzzers.run_fuzzers(config)
+      self.assertEqual(result, run_fuzzers.RunFuzzersResult.NO_BUG_FOUND)
       build_dir = os.path.join(TEST_FILES_PATH, 'out', self.BUILD_DIR_NAME)
       self.assertTrue(os.path.exists(build_dir))
       self.assertNotEqual(0, len(os.listdir(build_dir)))
-      self.assertTrue(run_success)
-      self.assertFalse(bug_found)
 
   def test_invalid_build(self):
     """Tests run_fuzzers with an invalid ASAN build."""
@@ -372,9 +371,8 @@ class RunAddressFuzzersIntegrationTest(RunFuzzerIntegrationTestMixin,
       config = _create_config(fuzz_seconds=FUZZ_SECONDS,
                               workspace=tmp_dir,
                               project_name=EXAMPLE_PROJECT)
-      run_success, bug_found = run_fuzzers.run_fuzzers(config)
-    self.assertFalse(run_success)
-    self.assertFalse(bug_found)
+      result = run_fuzzers.run_fuzzers(config)
+    self.assertEqual(result, run_fuzzers.RunFuzzersResult.ERROR)
 
 
 if __name__ == '__main__':