search

h2o / picotls

TLS 1.3 implementation in C (master supports RFC8446 as well as draft-26, -27, -28)
538 stars 142 forks source link

ci: add CIFuzz Github action #439

Closed DavidKorczynski closed 1 year ago

DavidKorczynski commented 1 year ago

Add CIFuzz workflow action to have fuzzers build and run on each PR.

This is a service offered by OSS-Fuzz where picotls already runs. CIFuzz can help detect catch regressions and fuzzing build issues early, and has a variety of features (see the URL above). In the current PR the fuzzers gets build on a pull request and will run for 300 seconds.

Signed-off-by: David Korczynski david@adalogics.com

huitema commented 1 year ago

I am of two minds regarding this. Yes, fuzzing is good practice, a good way to find issues before they materialize. But fuzzing involves choosing random input messages, which means different rounds of fuzzing will likely produce different results. What could well happen is that a PR fixes an issue, but the fuzzing CI detects by chance an unrelated crash that was missed in previous runs. The PR is then blocked until this unrelated crash is investigated and fixed. Is there a way to set a specific random seed for the fuzzer?

Also, on the engineering side. My experience is that fuzzing results are best if the code being tested is compiled with sanitizers turned on (e.g., ASAN, UBSAN). The sanitizers will capture "near crashes", such as a fuzzing input resulting in reading unallocated memory. Consider changing the workflow to compile with ASAN + UBSAN before running the fuzzer.

DavidKorczynski commented 1 year ago

What could well happen is that a PR fixes an issue, but the fuzzing CI detects by chance an unrelated crash that was missed in previous runs. The PR is then blocked until this unrelated crash is investigated and fixed. Is there a way to set a specific random seed for the fuzzer?

This won't be reported as OSS-Fuzz is very likely to have caught this (OSS-Fuzz runs the fuzzer for many hours whereas the CI is 300 secs). Notice the following in the documentation link which address this concern (link):

If CIFuzz finds a crash, it reports the crash only if both of following are true:

The crash is reproducible (on the PR/commit build).
The crash does not occur on older OSS-Fuzz builds. (If the crash does occur on older builds, then it was not introduced by the PR/commit being tested.)

Also, on the engineering side. My experience is that fuzzing results are best if the code being tested is compiled with sanitizers turned on (e.g., ASAN, UBSAN). The sanitizers will capture "near crashes", such as a fuzzing input resulting in reading unallocated memory. Consider changing the workflow to compile with ASAN + UBSAN before running the fuzzer.

OSS-Fuzz already compiles the fuzzers with sanitizers, so this is in fact what happens since CIFuzz uses the OSS-Fuzz set up. See the following in the link which addresses this (link):

sanitizer: Determines a sanitizer to build and run fuzz targets with. The choices are 'address', 'memory' and 'undefined'. The default is 'address'
kazuho commented 1 year ago

Thank you for the PR, this is a improvement.

Let's merge, we can take back / adjust things later if deemed necessary.