From 21b47a7a22d3e65927b853ed0858dfea39d35103 Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Thu, 4 Feb 2021 07:15:51 -0800 Subject: [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. --- infra/cifuzz/build_fuzzers.py | 1 - infra/cifuzz/clusterfuzz_deployment.py | 2 +- infra/cifuzz/fuzz_target.py | 18 +++++++++--------- infra/cifuzz/run_fuzzers.py | 33 ++++++++++++++++++++------------- infra/cifuzz/run_fuzzers_entrypoint.py | 7 +++---- infra/cifuzz/run_fuzzers_test.py | 26 ++++++++++++-------------- 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__': -- cgit v1.2.3