Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-107652: Set up CIFuzz to run fuzz targets continuously #107653

Merged
merged 8 commits into from Oct 9, 2023

Conversation

illia-v
Copy link
Contributor

@illia-v illia-v commented Aug 5, 2023

This sets up CIFuzz to run fuzz targets with three types of sanitizers for every pull request to the main branch and every commit in the main branch.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@illia-v illia-v requested a review from hugovk August 5, 2023 11:50
@hugovk
Copy link
Member

hugovk commented Aug 5, 2023

I'm mindful of CI availability and note this adds three slow, ~23-28 minute jobs:

Screenshot_20230805_144434_Chrome

Is there any caching that can be used to speed them up?

Should we restrict these jobs to only trigger when C files change, and not Python/other support files?

And, if we've already run this on PRs, do we also need to run on main after merge?

@AA-Turner
Copy link
Member

And, if we've already run this on PRs, do we also need to run on main after merge?

I'd second this, especially if given OSS Fuzz's runs (per the comment at the top of the new job).

A

@illia-v
Copy link
Contributor Author

illia-v commented Aug 6, 2023

I'm mindful of CI availability and note this adds three slow, ~23-28 minute jobs:

Is there any caching that can be used to speed them up?

I agree, but it seems that CIFuzz does not support any settings for caching and Python compilation happens in its Docker containers.

There is a fuzz-seconds: 600 setting, I used the default (and the recommended minimum) 10-minute value because it makes the tests run for roughly the same amount of time as "Windows (x86)".

Should we restrict these jobs to only trigger when C files change, and not Python/other support files?

This sounds like a good idea!

And, if we've already run this on PRs, do we also need to run on main after merge?

Running on PR updates should be enough.

@vstinner
Copy link
Member

I don't understand well which Python is tested. Does the job builds Python and run the fuzzer on the freshly built Python?


CIFuzz (address) pass but it see a log with an error:

=================================================================
==83==ERROR: AddressSanitizer: ABRT on unknown address 0x000000000053 (pc 0x7f4f5cf8600b bp 0x7fffccb9f100 sp 0x7fffccb9ee80 T0)
SCARINESS: 10 (signal)
    #0 0x7f4f5cf8600b in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #1 0x7f4f5cf65858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22858) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #2 0x586744 in LLVMFuzzerTestOneInput /src/cpython3/Modules/_xxtestfuzz/fuzzer.c
    #3 0x4578d3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #4 0x4570ba in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
    #5 0x458f24 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:826:7
    #6 0x459159 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:857:3
    #7 0x4487bf in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6
    #8 0x471e12 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #9 0x7f4f5cf67082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #10 0x4391fd in _start (build-out/fuzz_ast_literal_eval+0x4391fd)

DEDUP_TOKEN: raise--abort--LLVMFuzzerTestOneInput
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) in raise
==83==ABORTING

@illia-v
Copy link
Contributor Author

illia-v commented Aug 25, 2023

@vstinner please check "How it works".

It builds CPython from the source at a particular pull request, and it does not report crashes that occurred on older OSS-Fuzz builds. You can find that particular error in the list of known issues here.

@AA-Turner
Copy link
Member

Could CIFuzz be run as a buildbot instead of on CI? There's been a concerted effort to reduce CI times as of late, and I do worry that this PR adds a substantial amount of work for little apparent benefit, given that OSS Fuzz runs anyway on actual commits.

If we could do a "test with fuzz buildbots" label, that might be a compromise?

A

@vstinner
Copy link
Member

There's been a concerted effort to reduce CI times as of late, and I do worry that this PR adds a substantial amount of work for little apparent benefit, given that OSS Fuzz runs anyway on actual commits.

If the CI job is not mandatory and doesn't prevent to merge a PR, I think that it's acceptable to use GHA resources.

@illia-v
Copy link
Contributor Author

illia-v commented Aug 25, 2023

If we could do a "test with fuzz buildbots" label, that might be a compromise?

I am not sure how often the label will be set, it is pretty likely that this approach will not solve the problem of OSS-Fuzz builds being left broken from time to time

@illia-v
Copy link
Contributor Author

illia-v commented Oct 4, 2023

@ezio-melotti @hugovk is there anything I can do here to make this PR ready for merging?

@hugovk
Copy link
Member

hugovk commented Oct 6, 2023

@illia-v It's the core dev sprint next week, I'll bring it up for discussion then. I think fuzzing can be extremely valuable, and we need to balance how long it runs so we're not waiting too long for PRs to complete.

@AA-Turner
Copy link
Member

It seems @15r10nk found several issues that OSS-Fuzz missed, I assume using a different approach. If we're to adopt fuzzing, perhaps we should try his mechanism that seems to find more (release-blocking!) issues?

A

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Let's give this a shot! It's a bit slow, but it's about the same as the Windows jobs. We're trying to make the Windows ones faster, and if the duration fuzzing jobs end up being a problem, we can address it then.

.github/workflows/build.yml Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@hugovk hugovk merged commit ea7b53f into python:main Oct 9, 2023
29 checks passed
@vstinner
Copy link
Member

Is there a doc somewhere explaining how to handle a CIFuzz failure? Or this CI never fails? :-)

@vstinner
Copy link
Member

I just would like to know who I should contact if it goes wrong. @hugovk if I understood correctly :-)

@hugovk
Copy link
Member

hugovk commented Oct 11, 2023

If/when it fails, let's open an issue and figure it out :)

@gpshead
Copy link
Member

gpshead commented Mar 16, 2024

We've been seeing CIFuzz failures on otherwise good PRs not related to the PR change at hand of late with the build failing:

./Programs/_freeze_module importlib._bootstrap_external ./Lib/importlib/_bootstrap_external.py Python/frozen_modules/importlib._bootstrap_external.h
Segmentation fault (core dumped)
make: *** [Makefile:1660: Python/frozen_modules/importlib._bootstrap_external.h] Error 139

these are understandably confusing people.

@vstinner
Copy link
Member

We've been seeing CIFuzz failures on otherwise good PRs not related to the PR change at hand of late with the build failing:
./Programs/_freeze_module importlib._bootstrap_external ./Lib/importlib/_bootstrap_external.py Python/frozen_modules/importlib._bootstrap_external.h
Segmentation fault (core dumped)

@gpshead created issue #116886.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants