Add CIFuzz integration
Add CIFuzz workflow action to have fuzzers build and run on each PR. This is a service offered by OSS-Fuzz, on which PcapPlusPlus already runs.
Thanks @DavidKorczynski for your contribution! I'm trying to understand how this works (I also read the CIFuzz documentation). If the Fuzz runs on every PR - it may find issues that are not related to this PR, right? How can we guarantee that only issues related to the PR are found?
Yes, it may find issues not related to the PR. In short, it simply works by running the fuzzers on the PR. I think CIFuzz may do something to prioritise fuzzers that have coverage related to the files being changed in the PR, however, am not 100% sure. With that said, this is a very difficult problem in the general case as a given code change may affect the overall state of a project in various indirect ways.
I'm afraid this might make the CI less stable or predictable, which will make it harder for people to contribute. What is the common use-case for using CI Fuzz in every PR?
They key use case is to catch bugs before they are committed to a project. Many existing oss-fuzz projects already use this, e.g. curl, systemd, libssh and many more, and the feedback is in general quite positive!
Fuzzer are clever and doesn't just brute force.
I think it's a good idea but here it seems to fail and doesn't trig the CI.
=================================================================
==21==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fa1554a1120 at pc 0x000000723393 bp 0x7ffdd3edf670 sp 0x7ffdd3edf668
WRITE of size 1 at 0x7fa1554a1120 thread T0
SCARINESS: 46 (1-byte-write-stack-buffer-overflow)
#0 0x723392 in pcpp::IDnsResource::decodeName(char const*, char*, int) /src/PcapPlusPlus/Packet++/src/DnsResource.cpp:142:15
#1 0x7228f4 in pcpp::IDnsResource::IDnsResource(pcpp::DnsLayer*, unsigned long) /src/PcapPlusPlus/Packet++/src/DnsResource.cpp:16:17
#2 0x5f93df in DnsQuery /src/PcapPlusPlus/Packet++/./header/DnsResource.h:133:56
#3 0x5f93df in pcpp::DnsLayer::parseResources() /src/PcapPlusPlus/Packet++/src/DnsLayer.cpp:190:19
#4 0x5f8529 in init /src/PcapPlusPlus/Packet++/src/DnsLayer.cpp:86:3
#5 0x5f8529 in pcpp::DnsLayer::DnsLayer(unsigned char*, unsigned long, pcpp::Layer*, pcpp::Packet*) /src/PcapPlusPlus/Packet++/src/DnsLayer.cpp:22:2
#6 0x5c795e in pcpp::UdpLayer::parseNextLayer() /src/PcapPlusPlus/Packet++/src/UdpLayer.cpp:116:21
#7 0x5db5ad in pcpp::Packet::setRawPacket(pcpp::RawPacket*, bool, unsigned long, pcpp::OsiModelLayer) /src/PcapPlusPlus/Packet++/src/Packet.cpp:80:13
#8 0x5dce80 in pcpp::Packet::Packet(pcpp::RawPacket*, bool, unsigned long, pcpp::OsiModelLayer) /src/PcapPlusPlus/Packet++/src/Packet.cpp:126:2
#9 0x5704ce in LLVMFuzzerTestOneInput /src/PcapPlusPlus/Tests/Fuzzers/FuzzTarget.cpp:58:15
#10 0x467c63 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp
#11 0x46744a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp
#12 0x4692a4 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
#13 0x4694d9 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
#14 0x458f1d in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp
#15 0x481f62 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#16 0x7fa1560f00b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#17 0x4307fd in _start (build-out/FuzzTarget+0x4307fd)
@seladb it seems that only reproductible crash are declared as failure. It is used in lot of open source project.
If CIFuzz doesn’t find a crash during the allotted time, the CI test passes (green check). 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.)
@seladb it seems that only reproductible crash are declared as failure. It is used in lot of open source project.
If CIFuzz doesn’t find a crash during the allotted time, the CI test passes (green check). 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.)
@clementperon here is my concern: if people open PRs and CIFuzz will fail on (new) issues that are not related to the code that they added or changed, this will get them frustrated and discourage them from continuing to contribute to this project. CI is there to help us make sure that code changes don't break the existing functionality. If it's not deterministic or predictable it becomes more challenging to rely on it
@seladb fuzzing is already deployed in lot of OSS project and doesn't discourage people to contribute.
The fuzzing is deterministic and predictable when a failure occured, the job try to check if it's reproductible and if it is, you have the input buffer to retrig the error. Also the fuzzing doesn't failed if it was not triggered by the actual PR.
I think, this is what is happening here in this PR, the fuzzing found errors but it doesn't fail because it was present before his PR. (Maybe I'm wrong on this point)
Also to me the stack trace is pretty clear and can be easily check if the issue is trig by new code or not: #0 0x723392 in pcpp::IDnsResource::decodeName(char const*, char*, int) /src/PcapPlusPlus/Packet++/src/DnsResource.cpp:142:15
thanks for your comments @clementperon , let me research it some more and get back to you
Hi @DavidKorczynski ,
I'm trying to run a Fuzzing in the Cirrus CI. I have built the libpcap as static and run the fuzzer for 10min but it seems to never find a pattern that match a PCAP
My MR is here: https://github.com/seladb/PcapPlusPlus/pull/1045/files and the Cirrus CI is : https://cirrus-ci.com/task/6684920035147776
Do you have an idea what i'm missing ?
This PR is open for a very long time... I don't think we want to add fuzzing to our CI at this point so I'll close it now. Thank you @DavidKorczynski for working on it and sorry for the late response