diff --git a/infra/cifuzz/affected_fuzz_targets.py b/infra/cifuzz/affected_fuzz_targets.py index ca81752bf..b5f51ad63 100644 --- a/infra/cifuzz/affected_fuzz_targets.py +++ b/infra/cifuzz/affected_fuzz_targets.py @@ -11,7 +11,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Module for dealing with fuzzers affected by the change-under-test (CUT).""" +"""Module for dealing with fuzz targets affected by the change-under-test +(CUT).""" +import logging import os import sys @@ -21,11 +23,91 @@ sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import utils -def fix_git_repo_for_diff(repo_dir): - """Fixes git repos cloned by the "checkout" action so that diffing works on - them.""" - command = [ - 'git', 'symbolic-ref', 'refs/remotes/origin/HEAD', - 'refs/remotes/origin/master' - ] - return utils.execute(command, location=repo_dir) +def remove_unaffected_fuzz_targets(project_name, out_dir, files_changed, + repo_path): + """Removes all non affected fuzz targets in the out directory. + + Args: + project_name: The name of the relevant OSS-Fuzz project. + out_dir: The location of the fuzz target binaries. + files_changed: A list of files changed compared to HEAD. + repo_path: The location of the OSS-Fuzz repo in the docker image. + + This function will not delete fuzz targets unless it knows that the fuzz + targets are unaffected. For example, this means that fuzz targets which don't + have coverage data on will not be deleted. + """ + if not files_changed: + # Don't remove any fuzz targets if there is no difference from HEAD. + logging.info('No files changed.') + return + + logging.info('Files changed in PR: %s', files_changed) + + fuzz_target_paths = utils.get_fuzz_targets(out_dir) + if not fuzz_target_paths: + # Nothing to remove. + logging.error('No fuzz targets found in out dir.') + return + + coverage_getter = coverage.OSSFuzzCoveragGetter(project_name, + repo_path) + if coverage_getter.fuzzer_stats_url: + # Don't remove any fuzz targets unless we have data. + logging.error('Could not download latest coverage report.') + return + + affected_fuzz_targets = get_affected_fuzz_targets( + coverage_getter, fuzz_target_paths, files_changed) + + if not affected_fuzz_targets: + logging.info('No affected fuzz targets detected, keeping all as fallback.') + return + + logging.info('Using affected fuzz targets: %s.', affected_fuzz_targets) + unaffected_fuzz_targets = set(fuzz_target_paths) - affected_fuzz_targets + logging.info('Removing unaffected fuzz targets: %s.', unaffected_fuzz_targets) + + # Remove all the targets that are not affected. + for fuzz_target_path in unaffected_fuzz_targets: + try: + os.remove(fuzz_target_path) + except OSError as error: + logging.error('%s occurred while removing file %s', + error, fuzz_target_path) + + +def is_fuzz_target_affected(coverage_getter, fuzz_target_path, files_changed): + """Returns True if a fuzz target (|fuzz_target_path| is affected by + |files_changed|.""" + fuzz_target = os.path.basename(fuzz_target_path) + covered_files = coverage_getter.get_files_covered_by_target(fuzz_target) + if not covered_files: + # Assume a fuzz target is affected if we can't get its coverage from + # OSS-Fuzz. + logging.info('Could not get coverage for %s. Treating as affected.', + fuzz_target) + return True + + logging.info('Fuzz target %s is affected by: %s', + fuzz_target, covered_files) + for filename in files_changed: + if filename in covered_files: + logging.info('Fuzz target %s is affected by changed file: %s', + fuzz_target, filename) + return True + + logging.info('Fuzz target %s is not affected.', fuzz_target) + return False + + +def get_affected_fuzz_targets( + coverage_getter, fuzz_target_paths, files_changed): + """Returns a list of paths of affected targets.""" + affected_fuzz_targets = set() + for fuzz_target_path in fuzz_target_paths: + if is_fuzz_target_affected( + coverage_getter, fuzz_target_path, files_changed): + affected_fuzz_targets.add(fuzz_target_path) + + return affected_fuzz_targets diff --git a/infra/cifuzz/cifuzz.py b/infra/cifuzz/cifuzz.py index 49256c8eb..052aed650 100644 --- a/infra/cifuzz/cifuzz.py +++ b/infra/cifuzz/cifuzz.py @@ -24,8 +24,9 @@ import shutil import sys import time +import affected_fuzz_targets import fuzz_target -import coverage + # pylint: disable=wrong-import-position,import-error sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -77,6 +78,16 @@ _IMAGE_BUILD_TRIES = 3 _IMAGE_BUILD_BACKOFF = 2 +def fix_git_repo_for_diff(repo_dir): + """Fixes git repos cloned by the "checkout" action so that diffing works on + them.""" + command = [ + 'git', 'symbolic-ref', 'refs/remotes/origin/HEAD', + 'refs/remotes/origin/master' + ] + return utils.execute(command, location=repo_dir) + + def checkout_specified_commit(repo_manager_obj, pr_ref, commit_sha): """Checks out the specified commit or pull request using |repo_manager_obj|.""" @@ -211,19 +222,20 @@ class BaseBuilder: # 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_fuzzers + self.remove_unaffected_fuzz_targets ] for method in methods: if not method(): return False return True - def remove_unaffected_fuzzers(self): + def remove_unaffected_fuzz_targets(self): """Removes the fuzzers unaffected by the patch.""" fix_git_repo_for_diff(self.host_repo_path) changed_files = self.repo_manager.get_git_diff() - remove_unaffected_fuzzers(self.project_name, self.out_dir, changed_files, - self.image_repo_path) + affected_fuzz_targets.remove_unaffected_fuzzers( + self.project_name, self.out_dir, changed_files, + self.image_repo_path) return True @@ -516,78 +528,6 @@ def check_fuzzer_build(out_dir, return True -def remove_unaffected_fuzzers(project_name, out_dir, files_changed, - oss_fuzz_repo_path): - """Removes all non affected fuzzers in the out directory. - - Args: - project_name: The name of the relevant OSS-Fuzz project. - out_dir: The location of the fuzzer binaries. - files_changed: A list of files changed compared to HEAD. - oss_fuzz_repo_path: The location of the OSS-Fuzz repo in the docker image. - - This function will not delete fuzzers unless it knows that the fuzzer is - unaffected. For example, this means that fuzzers which don't have coverage - data on will not be deleted. - """ - if not files_changed: - # Don't remove any fuzzers if there is no difference from HEAD. - logging.info('No files changed compared to HEAD.') - return - - logging.info('Files changed in PR: %s', files_changed) - - fuzzer_paths = utils.get_fuzz_targets(out_dir) - if not fuzzer_paths: - # Nothing to remove. - logging.error('No fuzzers found in out dir.') - return - - coverage_getter = coverage.OSSFuzzCoveragGetter(project_name, - oss_fuzz_repo_path) - if not cov_report_info: - # Don't remove any fuzzers unless we have data. - logging.error('Could not download latest coverage report.') - return - - affected_fuzzers = get_affected_fuzzers( - coverage_getter, fuzzer_paths, files_changed) - - if not affected_fuzzers: - logging.info('No affected fuzzers detected, keeping all as fallback.') - return - - logging.info('Using affected fuzzers: %s.', affected_fuzzers) - unaffected_fuzzers = set(fuzzer_paths) - affected_fuzzers - logging.info('Removing unaffected fuzzers: %s.', unaffected_fuzzers) - # Remove all the fuzzers that are not affected. - for fuzzer_path in unaffected_fuzzers: - try: - os.remove(fuzzer_path) - except OSError as error: - logging.error('%s occurred while removing file %s', error, fuzzer_path) - - -def get_affected_fuzzers(coverage_getter, fuzzer_paths, files_changed): - """Returns a list of paths of affected fuzzers.""" - affected_fuzzers = set() - for fuzzer_path in fuzzer_paths: - fuzzer_name = os.path.basename(fuzzer_path) - covered_files = coverage_getter.get_files_covered_by_target(fuzzer_name) - - if not covered_files: - # Assume a fuzzer is affected if we can't get its coverage from OSS-Fuzz. - affected_fuzzers.add(fuzzer_path) - continue - - logging.info('Fuzzer %s is affected by: %s', fuzzer_name, covered_files) - for file in files_changed: - if file in covered_files: - affected_fuzzers.add(fuzzer_path) - - return affected_fuzzers - - def parse_fuzzer_output(fuzzer_output, out_dir): """Parses the fuzzer output from a fuzz target binary. diff --git a/infra/cifuzz/coverage.py b/infra/cifuzz/coverage.py index 07acf09a9..f9c056fd2 100644 --- a/infra/cifuzz/coverage.py +++ b/infra/cifuzz/coverage.py @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Module for determining coverage of fuzzers.""" +"""Module for determining coverage of fuzz targets.""" import logging import os import sys @@ -57,7 +57,7 @@ class OSSFuzzCoverageGetter: oss_fuzz_repo_path: The location of the repo in the docker image. Returns: - A list of files that the fuzzer covers or None. + A list of files that the fuzz targets covers or None. """ target_cov = self.get_target_coverage_report(target) if not target_cov: @@ -85,7 +85,8 @@ class OSSFuzzCoverageGetter: def _normalize_repo_path(repo_path): - # Make sure cases like /src/curl and /src/curl/ are both handled. + """Normalizes and returns |repo_path| to make sure cases like /src/curl and + /src/curl/ are both handled.""" repo_path = os.path.normpath(repo_path) if not repo_path.endswith('/'): repo_path += '/' @@ -129,6 +130,7 @@ def get_json_from_url(url): except urllib.error.HTTPError: logging.error('HTTP error with url %s.', url) return None + try: # read().decode() fixes compatibility issue with urllib response object. result_json = json.loads(response.read().decode())